[DependencyInjection] Add #[Constructor] attribute
#57654
Conversation
b0f5ee8 to
b04a840
Compare
#[Constructor] attribute
There was a problem hiding this comment.
As I review this PR (which I suggested ;) ) I realize that this will force inspecting every methods of every services, while currently we need to inspect only classes.
I might be wrong because maybe inspecting all methods is already what we do in some commonly used bundle?
If not, then it might be better to add the attribute at the class level #[Constructor(method: 'create'], or at least before that to bench the impact that scanning all methods has in practice so that we don't decrease the DX by increasing the compilation time.
Up to dig this aspect?
| if (!$reflector->isStatic()) { | ||
| throw new LogicException(sprintf('Cannot use "%s::%s()" as a constructor: the method should be static.', $reflector->class, $reflector->name)); | ||
| } | ||
| $declaringClass = $reflector->getDeclaringClass(); |
There was a problem hiding this comment.
by using the declaring class, we don't check the methods on the actual class
| @@ -699,6 +700,28 @@ public function load(array $configs, ContainerBuilder $container): void | |||
| $container->registerAttributeForAutoconfiguration(AsSchedule::class, static function (ChildDefinition $definition, AsSchedule $attribute): void { | |||
| $definition->addTag('scheduler.schedule_provider', ['name' => $attribute->name]); | |||
| }); | |||
| $container->registerAttributeForAutoconfiguration(Constructor::class, static function (ChildDefinition $definition, Constructor $attribute, \ReflectionMethod $reflector): void { | |||
| if (!$reflector->isStatic()) { | |||
There was a problem hiding this comment.
we should also check that the method is public
| $reachableStaticMethods[] = $method->name; | ||
| if ($declaringClass->name !== $method->class || // If it was declared by a parent | ||
| $reflector->name === $method->name || // If it's the method we're processing | ||
| 0 === \count($method->getAttributes(Constructor::class)) // If it doesn't have the attribute |
There was a problem hiding this comment.
| 0 === \count($method->getAttributes(Constructor::class)) // If it doesn't have the attribute | |
| !$method->getAttributes(Constructor::class) // If it doesn't have the attribute |
| } | ||
| $factory = $definition->getFactory(); | ||
| // Set the factory if it isn't set yet or if it's set to a method at the same/higher inheritance level | ||
| if (null === $factory || (\is_array($factory) && \in_array($factory[1], $reachableStaticMethods, true))) { |
There was a problem hiding this comment.
$factory cannot be set IIRC because $definition is empty here
|
Closing as this stalled. Please submit again once you could have a look at my previous comment. The points I raised there make the current approach a no-go IMHO. |
|
For the record, this can be achieve using |
Description
This PR introduces a new
#[Constructor]attribute to simplify the factory declaration using attributes.Right now to declare a factory using attributes, you have to use
#[Autoconfigure(constructor: 'staticMethod')]on the class.This new
#[Constructor], applied to a static method, will set it as the factory of the declaring class.Example
Todo
I had some trouble finding a way to test this. There aren't many tests on autoconfiguration callbacks and most of them are copy/paste (AsMessageHandler autoconfiguration and its test for example). Any input on how to tests this or where the tests should live would be appreciated.