Skip to content
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

[DI] fix taking lazy services into account when dumping the container #29247

Merged
merged 1 commit into from
Nov 20, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 18, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29246
License MIT
Doc PR -

This PR fixes issues found while working on #29246.
It does fix the infinite loop, but replaces it by an exception (reopening #29078):

It's a requirement to specify a Metadata Driver and pass it to Doctrine\ORM\Configuration::setMetadataDriverImpl()

The full fix is not immediately accessible as it needs some core changes to the dumping logic. Requiring symfony/proxy-manager-bridge works around the issue properly.

See #29251 for 4.2

@ro0NL
Copy link
Contributor

ro0NL commented Nov 18, 2018

Does it mean some form of BC break happend along the way? Im curious as only as of 4.2 things became problematic.. before it worked like a charm. Did i rely on something unsupported actually?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 18, 2018

3.4 is equally affected (that's where this PR applies.) No BC break here, just a core issue with dumping complex service graphs that have varying side effects depending on the state of the fixes. With this PR, the dumped container should always be correct (ie no infinite loop) BUT it doesn't dump setters in the most optimized order when circular loops are involved. This doesn't play well with Doctrine's Configuration objects which expect to be fully defined before being injected.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Nov 18, 2018

I found a workaround: disabling inlining of services with outgoing edges in lazy services is enough to fix the Doctrine use case - ie have a Configuration instance fully ready before injecting it.
In theory, the issue could come back again if the entity manager service is not declared lazy anymore. In practice, this was required with Symfony 2 - and I don't expect such complex graph + expectations to be common.

Should be good enough, PR ready.

For reference, the fully fixed behavior would involve dumping all services using the call stack (ie following references recursively when dumping - where the current logic stops at them) - for history and reference, #29252 contains a reproducer so that if fixing this is needed in the future, we can start from it.

@nicolas-grekas nicolas-grekas merged commit 67d7623 into symfony:3.4 Nov 20, 2018
nicolas-grekas added a commit that referenced this pull request Nov 20, 2018
…e container (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix taking lazy services into account when dumping the container

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29246
| License       | MIT
| Doc PR        | -

This PR fixes issues found while working on #29246.
It *does* fix the infinite loop, ~but replaces it by an exception (reopening #29078)~:
> ~It's a requirement to specify a Metadata Driver and pass it to Doctrine\ORM\Configuration::setMetadataDriverImpl()~

The full fix is not immediately accessible as it needs some core changes to the dumping logic. Requiring `symfony/proxy-manager-bridge` works around the issue properly.

See #29251 for 4.2

Commits
-------

67d7623 [DI] fix taking lazy services into account when dumping the container
@nicolas-grekas nicolas-grekas deleted the di-fix34 branch November 20, 2018 16:40
This was referenced Nov 26, 2018
This was referenced Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants