Problem/Motivation

Two problems following #2098119: Replace config context system with baked-in locale support and single-event based overrides:

1. The ConfigGlobalOverrideSubscriber was left around but never invoked.
2. There is no way to tell if overrides are currently enabled and operations with overrides may leave them in a state not desired. Code should return to the state it was before fiddling with overrides locally, not change global state permanently.

Proposed resolution

1. Remove this file.
2. Add a hasOverrides() method (or something along those lines) and use like getLanguage() to get prior value and return to that as needed.

Remaining tasks

Extend the hasOverrides() use to more places as needed.

User interface changes

None.

API changes

Add hasOverrides() method to ConfigFactory.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, post-config-context-cleanup.patch, failed testing.

alexpott’s picture

Can we also make the enable and disable methods return the ConfigFactory object so we can chain calls?

And the hasOverrides method and effects look like they need testing.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Yeah the enabled and disabled methods already return $this :) On the need for test, yup :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review

post-config-context-cleanup.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, post-config-context-cleanup.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.8 KB
1.74 KB

Added simple test coverage by extending the existing tests. Also looks like cannot be installed due to the core service reference to the subscriber that does not exist anymore obviously :)

tstoeckler’s picture

Category: Task » Bug report

Test looks good. I might be mistaken, but the last hunks in the patch show that this is actually fixing a bug, right?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Oops, wanted to do this as well.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

I don't think this can be RTBC yet. I only modified one file of all where the enable/disable construct is used. Also @alexpott was not entirely happy with the boilerplate required and suggested we move that logic to the config factory. Eg.

$had_overrides = $configFactory->ensureOverrideFree();
// Now has no overrides.
if ($had_overrides) {
  $configFactory->enableOverrides();
}

Or something along those lines. He actually suggested constants returned on whether it was already disabled or just disabled. I think we may also just want to do the language equivalent API:

$old_state = $configFactory->getOverrideState();
$configFactory->setOverrideState(FALSE);
// Now has no overrides.
$configFactory->setOverrideState($old_state);

One line less needed and is the same pattern as the languages.

Both of these are less boilerplate compared to what is in the patch now. What do people think?

Anonymous’s picture

i prefer the second version using something like the language pattern.

Gábor Hojtsy’s picture

All right, then this renames:

hasOverrides() (introduced in prior patches, not in core) => getOverrideState()
enableOverrides()/disableOverrides() => setOverrideState()

Also updates all uses to this pattern. I did not do setting back to the old state in the tests, since we are in a more controlled environment there. But did it everywhere outside of the tests (even in some more controlled environments like the config installer on principle).

Interdiff would almost be as big as the patch itself so did not roll that one.

This should have the complete changeset proposed here.

Status: Needs review » Needs work

The last submitted patch, 11: post-config-context-cleanup-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.19 KB
525 bytes

Especially useful if I'm actually setting the value :)

The last submitted patch, 13: post-config-context-cleanup-12.patch, failed testing.

Gábor Hojtsy’s picture

Title: Cleanup after configuration context removal » Config overrides may spill over to undesired places
Priority: Normal » Major
Issue tags: +Configuration system

Elevating to major due to the override spill-over bug explained.

Gábor Hojtsy’s picture

Should not have changed where each config factory is taken from. (This sets back several config factory calls to how they were before the patch). Should just change the method call :)

Berdir’s picture

Priority: Major » Critical
Status: Needs review » Needs work
Issue tags: +Performance

Started looking into the preload issue and found some things that need to be fixed in the config override code. No specific order and not everything is equally problematic but there are some pretty serious performance regressions, so setting to critical for now.

a) Trivial and easy to fix performance problem. DrupalKernel::initializeContainer() calls $factory->get('system.module')->load()->get('enabled'), the explicit load() does an unconditional call to $cached_storage->read() but the config has already been loaded by the config factory at that point. As discussed with @beejebus, maybe load() should be protected but that seems like a separate issue to figure out.

b) Fixing that, I noticed that ConfigFactory/CachedStorage isn't good at dealing with non-existing config files. color.module tries to load a non-existing color.bartik (might be a bug on it's own), then fun things happen:

1. ConfigFactory::get('color.bartik')
2. ""::loadMultiple()
3. CachedStorage::loadMultiple(array('color.bartik', 'language.config.en.color.bartik'))
4. Cache miss on both of them.
5. FileStorage::readMultiple() => read() for both of them, still not existing.
6. Back to CacheFactory::get(), loadMultiple() did not return anything, so we go into the else
7. CachedStorage::read('language.config.en.color.bartik') ($this->storage->read($this->getLanguageConfigName($this->language->id, $name)); => that config file name helper method can return FALSE!?)
8. another cache miss, another attempt to load the file from FileStorage, obviously another fail :)
9. So no config and no overrides (suprise! ;)), we return an empty config object, but this is not the end, all good things are 10 ;)
10. Now color.module calls get('load'), that somehow thinks that it hasn't been loaded yet and just to be sure tries to load color.bartik again. So another failed cache get, another call to FileStorage. I don't see why we still have the lazy-load implementation inside Config if we pre-load everything again?

Conclusion: Trying to load a single non-existing config file results in 4 cache misses and 4 FileStorage::read() calls.

Suggestion: Apart from trying to clean that mess (sorry :)) up, we need to keep track of non-existing files in CachedStorage, maybe in a single cache?

c) Note that creating a new config file (e.g. saving a new config entity) goes through the same chain right now, this is something that whatever missing-config files cache we will have in CachedStorage needs to somehow being able to handle this, so a save() needs to make sure to update that information.

d) It also applies, even though not quite as severe ("just" a single cache miss + FileStorage::read()) to every single non-existing language override config file read(). See above about needing an efficient non-existing config object cache.

e) And lastly, I'm wondering why we're even trying to load language overrides on my system at all, because this is not a multilingual installation. We can skip the language overrides completely if languageManager->isMultilingual() is FALSE and we might be able to optimize more there but that is going to be a bit more complicated (config entity objects all have a langcode, we don't need to look for overrides if that matches the current language or if it doesn't have such a key and we are using the default language?)

Anonymous’s picture

i've taken berdir's awesome points from #17 and put them in a new issue #2177739: Fix inefficient config factory caching.

berdir++

let's continue this issue with the scope of Gabor's patch.

sun’s picture

Hm. It is very concerning how much could possibly go wrong when a random module/function does not properly maintain the config context/override state.

I wonder whether we should replace this fragile construct with the Automatic TearDown Upon Object Destruction Upon Leaving Local Scope™ pattern, of which you can find a precedent in Database Transactions.

Essentially this:

public function myFunkyControllerCallback() {
  $context = $this->configFactory->createContextOverride('context');
  // Do whatever you want within the context override.
  ...
  return $response;
  // Local scope is left: $context is destructed. → Automatic cleanup, see below.
}
---
class ConfigContextOverride {
  public function __construct($config_factory) {
    $this->configFactory = $config_factory;
    $this->previousContext = $this->configFactory->getContext();
  }

  public function __destruct() {
    $this->configFactory->setContext($this->previousContext);
  }
}
Anonymous’s picture

i really don't like that pattern. magic sucks. i hate in the db layer also.

Gábor Hojtsy’s picture

@sun/@beejeebus: we used to have the context stack to maintain prior states automatically and not needing to know, but automatically getting back to prior state. That was removed due to being confusing. This all comes down to how we think best to solve this, there is really no silver bullet. I think we are going in circles :D

Gábor Hojtsy’s picture

So since #17 was off topic, we are at #16 discussing that patch. It fully converts core to a pattern previously agreed on by @beejeebus :) Other reviews? :)

Anonymous’s picture

alexpott’s picture

I think the magic and complexity of #19 might lead to more problems than it solves.

+++ b/core/modules/locale/lib/Drupal/locale/ParamConverter/LocaleAdminPathConfigEntityConverter.php
@@ -55,9 +55,9 @@ public function convert($value, $definition, $name, array $defaults, Request $re
-      $this->configFactory->disableOverrides();
+      $this->configFactory->setOverrideState(FALSE);
       $entity = $storage->load($value);
-      $this->configFactory->enableOverrides();
+      $this->configFactory->setOverrideState(TRUE);

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateSystemConfigsTest.php
@@ -181,11 +181,11 @@ public function testSystemFile() {
-    \Drupal::configFactory()->disableOverrides();
+    \Drupal::configFactory()->setOverrideState(FALSE);
     $config = \Drupal::config('system.file');
     $this->assertIdentical($config->get('path.private'), 'files/test');
     $this->assertIdentical($config->get('path.temporary'), 'files/temp');
-    \Drupal::configFactory()->enableOverrides();
+    \Drupal::configFactory()->setOverrideState(TRUE);

Why are we not using the $old_state pattern in these places?

Gábor Hojtsy’s picture

@alexpott: the idea there was that these are tests and we *know* how they are invoked. I'll update them to the old_state pattern, so people copy-pasting code will not use wrong patterns.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
16.99 KB
7.22 KB

Updated places where the override was still set sloppy :) This should now always use the $old_state pattern. Anything else? :)

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

catch’s picture

Title: Config overrides may spill over to undesired places » Change notice: Config overrides may spill over to undesired places
Category: Bug report » Task
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
+++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -163,12 +163,13 @@ public function buildForm(array $form, array &$form_state, Request $request = NU
+    // language to use for the form. This avoids repetitively settings and

Should be repetitively setting', but I've fixed this on commit and pushed. Thanks!

alexpott’s picture

FileSize
658 bytes

Fix broken HEAD

alexpott’s picture

Title: Change notice: Config overrides may spill over to undesired places » HEAD BROKEN: Config overrides may spill over to undesired places
Priority: Major » Critical
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: d8.fix-head.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.63 KB

@catch already did a revert. Patches merged.

alexpott’s picture

Title: HEAD BROKEN: Config overrides may spill over to undesired places » Config overrides may spill over to undesired places
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per the above :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2172561.32.patch, failed testing.

Gábor Hojtsy’s picture

Users table not found on custom block creation test is surely not related? :D

Gábor Hojtsy’s picture

Status: Needs work » Needs review

32: 2172561.32.patch queued for re-testing.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

looks ready to me if it's green.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Writing the change notice. No need to tag / retitle, I'll be quick :D

Gábor Hojtsy’s picture

Change notice at https://drupal.org/node/2182813 - this should be done :)

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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