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 dumping Doctrine-like service graphs (bis) #30096

wants to merge 1 commit into
base: 3.4


None yet
3 participants
Copy link

nicolas-grekas commented Feb 6, 2019

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

Same as #30046, with same comment: I'm unable to provide a reproducer for this, the required service reference graph is too crazy.

This change produces the following diff in the generated getDoctrine_Orm_DefaultEntityManagerService.php file:

> $d = ($this->services['doctrine.orm.default_entity_listener_resolver'] ?? $this->load('getDoctrine_Orm_DefaultEntityListenerResolverService.php'));
> if (isset($this->services['doctrine.orm.default_entity_manager'])) {
>     return $this->services['doctrine.orm.default_entity_manager'];
> }
< $a->setEntityListenerResolver(($this->services['doctrine.orm.default_entity_listener_resolver'] ?? $this->load('getDoctrine_Orm_DefaultEntityListenerResolverService.php')));
> $a->setEntityListenerResolver($d);

@ro0NL if you're up to reducing your reproducer at to a simpler one that we could add to the test suite, that'd be awesome!
If not, could you please rename it eg and keep it around?


This comment has been minimized.

Copy link

ro0NL commented Feb 6, 2019

the required service reference graph is too crazy

well, thank you :}

conceptually it evolves around using PSR4 prototyped + autowired services, where EM service is bound with a private alias pointing to the actual doctrine EM service. Cool huh 😅


Im not sure it's related to the scope where services are registered, in my case from an extension config file, but the alias is registered during bundle booting.

Ill try to replicate it in a minimal DI definition 👍 renamed the repo meanwhile.

Confirmed the patch works. Thanks 💯


This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Feb 6, 2019

Status: needs work
I'll try to fix #29637 here also, it's related somehow, and not fixed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment