Skip to content

debug:container --types & debug:autowiring can fail with missing deps #24639

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.

Sign up for GitHub

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

Jump to bottom
Closed
weaverryan opened this issue Oct 19, 2017 · 3 comments
Closed

debug:container --types & debug:autowiring can fail with missing deps #24639

weaverryan opened this issue Oct 19, 2017 · 3 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@weaverryan
Copy link
Member

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3 (I believe)

Here's the setup:

  1. Use Flex
  2. Install SensioFrameworkExtraBundle
  3. Run bin/console debug:container --types

screen shot 2017-10-19 at 7 26 28 pm

The issue is that SensioFrameworkExtraBundle registers a service that relies on symfony/security. Since security is not present, this service is removed: https://github.com/sensiolabs/SensioFrameworkExtraBundle/blob/master/DependencyInjection/Compiler/OptimizerPass.php#L27

However, I believe that the debug:container command uses the container before some of these removal passes. So, this service is still present when we check to see if the class exists: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php#L248. The autoloader causes the class to be loaded, and error because the parent class is missing.

So... long way of saying, we just need a safer way of checking to see if the service id is an existent class.
@nicolas-grekas
Copy link
Member

What about not checking that (but checking that there is a \ in the id instead?) Would the list be that different?

@weaverryan
Copy link
Member Author

Great question! For a Flex app with some basic dependencies installed, the only alias that does not have a \ in it is Twig_Environment... which of course also has a Twig\Environment alias.

So in practice, if we switched to look for a \... that would fix it. But there would be edge-case libraries where we would be in-accurate.

What if we did:

A) If there is a \, then we immediately assume it is a valid type (without calling class_exists())
B) if not, we use class_exists. Or better, we use reflection with a try/catch (like AutowirePass)

@weaverryan weaverryan mentioned this issue Oct 29, 2017
Merged
@weaverryan
Copy link
Member Author

Fixed in #24744

fabpot added a commit that referenced this issue Oct 29, 2017
@fabpot
bug #24744 debug:container --types: Fix bug with non-existent classes…
1da0ba5 
… (weaverryan)

This PR was merged into the 3.3 branch.

Discussion
----------

debug:container --types: Fix bug with non-existent classes

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24639
| License       | MIT
| Doc PR        | n/a

I've just tested manually that this *does* fix the issue I described in #24639.

Oddly enough, in a "stock" Flex project, after this patch, there is one *additional* "type" that's reported:

> Symfony\Component\PropertyAccess\PropertyAccessorInterface   alias for "property_accessor"

That is a valid type... for some reason `interface_exists()` return false for this (??? maybe a quirk of my machine). Anyways, this is also "fixed" with this new approach.

Commits
-------

4bb9d82 Fixing a bug where non-existent classes would cause issues
@fabpot fabpot closed this as completed Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @weaverryan @nicolas-grekas @xabbuh @carsonbot