-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autowire Drupal container params and plain values #6061
base: 13.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hoping that @greg-1-anderson can take a look at changes in this file. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,9 @@ | |
use Grasmash\YamlCli\Command\UnsetKeyCommand; | ||
use Grasmash\YamlCli\Command\UpdateKeyCommand; | ||
use Grasmash\YamlCli\Command\UpdateValueCommand; | ||
use League\Container\Container as DrushContainer; | ||
use League\Container\Container; | ||
use League\Container\DefinitionContainerInterface as DrushContainer; | ||
use Psr\Container\ContainerInterface; | ||
use Psr\Log\LoggerAwareInterface; | ||
use Psr\Log\LoggerInterface; | ||
use Robo\ClassDiscovery\RelativeNamespaceDiscovery; | ||
|
@@ -342,18 +344,16 @@ public function instantiateServices(array $bootstrapCommandClasses, DrushContain | |
|
||
// Prevent duplicate calls to delegate() by checking for state. | ||
if ($container && !$drushContainer->has('state')) { | ||
assert($drushContainer instanceof Container); | ||
// Combine the two containers. | ||
$drushContainer->delegate($container); | ||
} | ||
foreach ($bootstrapCommandClasses as $class) { | ||
$commandHandler = null; | ||
|
||
try { | ||
if ($this->hasStaticCreateFactory($class) && $this->supportsCompoundContainer($class, $drushContainer)) { | ||
// Hurray, this class is compatible with the container with delegate. | ||
$commandHandler = $class::create($drushContainer); | ||
} elseif ($container && $this->hasStaticCreateFactory($class)) { | ||
$commandHandler = $class::create($container, $drushContainer); | ||
if ($staticCreateFactoryArguments = $this->getStaticCreateFactoryArguments($class, $drushContainer, $container)) { | ||
$commandHandler = $class::create(...$staticCreateFactoryArguments); | ||
} elseif (!$container && $this->hasStaticCreateEarlyFactory($class)) { | ||
$commandHandler = $class::createEarly($drushContainer); | ||
} else { | ||
|
@@ -372,22 +372,60 @@ public function instantiateServices(array $bootstrapCommandClasses, DrushContain | |
return $commandHandlers; | ||
} | ||
|
||
/** | ||
* Determine if the first parameter of the create method supports our container with delegate. | ||
*/ | ||
protected function supportsCompoundContainer($class, $drush_container): bool | ||
protected function getStaticCreateFactoryArguments(string $class, DrushContainer $drushContainer, ?DrupalContainer $drupalContainer = null): ?array | ||
{ | ||
if (!method_exists($class, 'create')) { | ||
return null; | ||
} | ||
|
||
$reflection = new \ReflectionMethod($class, 'create'); | ||
$hint = (string)$reflection->getParameters()[0]->getType(); | ||
return is_a($drush_container, $hint); | ||
if (!$reflection->isStatic()) { | ||
return null; | ||
} | ||
|
||
$params = $reflection->getParameters(); | ||
|
||
$args = []; | ||
$type = ltrim((string) $params[0]->getType(), '?'); | ||
if ($drupalContainer && is_a($type, DrupalContainer::class, true)) { | ||
// The factory create() method explicitly expects a Drupal container as 1st argument. | ||
$args[] = $drupalContainer; | ||
} elseif (is_a($type, DrushContainer::class, true)) { | ||
// The factory create() method explicitly expects a Drush container as 1st argument. Don't add a 2nd | ||
// argument. If the Drupal container has been initialized, it's already a delegate. | ||
return [$drushContainer]; | ||
} elseif ($drupalContainer && is_a($type, ContainerInterface::class, true)) { | ||
// The factory create() method expects a container of any type as st argument and the Drupal container has | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do we mean "has already been initialized"? |
||
// been initialized. Pass it as 1st argument. | ||
$args[] = $drupalContainer; | ||
} | ||
|
||
// Add Drush container as 2nd argument if the method expects one. | ||
if (isset($params[1])) { | ||
$type = ltrim((string) $params[1]->getType(), '?'); | ||
if (is_a($type, ContainerInterface::class, true)) { | ||
$args[] = $drushContainer; | ||
} | ||
} | ||
Comment on lines
+403
to
+409
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont like commands and AutowireTrait having multiple numbers of params and type hints in create(). How about if we ask commands going forward to always type hint to |
||
|
||
return $args; | ||
} | ||
|
||
/** | ||
* Check to see if the provided class has a static `create` method. | ||
*/ | ||
protected function hasStaticCreateFactory(string $class): bool | ||
{ | ||
return static::hasStaticMethod($class, 'create'); | ||
if (!$hasStaticCreateFactory = static::hasStaticMethod($class, 'create')) { | ||
return false; | ||
} | ||
$reflection = new \ReflectionMethod($class, 'create'); | ||
// Check first two param typings. | ||
foreach (array_slice($reflection->getParameters(), 0, 2) as $param) { | ||
$typing = ltrim((string) $param->getType(), '?'); | ||
$hasStaticCreateFactory = $hasStaticCreateFactory && is_a($typing, ContainerInterface::class, true); | ||
} | ||
return $hasStaticCreateFactory; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Drupal\woot; | ||
|
||
final class AutowireTestService implements AutowireTestServiceInterface | ||
{ | ||
public function __toString(): string | ||
{ | ||
return 'Hello World!'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Drupal\woot; | ||
|
||
interface AutowireTestServiceInterface extends \Stringable {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
|
||
namespace Custom\Library; | ||
|
||
use Drupal\Component\DependencyInjection\ContainerInterface; | ||
use Drupal\Core\Routing\RedirectDestinationInterface; | ||
use Drush\Commands\DrushCommands; | ||
use Psr\Log\LoggerInterface; | ||
|
||
final class CreateFactoryExpectsDrupalContainer extends DrushCommands | ||
{ | ||
public function __construct( | ||
public readonly string $string, | ||
public readonly RedirectDestinationInterface $redirectDestination, | ||
) { | ||
parent::__construct(); | ||
} | ||
|
||
public static function create(ContainerInterface $container): self | ||
{ | ||
return new self('a string as it is', $container->get('redirect.destination')); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
<?php | ||
|
||
namespace Custom\Library; | ||
|
||
use Drush\Commands\DrushCommands; | ||
use League\Container\DefinitionContainerInterface; | ||
use Psr\Log\LoggerInterface; | ||
|
||
final class CreateExpectsDrushContainer extends DrushCommands | ||
{ | ||
public function __construct( | ||
public readonly string $string, | ||
public readonly LoggerInterface $log, | ||
) { | ||
parent::__construct(); | ||
} | ||
|
||
public static function create(DefinitionContainerInterface $container): self | ||
{ | ||
return new self('a string as it is', $container->get('logger')); | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am seeing an error when I run |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Custom\Library\Drush\Commands; | ||
|
||
use Drupal\woot\AutowireTestService; | ||
use Drupal\woot\AutowireTestServiceInterface; | ||
use Drush\Attributes as CLI; | ||
use Drush\Boot\DrupalBootLevels; | ||
use Drush\Commands\AutowireTrait; | ||
use Drush\Commands\DrushCommands; | ||
use League\Container\ContainerAwareInterface; | ||
use League\Container\ContainerAwareTrait; | ||
use Symfony\Component\DependencyInjection\Attribute\Autowire; | ||
|
||
final class AutowireTestCommands extends DrushCommands | ||
{ | ||
use AutowireTrait; | ||
|
||
public function __construct( | ||
#[Autowire('a string as it is')] | ||
private readonly string $argListStringValue, | ||
#[Autowire(null, 'woot.autowire_test')] | ||
private readonly AutowireTestService $argListContainerService, | ||
#[Autowire(null, null, null, null, 'foo')] | ||
private readonly string $argListContainerParam, | ||
#[Autowire(value: 'a string as it is')] | ||
private readonly string $namedArgStringValue, | ||
#[Autowire(service: 'woot.autowire_test')] | ||
private readonly AutowireTestService $namedArgContainerService, | ||
#[Autowire(param: 'foo')] | ||
private readonly string $namedArgContainerParam, | ||
private readonly AutowireTestServiceInterface $noAutowireAttributeContainerService, | ||
) { | ||
parent::__construct(); | ||
} | ||
|
||
#[CLI\Command(name: 'test_autowire:drupal-container')] | ||
#[CLI\Bootstrap(level: DrupalBootLevels::FULL)] | ||
#[CLI\Help(hidden: true)] | ||
public function drupal(): string | ||
{ | ||
$values = []; | ||
$constructor = new \ReflectionMethod($this, '__construct'); | ||
foreach ($constructor->getParameters() as $param) { | ||
$values[] = (string) $this->{$param->getName()}; | ||
} | ||
return implode("\n", $values); | ||
} | ||
|
||
#[CLI\Command(name: 'test_autowire:drush-container')] | ||
#[CLI\Bootstrap(level: DrupalBootLevels::NONE)] | ||
#[CLI\Help(hidden: true)] | ||
public function drush(): string | ||
{ | ||
$values = []; | ||
$constructor = new \ReflectionMethod($this, '__construct'); | ||
foreach ($constructor->getParameters() as $param) { | ||
$values[] = (string) $this->{$param->getName()}; | ||
} | ||
return implode("\n", $values); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is based on https://github.com/drupal/drupal/blob/11.x/core/lib/Drupal/Core/DependencyInjection/AutowireTrait.php. Looks like this PR wants to expand its duties considerably. I'm not opposed to that, but I fear that this may get tricky to maintain as Symfony evolves.