Skip to content
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

ContainerBuilder::getReflectionClass() + autowiring breaks resource cache & debug:config #29019

Closed
weaverryan opened this issue Oct 29, 2018 · 4 comments

Comments

@weaverryan
Copy link
Member

@weaverryan weaverryan commented Oct 29, 2018

Symfony version(s) affected: 4.1.6 (but probably back to 3.4)

Description
Hey guys!

I found a very deep bug related to autowiring & the container resources. Steps to reproduce are below, but here is a description of the issue. Also, this seems to only affect the debug:config and related commands where the container is rebuilt manually. The normal container does not seem to suffer from this problem. The key part is 3 - I believe that's where the bug actually exists.

  1. In AutowirePass, a type somewhere in the system cannot be guessed. And so, AutowirePass:: populateAvailableTypes() is called to try to determine a good error message. This is already slightly flawed, as the error message may ultimately be "hidden" because the definition in question is removed... and so we're doing work unnecessarily. But, that's not the bug - just a little performance thing.

  2. The AutowirePass::populateAvailableTypes() eventually calls: $this->container->getReflectionClass($definition->getClass(), false) on each definition.

  3. Here is the important part: IF the class in question exists (e.g. DoctrineOrmTypeGuesser was the culprit for me) but some of the interfaces that it implements do NOT exist, then the ContainerBuilder::getReflectionClass() method will create and use a ClassExistenceResource for that class where $exists = false.

  4. The next time the container checks to see if its cache is fresh, it will always return false because the class above (e.g. DoctrineOrmTypeGuesser) DOES exist, but its ClassExistenceResource expects it to not exist.

How to reproduce
Reproducer: https://github.com/weaverryan/autowiring-class-existence-reproducer

Steps:

  1. Install form (because it has a good example - DoctrineOrmTypeGuesser) of a class that will be registered as a service, even though some of its parent interfaces do not exist in the app

  2. Create an "autowiring failure" situation that will ultimately be silent. Easy way: add a controller and type-hint it with an entity.

  3. Install security. Not needed for the bug - it's just an easy way to "see" the bug... because the bad behavior causes a weird situation.

  4. Run rm -rf var/cache/* && php bin/console debug:config framework. You'll see this exception:

The service "debug.security.firewall" has a dependency on a non-existent service ".security.
request_matcher.zfHj2lW".

This is not really the bug - it's just a "symptom" of the fact that debug:config will build the container multiple times, because after building it, it immediately looks "stale".

Possible Solution
Make ContainerBuilder::getReflectionClass() a bit smarter: only add the class existence resource if the class really does not exist?

@stof
Copy link
Member

@stof stof commented Oct 29, 2018

if getReflectionClass reported a non-existent thing because of a missing interface on it, we would need to register a ClassExistenceResource on the missing interface though, as having this interface created could change the behavior of the container though.

Creating a ClassExistenceResource is not a bug. The bug is that it creates the wrong one.

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 6, 2018

Fix in #29107

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 6, 2018

the error message may ultimately be "hidden" because the definition in question is removed... and so we're doing work unnecessarily.

fixed in #29108

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 6, 2018

Cheers :)

nicolas-grekas added a commit that referenced this issue Nov 6, 2018
…ing error messages (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] dont track classes/interfaces used to compute autowiring error messages

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29019
| License       | MIT
| Doc PR        | -

This will also improve DX since tracking these files is not needed at all.

Commits
-------

09a0c23 [DI] dont track classes/interfaces used to compute autowiring error messages
nicolas-grekas added a commit that referenced this issue Dec 13, 2018
…-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] compute autowiring error messages lazily

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29019
| License       | MIT
| Doc PR        | -

As suggested in the linked issue:

> the error message may ultimately be "hidden" because the definition in question is removed... and so we're doing work unnecessarily.

Commits
-------

3b3a1bd [DI] compute autowiring error messages lazily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.