8000 [DependencyInjection] do not inline service factories by xabbuh · Pull Request #13914 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

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
Merged
merged 1 commit into from
Mar 17, 2015
Merged

[DependencyInjection] do not inline service factories #13914

merged 1 commit into from
Mar 17, 2015

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Mar 12, 2015
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 the
used container, is not capable to dump inlined factories.

@xabbuh xabbuh mentioned this pull request Mar 12, 2015
Closed
@xabbuh
Copy link
Member Author
xabbuh commented Mar 12, 2015

fixed tests (failures are not related to this pull request)

@nicolas-grekas
Copy link
Member

I've been hit by the bug, patch fixes the issue 👍

@fabpot
Copy link
Member
fabpot commented Mar 16, 2015

I would have preferred to actually inline the service instead of keeping it as a separate method.

@fabpot
Copy link
Member
fabpot commented Mar 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
Copy link
Member Author
xabbuh commented Mar 16, 2015

@fabpot What kind of behaviour would you expect in the XmlDumper when it gets passed an "inlined" service factory? Should it throw an exception?

@nicolas-grekas
Copy link
Member

XML-dumping a definition using a private factory fails without your patch, and works with. This could be tested.

@stof
Copy link
Member
stof commented Mar 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
Copy link
Member Author
xabbuh commented Mar 16, 2015

@nicolas-grekas The issue with your suggestion is that this is no related to the XmlDumper itself, but is related to how the instance looks like that is passed to it (i.e. the container as it has been modified by all compiler passes). So I added a test for the InlineServiceDefinitionPass which ensures that factories are not inlined.

@xabbuh
Copy link
Member Author
xabbuh commented Mar 16, 2015

@stof Doing so reveals that the XmlDumper also isn't capable of dumping inlined configurators (I suggested a way to dump them in #13557). Should I remove this possibility here too (given that adding such an option might be considered a new feature)?

@xabbuh
do not inline service factories
663ae9f 
The `XmlDumper`, which is used in the full-stack framework to dump the
used container, is not capable to dump inlined factories.
@xabbuh
Copy link
Member Author
xabbuh commented Mar 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
Copy link
Member
fabpot commented Mar 17, 2015

Thank you @xabbuh.

@fabpot fabpot merged commit 663ae9f into symfony:2.6 Mar 17, 2015
fabpot added a commit that referenced this pull request Mar 17, 2015
@fabpot
bug #13914 [DependencyInjection] do not inline service factories (xab…
9b736c8 
…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 the
used container, is not capable to dump inlined factories.

Commits
-------

663ae9f do not inline service factories
@xabbuh xabbuh deleted the issue-13913 branch March 17, 2015 07:14
@xabbuh
Copy link
Member Author
xabbuh commented Mar 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?

This was referenced Mar 17, 2015
Closed
Merged
fabpot added a commit that referenced this pull request Mar 23, 2015
@fabpot
bug #14013 [DependencyInjection] prevent inlining service configurato…
99330cb 
…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 service
configurators. Since Symfony applications dump the compiled container
in XML, inlined configurators will break this process.

We did something similar before with service factories in #13914.

Commits
-------

34619fe prevent inlining service configurators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xabbuh @nicolas-grekas @fabpot @stof
0