🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@ruudk
Copy link
Contributor

@ruudk ruudk commented Jan 23, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #48392
License MIT

This is a proof of concept for #48392

Needs some tests.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had more something like below in mind.
Of course without any changes to AbstractRecursivePass.
If you want to process container.excluded services, you'd first need to extract them from the container and remove the tag on them.

--- a/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
+++ b/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
@@ -162,6 +162,11 @@ abstract class FileLoader extends BaseFileLoader
             if (interface_exists($class, false)) {
                 $this->interfaces[] = $class;
             } else {
+                $r = $this->container->getReflectionClass($class);
+                if (!$r->isInstantiable()) {
+                    $this->addContainerExcludedTag($class, $source);
+                    continue;
+                }
                 $this->setDefinition($class, $definition = $getPrototype());
                 if (null !== $errorMessage) {
                     $definition->addError($errorMessage);
@@ -179,7 +184,6 @@ abstract class FileLoader extends BaseFileLoader
                 if (!$autoconfigureAttributes) {
                     continue;
                 }
-                $r = $this->container->getReflectionClass($class);
                 $defaultAlias = 1 === \count($interfaces) ? $interfaces[0] : null;
                 foreach ($r->getAttributes(AsAlias::class) as $attr) {

@nicolas-grekas
Copy link
Member

(Loosely related: this makes me think we could add a #[Constructor] attribute that could be used on a static constructor to let autowiring spot them.)

@ruudk
Copy link
Contributor Author

ruudk commented Jan 23, 2024

If you want to process container.excluded services, you'd first need to extract them from the container and remove the tag on them.

Hmm, but then I have to reflect on all of those excluded services, to find attributes on the class again. Basically, doing everything from scratch that Symfony already handles.

Does that make sense? We can rely on registerAttributeForAutoconfiguration for almost everything (classes, methods, properties, etc...), except when the constructor is private. I feel like we should handle this, as I'm sure a lot of people are not aware of this hidden logic (private constructor means no service).

@nicolas-grekas
Copy link
Member

then I have to reflect on all of those excluded services, to find attributes on the class again

not really: you can build a side container with only the service that are of interest to you, remove that tag from them, and run the compiler passes you want on it by compiling the container

without any changes to AbstractRecursivePass

this is the critical part - we cannot change this behavior

but let me know if you have another idea that would fit this constraint

@ruudk
Copy link
Contributor Author

ruudk commented Jan 23, 2024

What if we do the following:

  • Revert changes to AbstractRecursivePass
  • Tag service with container.not_instantiable when not instantiable
  • This allows processing the attributes
  • When attributes are processed, have another tag that removes definitions tagged with container.not_instantiable from the container, just like container.excluded.
  • Or, have a pass that applies container.excluded tag for definitions with container.not_instantiable tag.

@pounard
Copy link
Contributor

pounard commented Jun 27, 2024

I like this issue because it's near from what I attempt to do: read attributes at compile time on non-services classes. Yet, near is not enough, because this probably would allow to deal with excluded services, but I'm not sure extending usage of this to, in my use case, messenger messages, would be accepted by core contributors. @nicolas-grekas what is your opinion about this?

@nicolas-grekas
Copy link
Member

I just reopened the topic and submitted #59712 to fill the gap.
Thanks for the help pushing this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DependencyInjection] Allow registering attributes on classes that cannot be instantiated

6 participants