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

Using Workflow & FrameworkBundle leads to error when triggering by type autowiring #23946

Closed
RobertMe opened this Issue Aug 22, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@RobertMe
Contributor

RobertMe commented Aug 22, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.6

I'm using some standalone parts of Symfony without using the full symfony/symfony dependency. This currently means that I'm using the FrameworkBundle and Workflow component. But this leads to some "complicated" problem where compiling the container works fine, but when checking if the container is still fresh it leads to an error, being:

Uncaught exception 'ReflectionException' with message 'Class Symfony\Component\Security\Core\Authorization\ExpressionLanguage not found' in /var/www/symfony-error/vendor/symfony/workflow/EventListener/ExpressionLanguage.php on line 22

After a lot of debugging I found out there are two (possible) issues which lead to this problem.

The first issue is the fact that the class is added as ClassExistsResource due to the now deprecated by type auto wiring. This due to AutoWirePass::populateAvailableType calling $this->container->getReflectionClass which will add this class as resource, because it didn't exist (or in this case couldn't be loaded). But IMHO it is strange to add such class as resource because of the auto wiring. As it means that classes which aren't used in the compiled container (which is the case for this ExpressionLanguage class, the service for it doesn't exist as it is unused) are still checked when checking if the container is still "fresh". While it is completely unsure whether such service would actually suffice to be used as auto wire argument (which even most likely is the case in all of these scenarios, that such missing class can't suffice as argument of a completely different service). And I think this also means that all services which are defined will be checked whether the container is still fresh, so either as ClassExistenceResource, or as ReflectionClassResource, even when they aren't available in the compiled container. Which will thus lead to a performance impact in those cases where every call checks if the container is still fresh (so at least dev environment).

The second issue is in ClassExistenceResource, where throwOnRequiredClass seems to fail. For the $i = 1 + array_search($autoloadFrame, $trace, true); line the array_search is false in my case (on PHP 5.6), so $i will become 1 which in my case means that the actual spl_autoload_call is tested. Furthermore if the check did work, it would end up testing on the include call from Composer which would still mean that it would throw in this case.

To demonstrate the issue I've created a test project: https://github.com/RobertMe/symfony-error The first time the page will load fine (and display "loaded"), but when refreshing the page it gives a fatal error due to aboves exception. As can be observed the ExpressionLanguage class isn't present as service in the dumped container, but it is present in the compiled containers meta file, as ClassExistenceResource. When changing the configuration of the Foo service by disabling auto wiring, or creating a new Bar service (matching the type of the argument), it does work. This because in that case the by type auto wiring isn't triggered, and thus the ExpressionLanguage class isn't monitored for existence.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 29, 2017

Member

Hey, thanks for the report and the deep analyse. I think this has been fixed already, could you retry on 3.3.8 (released yesterday)? Please tell us if the issue still happens or just close if it's ok. Thanks.

Member

chalasr commented Aug 29, 2017

Hey, thanks for the report and the deep analyse. I think this has been fixed already, could you retry on 3.3.8 (released yesterday)? Please tell us if the issue still happens or just close if it's ok. Thanks.

@RobertMe

This comment has been minimized.

Show comment
Hide comment
@RobertMe

RobertMe Aug 30, 2017

Contributor

The error is indeed fixed in 3.3.8 but the first point is still valid.

When by type auto wiring is triggered (because it can't autowire an argument based on the new service id = classname logic) every class which is registered in the container (either public or private) is checked and either the ReflectionClassResource or ClassExistenceResource is created for it (and thus checked during every isFresh check). While there is a low chance that such class would actually match the type being searched by the autowiring. This is visible by unserializing the .meta file, which shows that the (updated) test project has 16 resources, while, when adding a Bar service, there are only 14 resources being checked. This difference isn't that much, but in my actual project the difference of course is bigger. So personally I find this a bit odd and I think it would be better if these classes/files weren't checked on every call.

Contributor

RobertMe commented Aug 30, 2017

The error is indeed fixed in 3.3.8 but the first point is still valid.

When by type auto wiring is triggered (because it can't autowire an argument based on the new service id = classname logic) every class which is registered in the container (either public or private) is checked and either the ReflectionClassResource or ClassExistenceResource is created for it (and thus checked during every isFresh check). While there is a low chance that such class would actually match the type being searched by the autowiring. This is visible by unserializing the .meta file, which shows that the (updated) test project has 16 resources, while, when adding a Bar service, there are only 14 resources being checked. This difference isn't that much, but in my actual project the difference of course is bigger. So personally I find this a bit odd and I think it would be better if these classes/files weren't checked on every call.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr
Member

chalasr commented Sep 2, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 3, 2017

Member

That's normal behavior. The missing classes have to be tracked as such. Imagine you create them after the container is dumped: then the container needs to be recompiled.

Member

nicolas-grekas commented Sep 3, 2017

That's normal behavior. The missing classes have to be tracked as such. Imagine you create them after the container is dumped: then the container needs to be recompiled.

@chalasr chalasr added the Config label Sep 3, 2017

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Sep 9, 2017

Member

Can we close?

Member

chalasr commented Sep 9, 2017

Can we close?

@jakzal

This comment has been minimized.

Show comment
Hide comment
@jakzal

jakzal Oct 5, 2017

Member

Closing for reasons given by Nicolas.

Member

jakzal commented Oct 5, 2017

Closing for reasons given by Nicolas.

@jakzal jakzal closed this Oct 5, 2017

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 5, 2017

Member

Well, checking if the container is still fresh should not trigger a ReflectionException though. Invalidating the cached container is not the same than breaking things when checking whether it should be invalidated.

Member

stof commented Oct 5, 2017

Well, checking if the container is still fresh should not trigger a ReflectionException though. Invalidating the cached container is not the same than breaking things when checking whether it should be invalidated.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

@stof my answer applied to #23946 (comment)
the exception is gone in 3.3.8 :)

Member

nicolas-grekas commented Oct 5, 2017

@stof my answer applied to #23946 (comment)
the exception is gone in 3.3.8 :)

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