Problem/Motivation

I was investigating an issue in String module that lead me to this issue.

String runs a yaml plugin discovery inside module and theme folder. The circular dependency occurs when discovery is done for "theme" folders. More details in the issue https://www.drupal.org/project/string/issues/3514618.

Steps to reproduce

- Download "string version 1.1.0" and "symfony_mailer 1.x-dev"
- Enable string and symfonay_mailer module
- Run `drush updb --debug`
- `drush updb` fails with following message

In Container.php line 222:
                                                                                                                                            
  [Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException]                                                       
  Circular reference detected for service "entity_type.manager", path: "entity_field.manager -> entity_type.manager -> plugin.manager.emai  
  l_builder -> entity_type.manager".

Proposed resolution

@TODO

Remaining tasks

@TODO

User interface changes

@TODO

API changes

@TODO

Data model changes

@TODO

Issue fork symfony_mailer-3524894

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

d34dman created an issue. See original summary.

d34dman’s picture

d34dman commented
Related issues: +#3355846: Circular reference on multi-language sites, +#3514618: String + Symfony Mailer causes circular reference for service 'entity_type.manager'
adamps’s picture

adamps commented

I feel it's not really a bug in this module. It comes from a combination of the 2 modules. Either one alone works fine.

AFAICS StringManager calls ThemeHandler which gets config which calls MailerConfigOverride which calls EmailBuilderManager which depends on entity_type.manager.

Where is the circle?? I guess that somehow StringManager has registered in a way that makes it called from entity_type.manager. If you can change that then the problem goes away.

I'm not sure there's much we can change in this module.

d34dman’s picture

d34dman commented

@adamps,

I feel it's not really a bug in this module. It comes from a combination of the 2 modules. Either one alone works fine.

Thanks for the response. The problem could be a bug in Drush or Core.

AFAICS StringManager calls ThemeHandler which gets config which calls MailerConfigOverride which calls EmailBuilderManager which depends on entity_type.manager.

Where is the circle?? I guess that somehow StringManager has registered in a way that makes it called from entity_type.manager. If you can change that then the problem goes away.

The circle is happening over here

string module includes theme handler https://git.drupalcode.org/project/string/-/blob/1.1.x/src/StringManager...

and when symfony_mailer loads plugin.manager.email_builder in MailerConfigOverride https://git.drupalcode.org/project/symfony_mailer/-/blob/1.x/src/Process...
.

I was investigating this issue and even though I don't understand 100%, I see that this circular dependency is being triggered due to config override depending on services that could have dependency on configuration.

However, what if we refactor the MailerConfigOverride to read from a Drupal State instead of calling `plugin.manager.email_builder` ?

So the idea for breaking the loop being,

- After Plugin Discovery, write the overrides into Drupal State.
- While overriding via Module, read the overrides from Drupal State.

This could lead to module override reading from a stale or empty state for some edge cases. But since the override is via module, the effect should be quite minimal.

This should allow you to use Dependency injection in your classes as well. What do you think?

adamps’s picture

adamps commented

I agree that the code in this module is dangerous for causing circles. However it doesn't cause any alone. I don't really understand why that code in "String" module is enough to trigger a circle. Perhaps the exception message about the circle isn't complete?

MailerConfigOverride already has code that tries to avoid a circle. It ignores any config requests before module hander is loaded:

if (!$this->builtCache && $this->moduleHandler->isLoaded()) {

Indeed I was trying to wait until "After Plugin Discovery" but I couldn't see how to test that. I believe we don't need to write anything to state. We can just change the check on module handler to a better one that happens later on in the bootstrap sequence.

d34dman’s picture

d34dman commented

@adamps,

Thanks for quick reply. Actually, the problem is not with the module handler call. As you can see in the fix that i pushed in string module to temporary unblock deployment, https://git.drupalcode.org/project/string/-/commit/f78038a7f26d82c56fbb7... , the circular dependency is caused when $theme_directories = $this->themeHandler->getThemeDirectories(); is executed. And the themeHandler doesn't have the equivalent isLoaded method like module handler does. So not sure how to introduce a similar check for theme handler.

I (maintainer of string module) would like to avoid catching that exception in string module as it can lead to un-expected consequence that plugins in theme folder would never work when symfony_mailer is present in the website.

adamps’s picture

adamps commented
Version: 1.x-dev » 2.x-dev

adamps opened merge request !163

adamps’s picture

adamps commented
Status: Active » Needs review

Here's a patch for 2.x that worked for me.

It's not really systematic😃. We already waited for module handler to load. Now we also wait for EntityTypeManager. Maybe in the future we would have to add another one. However it works for now on my site at least.

  • adamps committed ab49c8d9 on 2.x 
    Issue #3524894 by adamps, d34dman: Circular reference error when used...
adamps’s picture

adamps commented
Status: Needs review » Fixed

I'm going to make a release soon, so I'll commit this now. Please do test and let me know when you have a chance.

d34dman’s picture

d34dman commented

My projects are running on 1.x. I had problem upgrading to 2.x and got distracted and ended up not able to test it. I will try to pick this up later this week.

Meanwhile, I tried to apply the changes to corresponding file in 1.x and it didn't resolve the issue though.

adamps’s picture

adamps commented
Status: Fixed » Needs work

OK thanks

adamps’s picture

adamps commented
Status: Needs work » Fixed

Let's put this back to fixed for now, because that's correct for 2.x.

I'm prioritising my time on 2.x, so I'm aiming to do 1.x backports only for regressions or major bugs.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.