The KeyConfigOverrides class, which is the controller for the key.config_override service, currently uses static method calls to the global Drupal object. Instead, those services should be passed in as arguments.

CommentFileSizeAuthor
#18 interdiff.txt1.77 KBrlhawk
#18 2939854-18.patch2.72 KBrlhawk
#16 key-override-recursion-2939854-16.patch3.28 KBdouggreen
#12 2939854-11.patch3.68 KBrlhawk
#2 2939854-1.patch3.33 KBrlhawk

Comments

rlhawk created an issue. See original summary.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Active » Needs review
StatusFileSize
new 2939854-1.patch3.33 KB

Here's a patch that implements this. Two notes:

  • I've added static call fallbacks in the constructor to minimize disruption when upgrading; these can be removed in a later release
  • The cache.data service is passed in and is not used yet, but it will be soon

Status: Needs review » Needs work

The last submitted patch, 2: 2939854-1.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Needs work » Needs review
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

The failing tests seem to be the result of a Drupal core bug in 8.6.

tynor’s picture

tynor commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • rlhawk committed b7dbb4b on 8.x-1.x 
    Issue #2939854 by rlhawk, tynor: Pass services as arguments to key....
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Reviewed & tested by the community » Fixed
gaydamaka’s picture

gaydamaka commented

After merging this patch I have the error after a site-install:

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "language_manager", path: "language_request_subscriber -> language_manager -> config.factory -> key.config_override -> key.repository -> entity_type.manager -> string_translation -> string_translator.locale.lookup". in Drupal\Component\DependencyInjection\Container->get() (line 141 of /home/d07d/www/core/lib/Drupal/Component/DependencyInjection/Container.php).

Modules language and locale are enabled. I have the same error after Locale module installing. I’ve found the issue which fixes this one https://www.drupal.org/project/key/issues/2924257 but after merging this patch doesn’t work.

manuel garcia’s picture

manuel garcia commented

On #2924257: Circular reference prevents module installation it was found that switching to static loading of the Entity Type Manager, instead of using dependency injection, fixed the circular reference issue. However here we are re-introducing dependency injection, although for different things. I don't know enough about the previous bug, but this is probably why the issue is reappearing?

mattlt’s picture

mattlt
he/him/they/them
Primary language English
Location Freeport, MN
commented

I'm getting the following error when running `drush cr` when updating to this commit, more circular references…

[error]  Circular reference detected for service "entity_type.manager", path: "workbench_moderation.inline_editing_disabler -> workbench_moderation.moderation_information -> entity_type.manager -> string_translation -> string_translator.locale.lookup -> config.factory -> key.config_override -> key.repository".
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Fixed » Needs review
StatusFileSize
new 2939854-11.patch3.68 KB

I guess we'll need to stick with static calls until the circular dependency issue can be sorted out. Here's a patch that reverts #2924257: Circular reference prevents module installation and adds an update hook to clear the cache.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

I meant that the patch reverts commit b7dbb4b from this issue.

douggreen’s picture

douggreen commented

I have another patch coming, please wait to commit this.

douggreen’s picture

douggreen commented

See also #2936885: Calling EntityTypeManager::getDefinition during ConfigFactoryOverrideInterface::loadOverrides can lead to corrupt entity type handlers

douggreen’s picture

douggreen commented
StatusFileSize
new key-override-recursion-2939854-16.patch3.28 KB

New patch attached, reverts some of the recursion and dependency injection.

douggreen’s picture

douggreen commented

Some more context...

Key module implemented key config overrides in the 1.6 release. We don't use them.

Config overrides introduced new code that runs early in the loading process, that sometimes causes recursion. Whenever the config is loaded, it triggers the key module override, which tries to load several services and potentially load a key, to see if it overrides the config item being loaded.

The primary culprit that we've noticed is the entity type manager and locale. This patch solves the problem by catching an exception and returning with no overrides. The consequence is, that anything that throws an exception like this, loads too early for key module to override.

The first locale aware configuration item that is loaded will probably trigger the recursion. In our case it was syslog.settings.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
StatusFileSize
new 2939854-18.patch2.72 KB
new interdiff.txt1.77 KB

We can't remove the $inOverride check. It's critical to the override functionality. I'd also like to keep the static call fallbacks, just to ease updating.

douggreen’s picture

douggreen commented

I think/suspect that the exception does the same thing as both those checks, but I'm fine with the extra code.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented

The exception catch doesn't duplicate the $inOverride check. They address different issues.

I'll commit this and prepare a new release.

  • rlhawk committed b6c5afb on 8.x-1.x authored by douggreen 
    Issue #2939854 by douggreen, rlhawk: Pass services as arguments to key....
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
commented
Status: Needs review » Fixed
douggreen’s picture

douggreen commented

Yes, you were right, we needed that as well.

Status: Fixed » Closed (fixed)

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