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

[Dependency Injection] No Circular reference warning for Symfony 3.4 & Doctrine #24775

Closed
mimol91 opened this issue Oct 31, 2017 · 21 comments
Closed

Comments

@mimol91
Copy link

mimol91 commented Oct 31, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4

Recently I've experienced quite unexpected behavior while using doctrine (#24700) After some deep investigation it seems that reason is simple -> Circular references.

It seems that different Symfony version show or not different errors.

I've tested it on clean Symfony version
It can be simply recreated just just by creating empty class and adding this code to servies.yml

services:
    core.listener.doctrine:
        class: AppBundle\DoctrineListener
        arguments: ['@doctrine.orm.default_entity_manager']
        public: false
        tags:
            - { name: doctrine.orm.entity_listener }

There are three possible solution how application will behave:

Error Id Symfony version Empty cache Service marked as public
1 3.4
1 3.3
0 3.4
0 3.3
0 3.4
2 3.3
0 3.4
0 3.3
Error Id Message
0 No error
1 It's a requirement to specify a Metadata Driver and pass it to Doctrine\ORM\Configuration::setMetadataDriverImpl().
2 Circular reference detected for service "doctrine.orm.default_entity_manager", path: "doctrine.orm.default_entity_manager -> doctrine.orm.default_entity_listener_resolver".

As you can see SF3.4 never mention about circular reference. Which I think still may occur ( or in my case cause some unexpected behavior)
SF 3.3 display (or not ) different error based of condition if service is public or state of cache.

And yes it's again example with Doctrine, however I think DI and circular references are part of Symfony.
(Doctrine version is this same for each of SF version)

Want more magic? Try to install doctrine-migration-bundle (DO NOT register it in AppKernel)
Now if service is private the error about circular reference never shows!

I am totally aware that its not a 'common' case, but I am sure that this missing error about circular reference could save a lot of hours on debugging =). I do not mind if you close this issue, I just try to save other folks time, because they may have similar problem like I do.

@Simperfit
Copy link
Contributor

Simperfit commented Nov 1, 2017

I think something could to be done to improve the DX, what do you want to have in order to ease the debbuging ?

@mimol91
Copy link
Author

mimol91 commented Nov 1, 2017

It would be just great to see this Circular reference detected error in each SF version.

@Simperfit
Copy link
Contributor

Status: Needs Review

@weaverryan
Copy link
Member

Yea, this definitely "smells" weird to me (i.e. a bug) - especially with the different behavior on the different versions and public vs private. Someone needs to dig in a bit.

@mimol91
Copy link
Author

mimol91 commented Nov 3, 2017

@nicolas-grekas
I did git-bisect, and it looks that it stop displaying warning after #23741
I will try to find a reason, however your help will be invaluable

@nicolas-grekas
Copy link
Member

No idea yet, if someone can look at a reproducer or a fix, that'd be great :)

@mimol91
Copy link
Author

mimol91 commented Nov 3, 2017

Seems moving $this->loading[$id] = true;
https://github.com/symfony/symfony/pull/23741/files#diff-1df5b32be44d0f592ec285c6bbf386bbL332
to line 301 solve the problem.
(Previously it was set before condition if (isset($this->fileMap[$id])) ). Probably an typo. Would be great if someone could verify my theory

@nicolas-grekas
Copy link
Member

@mimol91 could you try to work on a PR, fix the fix and a test case? That'd be the best way to prove your theory :)

@mimol91
Copy link
Author

mimol91 commented Nov 3, 2017

Sure, I will try

@mimol91
Copy link
Author

mimol91 commented Nov 3, 2017

Ok,
The problem is with dumping container with 'as_files' => true,.
What happens on SF3.3:

  1. Warmup cache
  2. Request for loading doctrine.orm.default_entity_manager and mark it as lazy
  3. Run CacheWarmerAggregate
  4. Run getDoctrine_Orm_DefaultEntityListenerResolverService
$instance->register(new \AppBundle\DoctrineListener(${($_ = isset($this->services['doctrine.orm.default_entity_manager']) ? $this->services['doctrine.orm.default_entity_manager'] : $this->get('doctrine.orm.default_entity_manager')) && false ?: '_'}));

The important part is that if service is not registered in container it call $this->get('doctrine.orm.default_entity_manager')) && false ?: '_'})); which then check if service is marked as loading (and if is) throw circular ref exception

What happens on SF3.4:
Steps are the same, only getDoctrine_Orm_DefaultEntityListenerResolverService is changed

$instance->register(new \AppBundle\DoctrineListener(${($_ = isset($this->services['doctrine.orm.default_entity_manager']) ? $this->services['doctrine.orm.default_entity_manager'] : $this->load(__DIR__.'/getDoctrine_Orm_DefaultEntityManagerService.php')) && false ?: '_'}));

Similar as previous service does not exist, so $this->load(__DIR__.'/getDoctrine_Orm_DefaultEntityManagerService.php') is executed. -> There is not check if services['doctrine.orm.default_entity_manager'] is in loading state

How I think it can be fixed,

$instance->register(new \AppBundle\DoctrineListener(${($_ = isset($this->services['doctrine.orm.default_entity_manager'])
    ? $this->services['doctrine.orm.default_entity_manager']
    : $this->loading['doctrine.orm.default_entity_manager']
        ? $this->get('doctrine.orm.default_entity_manager')
        : $this->load(__DIR__.'/getDoctrine_Orm_DefaultEntityManagerService.php')) && false ?: '_'}));

I think its the root cause, would be great if someone would create PR with fix, becaue I am not sure how can I write tests for it, I do not feel comfortable with this phpDumper.

@Koc
Copy link
Contributor

Koc commented Nov 3, 2017

😨 Is it possible extract some variables to make generated code more readable?

@nicolas-grekas
Copy link
Member

@Koc the generated code must fit in one expression. Readability is not a criteria.

@mimol91 or better: do the check in the first lines of the generated files? Btw, doesn't this issue exist in 3.4 also, when as_files=false?

@mimol91
Copy link
Author

mimol91 commented Nov 4, 2017

The issue does not exist when changing as_files to false - its start to display circular ref error (also

require_once $cache->getPath();
$this->container = new $class();

) has to be changed

Ha. You can do check, but You will need to import ServiceCircularReferenceException. It was my 1st tough, however I was not sure if its only 'check' done in container while invoking get()

@nicolas-grekas
Copy link
Member

@mimol91 can you provide a fork of the standard edition that reproduces the issue? It would help a lot.

@mimol91
Copy link
Author

mimol91 commented Nov 4, 2017

@nicolas-grekas
Copy link
Member

This is a variant of #19362, the reproducer provided there exhibits exactly the same issue.
Now trying to figure out how to fix that.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 4, 2017

Fixed in #24822 (not by throwing an exception, but by handling properly the situation to create a fixed dependency tree.)

fabpot added a commit that referenced this issue Nov 5, 2017
…grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix "almost-circular" dependencies handling

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

In a situation like the following one, we used to trigger a circular reference exception. But this was a false positive, as the reference is resolvable without hitting the circle. Fixing this exception could be considered as a new feature (because no existing config needs it, since it fails). But for 3.4, this should be considered a bug fix, as reported in #24775: not handling this situation now means creating broken service trees.

``` php
$containerBuilder = new ContainerBuilder();

$container->register('foo', FooCircular::class)->setPublic(true)
   ->addArgument(new Reference('bar'));

$container->register('bar', BarCircular::class)
    ->addMethodCall('addFoobar', array(new Reference('foobar')));

$container->register('foobar', FoobarCircular::class)
    ->addArgument(new Reference('foo'));

$foo = $containerBuilder->get('foo');
```

Commits
-------

beb4df7 [DI] Fix "almost-circular" dependencies handling
@fabpot fabpot closed this as completed Nov 5, 2017
@gpekz
Copy link
Contributor

gpekz commented May 19, 2018

Recently I had this error while clearing the cache : "Segmentation fault (core dumped)"
The problem was a Circular Reference.
The context : injection of a service (which use doctrine manager / repository) into a doctrine subscriber.

@kgrieco
Copy link

kgrieco commented Sep 26, 2018

I'm also having problem with circular reference on Symfony 4.1.4. I have a service (A) where some other services (B, C, D) are injected into it, one of them has Doctrine. If I try to inject service A into a Doctrine subscriber I get an exception:

PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Maximum function nesting level of '500' reached, aborting! in /home/kenzo/PhpstormProjects/sarah-server/vendor/doctrine/persistence/lib/Doctrine/Common/Persistence/Mapping/Driver/SymfonyFileLocator.php:80
Stack trace:
#0 /home/kenzo/PhpstormProjects/sarah-server/vendor/doctrine/persistence/lib/Doctrine/Common/Persistence/Mapping/Driver/SymfonyFileLocator.php(80): array_merge(Array, Array)
#1 /home/kenzo/PhpstormProjects/sarah-server/vendor/doctrine/persistence/lib/Doctrine/Common/Persistence/Mapping/Driver/SymfonyFileLocator.php(61): Doctrine\Common\Persistence\Mapping\Driver\SymfonyFileLocator->addNamespacePrefixes(Array)
#2 /home/kenzo/PhpstormProjects/sarah-server/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/SimplifiedYamlDriver.php(40): Doctrine\Common\Persistence\Mapping\Driver\SymfonyFileLocator->__construct(Array, '.orm.yml')
#3 /home/kenzo/PhpstormProjects/sarah-server/var/cache/dev/ContainerQhEyouc/AppDevDebugProjectContainer.php( in /home/kenzo/PhpstormProjects/sarah-server/vendor/doctrine/persistence/lib/Doctrine/Common/Persistence/Mapping/Driver/SymfonyFileLocator.php on line 80

@stof
Copy link
Member

stof commented Sep 26, 2018

@kgrieco that issue is closed since almost 1 year. It is unlikely that your issue is the same:

  • original issue was fixed
  • code is not the same anymore
  • the error message you get is not the same one

Please create a new issue rather than commenting on closed ones (you were lucky that I took attention to the email notification for this comment, as the notification is the only way someone could see it. We don't go read closed issues again otherwise)

@cluis13915
Copy link

Just in case someone is struggling with this yet, I found that getting repository from services constructor throws "Segmentation fault (core dumped)" when clearing the cache. Removing getRepository() calls from constructors makes the error goes away.

Bad:

public function __construct(EntityManagerInterface $em)
{
  $this->repository = $em->getRepository(Empresa::class);
}

public function myMethod()
{
  $empresa = $this->repository->find(...);
}

Good:

public function __construct(EntityManagerInterface $em)
{
  $this->em = $em;
}

public function myMethod()
{
  $empresa = $this->em->getRepository(Empresa::class)->find(...);
}

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

No branches or pull requests