8000 [DependencyInjection] Cannot dump compiled Container if a factory service is inlined · Issue #13913 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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
Closed
paultregoing opened this issue Mar 12, 2015 · 7 comments
Closed

[DependencyInjection] Cannot dump compiled Container if a factory service is inlined #13913

paultregoing opened this issue Mar 12, 2015 · 7 comments
Labels
Bug Critical DependencyInjection

Comments

@paultregoing
Copy link

The DIC v2.6 introduced Definition::setFactory() (and corresponding config syntax) and deprecated setFactoryService(), 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:

services:
    factory_service:
        class: Some\Factory
        public: false

    my_service:
        class: MyClass
        factory: [ '@factory_service', 'getThing' ]

Compiling a container built with this config, then attempting to dump...

$container->compile();
$dumper = new Symfony\Component\DependencyInjection\Dumper\XmlDumper($container);
$dumper->dump();

...throws the following ContextErrorException:

Warning: DOMElement::setAttribute() expects parameter 2 to be string, object given

If one reverts to the old config syntax/Definition setter methods, there is no issue. The following config results in a dumpable, compiled container:

services:
    factory_service:
        class: Some\Factory
        public: false

    my_service:
        class: MyClass
        factory_service: factory_service
        factory_method: getThing

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 calls inlineArguments() on Definition::getFactory(), but there is no corresponding inlineArguments() call on Definition::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 we print_r() the compiled container's definitions property...

Inlined (uses setFactory()):

    [my_service] => Symfony\Component\DependencyInjection\Definition Object
        (
            [class:Symfony\Component\DependencyInjection\Definition:private] => MyClass
            [file:Symfony\Component\DependencyInjection\Definition:private] => 
            [factory:Symfony\Component\DependencyInjection\Definition:private] => Array
                (
                    [0] => Symfony\Component\DependencyInjection\Definition Object
                        (
                            [class:Symfony\Component\DependencyInjection\Definition:private] => Some\Factory

Not inlined (uses setFactoryService()):

    [my_service] => Symfony\Component\DependencyInjection\Definition Object
        (
            [class:Symfony\Component\DependencyInjection\Definition:private] => MyClass
            [file:Symfony\Component\DependencyInjection\Definition:private] => 
            [factory:Symfony\Component\DependencyInjection\Definition:private] => 
            [factoryClass:Symfony\Component\DependencyInjection\Definition:private] => 
            [factoryMethod:Symfony\Component\DependencyInjection\Definition:private] => factoryMethod
            [factoryService:Symfony\Component\DependencyInjection\Definition:private] => Symfony\Component\DependencyInjection\Reference Object
                (
                    [id:Symfony\Component\DependencyInjection\Reference:private] => factory_service
                    [invalidBehavior:Symfony\Component\DependencyInjection\Reference:private] => 1
                    [strict:Symfony\Component\DependencyInjection\Reference:private] => 1
                )

The stacktrace of the exception shows that, during XmlDumper::addService() the factory or factoryService object is passed into a DOMElement::setAttribute() call, and therefore must be castable to a string. There is no __toString() method available on Symfony\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 the Symfony\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:

services:
    factory_service:
        class: Some\Factory
        public: false

    my_service:
        class: MyClass
        factory: [ '@factory_service', 'getThing' ]

Could be represented as

services:
    my_service:
        class: MyClass
        factory: [ 'Some\Factory', 'getThing' ]

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:

services:
    factory_service:
        class: Some\Factory
        arguments: ['Some\Vague\HelperClass']
        public: false

    my_service:
        class: MyClass
        factory: [ '@factory_service', 'getThing' ]

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"...
@stof
Copy link
Member
stof commented Mar 12, 2015

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)

@stof
Copy link
Member
stof commented Mar 12, 2015

for the bug reported here, I see 3 solutions:

  1. preventing the inlining of factories
  2. adding a way to support inline service definitions for factories in the XML format
  3. stop dumping the compiled container as XML (and stop considering it as a supported use case)

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 ->compile() again)

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)

@xabbuh xabbuh mentioned this issue Mar 12, 2015
Merged
@xabbuh
Copy link
Member
xabbuh commented Mar 12, 2015

I removed the ability to inline service factories in #13914.

@paultregoing
Copy link
Author

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

@xabbuh xabbuh mentioned this issue Mar 16, 2015
Closed
fabpot added a commit that referenced this issue 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
@fabpot fabpot closed this as completed Mar 17, 2015
@mpdude
Copy link
Contributor
mpdude commented Apr 4, 2015

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?

@xabbuh
Copy link
Member
xabbuh commented Apr 11, 2015

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

@mpdude
8000
Copy link
Contributor
mpdude commented Apr 11, 2015

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 deps=low test, this faulty branch was used.

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!

iteman added a commit to phpmentors-jp/pageflower-bundle that referenced this issue Jun 12, 2015
@iteman
change the version of symfony/dependency-injection to ~2.6,>=2.6.5 …
40963cc 
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Critical DependencyInjection
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @stof @mpdude @paultregoing @xabbuh
0