Skip to content

Autowiring tries to wire unused/non-service classes #22977

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

Closed
samsch opened this issue May 30, 2017 · 10 comments
Closed

Autowiring tries to wire unused/non-service classes #22977

samsch opened this issue May 30, 2017 · 10 comments

Comments

@samsch
Copy link

samsch commented May 30, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? Not sure
RFC? no
Symfony version 3.3.0

It seems autowiring tries to auto create the dependencies for a service before checking that the service is actually used.

It seems like it should not attempt to retrieve a dependency for a service which isn't used. If this error is actually expected or a limitation of the system, then I think there should be at least a note in the documentation, both in the autowiring doc and in the upgrade guide.

For my actual use case, I have a data class which is used as a form data object. The constructor for this class takes a service which I directly pass to it where I instantiate the object. Autowiring tries to find this service during container compile and fails.

Steps to reproduce

  1. Create new 3.3 app symfony new test-app 3.3
  2. Add new src/AppBundle/Class1 and src/AppBundle/Class2 with source as below.
  3. Run app (php bin/console server:start)
  4. Open webpage to error

Error

AutowiringFailedException
Cannot autowire service "AppBundle\Services\DataThings\OtherData": argument "$data" of method "__construct()" must have a type-hint or be given a value explicitly.

Class1 source

<?php

namespace AppBundle;

class Class1
{
    public function __construct(Class2 $c2)
    {}
}

Class2 source

<?php

namespace AppBundle;

class Class2
{
    public function __construct($something)
    {}
}
@xabbuh
Copy link
Member

xabbuh commented May 30, 2017

When using the new configuration services will be automatically registered, but you can explicitly exclude some classes (take a look at section http://symfony.com/doc/current/service_container/3.3-di-changes.html#step-4-auto-registering-services from your second link).

@samsch
Copy link
Author

samsch commented May 31, 2017

If you always use a organized-by-type folder structure where all your data classes are in a single folder, that might be a reasonable solution. However, for projects organized by domain, where data classes will not necessarily be under any specific folder, you would likely need to add an exception for each individual data class, or use a naming convention to exclude the files. Either solution is fragile.

In my opinion, file exclusions don't properly address the issue.

If there wasn't this note: "...if any of the services are not used in your code, they're automatically removed from the compiled container." (ref) then I might assume another solution isn't possible. However, at some level unused services can be dropped. It seems reasonable to ignore errors thrown while trying to load a unused "service".

I'm not familiar with the autowiring implementation, but perhaps if dropping a unused service and it's errors isn't possible (or breaks things), is there maybe a way we could mark non-service classes at the class level? Perhaps an annotation: @NoAutowire?

@xabbuh
Copy link
Member

xabbuh commented May 31, 2017

However, at some level unused services can be dropped. It seems reasonable to ignore errors thrown while trying to load a unused "service".

Before being able to drop services the container first needs to build the entire dependency graph. This is not possible if some of the service definitions are incomplete.

@weaverryan
Copy link
Member

This is legit. There's an error here:

if ($container->hasDefinition($exception->getServiceId()) || in_array($exception->getServiceId(), $inlinedIds)) {

Class2 is removed from the container (because neither Class1 nor Class2 are referenced). But, Class2 is first inlined into Class1, so the if statement thinks the service was not actually removed, but just inlined. So, the exception is thrown when it should not be.

I'll see if I can find a fix :)

@weaverryan
Copy link
Member

PR #22993

fabpot added a commit that referenced this issue May 31, 2017
…emoved (weaverryan)

This PR was squashed before being merged into the 3.3 branch (closes #22993).

Discussion
----------

[DI] Autowiring exception thrown when inlined service is removed

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | yes (on a new & internal method)
| Tests pass?   | yes
| Fixed tickets | #22977
| License       | MIT
| Doc PR        | n/a

We suppress autowiring exceptions if a service is ultimately removed from the container. This fixes a bug where we incorrectly report that a service was NOT removed, when really, it WAS removed. This happens when `ServiceA` is inlined in `ServiceB`... but then `ServiceB` is removed from the container for being unused.

Commits
-------

793b9a0 [DI] Autowiring exception thrown when inlined service is removed
@fabpot fabpot closed this as completed May 31, 2017
@smcjones
Copy link

smcjones commented Jan 25, 2018

Would be nice to be able to exclude folders by wildcard.

Example:

exclude: '../../src/AppBundle/Utils/{*/Exception,Tests}'

@nicolas-grekas
Copy link
Member

Isn't it working already if you remove the curly brackets? (glob makes them special only if they contain a comma to define an alternative)

@smcjones
Copy link

Hmm... I did not try it without curly brackets. I can try tomorrow. I did update my example to include more than one comma separated value as that was my use case.

@sinner
Copy link

sinner commented Aug 3, 2018

I'm still having this trouble.

@xabbuh
Copy link
Member

xabbuh commented Aug 3, 2018

@sinner Please open a new issue with an example that allows to reproduce your issue.

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

No branches or pull requests

8 participants