Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] do not inline service factories#13914
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMar 12, 2015
fixed tests (failures are not related to this pull request) |
nicolas-grekas commentedMar 16, 2015
I've been hit by the bug, patch fixes the issue 👍 |
fabpot commentedMar 16, 2015
I would have preferred to actually inline the service instead of keeping it as a separate method. |
fabpot commentedMar 16, 2015
hmmm, as this breaks the XML dump, we need a test for the XML dumper, to ensure we don't reintroduce the bug later on. |
xabbuh commentedMar 16, 2015
@fabpot What kind of behaviour would you expect in the |
nicolas-grekas commentedMar 16, 2015
XML-dumping a definition using a private factory fails without your patch, and works with. This could be tested. |
stof commentedMar 16, 2015
I suggest adding a test taking each of our container fixture in the testsuite, compiling the container, and then dumping it as XML. this way, we would detect cases whichcannot be dumped |
xabbuh commentedMar 16, 2015
@nicolas-grekas The issue with your suggestion is that this is no related to the |
xabbuh commentedMar 16, 2015
The `XmlDumper`, which is used in the full-stack framework to dump theused container, is not capable to dump inlined factories.
xabbuh commentedMar 16, 2015
By the way, most of the container fixtures were not usable because of undefined parameters. I added tests for the others though as you suggested. |
fabpot commentedMar 17, 2015
Thank you@xabbuh. |
…buh)This PR was merged into the 2.6 branch.Discussion----------[DependencyInjection] do not inline service factories| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#13913| License | MIT| Doc PR |The `XmlDumper`, which is used in the full-stack framework to dump theused container, is not capable to dump inlined factories.Commits-------663ae9f do not inline service factories
xabbuh commentedMar 17, 2015
@fabpot Any opinion on the dumper (see the PR description)? I tend to adapt it as well in case someone uses the dumper without the parameter resolver pass (parameters in all other features seem to be resolved there too). What do you think? |
…rs (xabbuh)This PR was merged into the 2.6 branch.Discussion----------[DependencyInjection] prevent inlining service configurators| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Currently, only the `PhpDumper` is able to dump inlined serviceconfigurators. Since Symfony applications dump the compiled containerin XML, inlined configurators will break this process.We did something similar before with service factories in#13914.Commits-------34619fe prevent inlining service configurators
The
XmlDumper, which is used in the full-stack framework to dump theused container, is not capable to dump inlined factories.