-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Cannot dump compiled Container if a factory service is inlined #13913
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
Comments
Regarding the YamlDumper, it is not meant to be able to dump a compiled container (the Yaml format does not support any of the inlined syntaxes) |
for the bug reported here, I see 3 solutions:
The third solution is not acceptable given that we have a few other places depending on this XML (even though they can create issues in case you load this file in a ContainerBuilder and then run The first solution is probably the best one for a bugfix in existing branch (adding new features in the XML format would logically require being done in 2.7, not in a bugfix), but the XML support should be considered for 2.7 because inlining factories optimizes the container a bit more. @paultregoing a workaround for now would be to keep your factory service public (public services can never be inlined) |
I removed the ability to inline service factories in #13914. |
@stof regarding YamlDumper / dumping a compiled container... perhaps the YamlDumper ought to test for $container->isFrozen() and throw an exception rather than generating a broken config? Just a thought. |
…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
I still see builds of PRs for 2.7 fail with this error message, for example here. Anything I need to consider or is that a different problem? |
@mpdude Although I'm a bit late to the party, the next time you see something similar, rebasing on the latest upstream branch you based your work on, will probably resolve the failures. |
Sure, I tried that. In my particular case it turned out that there were broken tests on another Symfony branch than the one my PR was based on. Then, in the I was able to resolve this and learned a lot about the Symfony test/build system along the way. :-). But anyway - thanks for your help @xabbuh! |
…#5 This changes will avoid `ContextErrorException` as the following: Warning: DOMElement::setAttribute() expects parameter 2 to be string, object given For more information, see symfony/symfony#13913.
The DIC v2.6 introduced
Definition::setFactory()
(and corresponding config syntax) and deprecatedsetFactoryService()
,setFactoryMethod()
etc.If one injects a private service (for use as factory) using the new method, and then compiles the container, the resulting container definition cannot be dumped with
Symfony\Component\DependencyInjection\Dumper\XmlDumper
. For example:Compiling a container built with this config, then attempting to dump...
...throws the following
ContextErrorException
:If one reverts to the old config syntax/Definition setter methods, there is no issue. The following config results in a dumpable, compiled container:
I dug into the problem and concluded that the compiler is inlining service factories only if they are configured using the new method. It will skip inlining if they are set with the old methods. The reason for this lies inside
Symfony\Component\DependencyInjection\Compiler\InlineServiceDefinitionsPass::process
- it callsinlineArguments()
onDefinition::getFactory()
, but there is no correspondinginlineArguments()
call onDefinition::getServiceFactory()
. It would appear that inlined factory services cannot be dumped, because the compiler pass results in the "my_service" Definition object having a Reference object swapped for a Definition. If weprint_r()
the compiled container's definitions property...Inlined (uses
setFactory()
):Not inlined (uses
setFactoryService()
):The stacktrace of the exception shows that, during
XmlDumper::addService()
thefactory
orfactoryService
object is passed into aDOMElement::setAttribute()
call, and therefore must be castable to a string. There is no__toString()
method available onSymfony\Component\DependencyInjection\Definition
...It's worth noting that there is no issue with using the "my_service" itself, after compilation. The optimised, inline factory service is fully usable within the container. This is not a show-stopper for projects using the standalone DIC. However, full-stack symfony projects have an issue:
XmlDumper::dump
is called by theSymfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\CompilerDebugDumpPass
, which is difficult to circumvent, and such an act would be undesirable anyway as debugging container config is a normal part of the development process. In order to mitigate at present: either all factory services must be public, or used multiple times, or defined using the deprecated functionality.For what my opinion is worth (probably not much), I would argue that in-lining of factory services should be disabled until the config syntax is expressive enough to support the resulting compiled container definition: a factory service likely has its own dependencies configured, and these cannot be represented in the current yaml/xml format if they are switched to inline. This is because only factory method injection is currently supported.
For example:
Could be represented as
However, despite the fact that factory service dependencies will be preserved in the compiled container, it is not possible to represent the factory's own constructor/setter dependencies using the inline config form. The following cannot be represented in inline config, because there would be no way to inject the constructor argument:
Finally, I had brief look at how YamlDumper behaves in the above scenario: although YamlDumper does not throw an exception when dumping the compiled container, it is setting the factory array's class parameter to "null"...
The text was updated successfully, but these errors were encountered: