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

[DependencyInjection] mutated service loses its constructor arguments #23325

Closed
MatTheCat opened this issue Jun 29, 2017 · 8 comments
Closed

Comments

@MatTheCat
Copy link
Contributor

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

We have a dependency to a bundle which define this service:

<service
    id="dnl_user.user_registrar"
    class="Dnl\Bundle\UserBundle\Security\User\Registrar"
>
    <argument type="service" id="security.password_encoder"/>
    <argument type="service" id="dnl_user.user_entity_manager"/>
    <argument type="service" id="event_dispatcher"/>
</service>

We override this definition through a compiler pass:

$container
    ->getDefinition('dnl_user.user_registrar')
    ->setClass('Dnl\BackBundle\User\Registrar')
;

Now when injecting the service into a controller action we get

  • when typehinting with Dnl\BackBundle\User\Registrar

Argument 1 passed to Dnl\Bundle\UserBundle\Security\User\Registrar::__construct() must implement interface Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface, none given

  • when typehinting with Dnl\Bundle\UserBundle\Security\User\Registrar

Controller "Dnl\BackBundle\Controller\UserAdminController::newAction()" requires that you provide a value for the "$registrar" argument.

Setter injection seems to be working except for overridden services, is there something I overlooked?

@stof
Copy link
Member

stof commented Jun 29, 2017

The code you are showing is not overwriting the service definition. It is mutating the existing definition.

Overwriting the service indeed looses all constructor arguments (and also all method calls), as it replaces the whole Definition object. But this does not look like what you're doing here.

to help you with your errors, I would need the stack traces too (or at least the location of the error), to know where they happen.

@MatTheCat MatTheCat changed the title [DependencyInjection] overridden service loses its constructor arguments [DependencyInjection] mutated service loses its constructor arguments Jun 29, 2017
@MatTheCat
Copy link
Contributor Author

Here you go:

mlechat base project dev7 ovh-user-admin-new iphone 5

@stof
Copy link
Member

stof commented Jun 29, 2017

OK, so the first issue is because Symfony's autowiring looked for a Dnl\BackBundle\User\Registrar service id (as this is the class it tried to autowire). As it could not find one and the class is in the same top-level namespace than the autowired class (your controller), it created a new service. and this is the one missing the constructor argument. The error does not come from your dnl_user.user_registrar service.
I don't understand why the newly created service does not get autowired properly though. @nicolas-grekas @weaverryan do you have any idea ?

If you want your dnl_user.user_registrar service to be used when autowiring the Dnl\BackBundle\User\Registrar typehint, create a private alias named Dnl\BackBundle\User\Registrar and targetting dnl_user.user_registrar.

@MatTheCat
Copy link
Contributor Author

So I'm not sure, is the issue from my side because I didn't define an alias or from Symfony's side because this should work?

@stof
Copy link
Member

stof commented Jun 29, 2017

Well, both.
If you want your existing service to be used, you need the alias (and in your case, I'm almost sure you want your own service to be used).
But when Symfony creates one, it must configure it properly (or fail loudly if it cannot). Defining a broken service is not good. So I will keep this issue opened at the moment as we need to figure out what's wrong with it.

@MatTheCat
Copy link
Contributor Author

Ok I understand thanks for the explanation!

@nicolas-grekas
Copy link
Member

The current WTF will be fixed by #23350. I guess we can close then?

@javiereguiluz
Copy link
Member

Yes, it looks like we can close this issue. Thanks!

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

6 participants