-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Enable replacing of services #7040
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
Conversation
|
||
foreach ($container->findTaggedServiceIds('framework.service_replacer') as $serviceId => $tag) { | ||
if (!isset($tag[0]['replaces'])) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to throw exception here?
Why not override the configuration of the service you want to replace? Can you explain the use case? |
This is not the proper way IMO. A clean way would be to provide a genericcompiler pass handling the composition of services (i.e. exactly my code available in several bundles but with the ids configured through the constructor). And then bundles needed to composer a service could register the compiler pass. @fabpot this code is not overwriting a service. It is wrapping the original one using composition. |
@stof IMO adding a compiler pass by user-defined bundle in some cases can fail eg: Imagine that you want to replace service that is used eg. as form type, by default user bundles are registered as last, so FormPass will already process and inject non-replaced service. Having this as a pass registered by user bundle will require user to move his bundle instantiation in kernel to top. @fabpot also good use case is in referenced issue |
@canni the FormPass injects a reference. When wrapping a service, your own service has the id of the one you were wrapping, so it will be used. |
and the use case defined in the related issue can entirely be solved with my proposal (which is already applied in several bundles by duplicating the code instead of shipping it in Symfony) and was suggested in the issue |
I don't argue that this is better than that, I can change implementation :) both will do the work... Just IMO this feature should be shipped with framework or DI component as is very helpful. Follow DRY code re-pasting everywhere is evil :) Solution with tags is just more generic, and less interrupting to yours everyday work with DI. |
I think a generic compiler pass (shipped in the component) is cleaner than a hackyy solution with tags. Thus, the id used to reference the old service (used as dependency in the wrapper) should be created based on the wrapper id, not on the original id. Otherwise, it break things when 2 wrappers are applied on the same service |
ok, you convinced me, I'll remove tags support, and update cookbook entry, and place this as available compiler pass, then it belongs to DI itself. PS @stof am I right that replaced service should loose tags? So I came to the point, that this should fit into separate bundle like DI extra or something similar |
Tags are not loosed. But they are related to a service definition So they are still on the original sevrice. And IMO, the compiler pass itself belongs to the component. |
Now bundles can register services, that will replace other services/aliases, tagging system fits this scenarion quite well, and makes solution generic.