Config static cache is not cleared properly on delete

Apparently a test failure in CKEditor module's tests. Taking a look at this.

Reproduced, in both the test, and through manual testing. At some point, a FlattenException is thrown that becomes too large to handle. It specifically happens when enabling the 'Styles Combo' plugin through the UI, after which editor_form_filter_admin_format_submit() calls $editor->save(). And now the party begins. Let's follow the stack trace.

Editor::save() -> ConfigEntityStorage->save() -> EntityStorageBase::save() -> ConfigEntityBase::preSave()

Here something interesting happens:

    // If this entity is not new, load the original entity for comparison.
    if (!$this->isNew()) {
      $original = $storage->loadUnchanged($this->getOriginalId());
      // Ensure that the UUID cannot be changed for an existing entity.
      if ($original && ($original->uuid() != $this->uuid())) {
        throw new ConfigDuplicateUUIDException(String::format('Attempt to save a configuration entity %id with UUID %uuid when this entity already exists with UUID %original_uuid', array('%id' => $this->id(), '%uuid' => $this->uuid(), '%original_uuid' => $original->uuid())));
      }
    }

We load the unchanged entity. In there, we break. Let's continue following the stack trace.

EntityStorageBase::loadUnchanged() (to do this, it rests its own static cache) -> EntityStorageBase::load() -> EntityStorageBase::loadMultiple() -> EntityStorageBase::doLoadMultiple() -> EntityStorageBase::mapFromStorageRecords() -> Editor::__construct()

Another particularly interesting point is hit here, in the constructor. Because that looks like this:

  public function __construct(array $values, $entity_type) {
    parent::__construct($values, $entity_type);

    $plugin = $this->editorPluginManager()->createInstance($this->editor);
    $this->settings += $plugin->getDefaultSettings();
  }

Turns out $this->editor === NULL, and hence no plugin is found (now if only we could have string typehints, then we'd have found it much easier…).

The cause for that is that ConfigEntityStorage::doLoadMultiple() calls ConfigEntityStorage::mapFromStorageRecords() with $records = [0 => ''], which then results in entity constructor calls with $values = [], hence $this->editor never is set.

So… what is the root cause of us getting $records = [0 => '']? Something goes wrong in this code in ConfigEntityStorage, so let's dive deeper there:

    // Load all of the configuration entities.
    $records = array();
    foreach ($this->configFactory->loadMultiple($names) as $config) {
      $records[$config->get($this->idKey)] = $config->get();
    }

So, somewhere in ConfigFactory::loadMultiple(), things are going wrong. And yes, this patch does change that method:

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -109,27 +109,20 @@ public function get($name) {
@@ -143,10 +136,8 @@ public function loadMultiple(array $names) {

@@ -143,10 +136,8 @@ public function loadMultiple(array $names) {
     $list = array();
 
     foreach ($names as $key => $name) {
-      // @todo: Deleted configuration stays in $this->cache, only return
-      //   configuration objects that are not new.
       $cache_key = $this->getConfigCacheKey($name);
-      if (isset($this->cache[$cache_key]) && !$this->cache[$cache_key]->isNew()) {
+      if (isset($this->cache[$cache_key])) {
         $list[$name] = $this->cache[$cache_key];
         unset($names[$key]);
       }

However, undoing these changes does not fix the problem for me.

So: I don't know the solution, but at least you should now have a good starting point.

To get to the same debugging, add a conditional breakpoint on this line (currently line 181) in ConfigEntityStorage:

foreach ($this->configFactory->loadMultiple($names) as $config) {

with the following condition:

$id === 'restricted_html' && $prefix === 'editor.editor.'

Then go to admin/config/content/formats/manage/restricted_html, enable CKEditor, save it. Then go back to that form, add the "Styles" button, type something valid in its settings, enable XDebug, and now you will hit that conditional breakpoint.

Critical due to #2392319-19: Config objects (but not config entities) should by default be immutable by @alexpott.

#2386571: Large array structures (e.g. $form) in stack trace results in huge memory usage in FlattenException::flattenArgs() just went in so should fix the huge exception issue.

catch: the huge exception was only a symptom of a deeper problem, see #5.

+++ b/core/modules/editor/editor.module
@@ -215,7 +215,7 @@ function editor_form_filter_admin_format_submit($form, FormStateInterface $form_
   $format_id = $form_state->getFormObject()->getEntity()->id();
   $original_editor = editor_load($format_id);
-  if ($original_editor && $original_editor->getEditor() != $form_state->getValue('editor')) {
+  if ($original_editor && $original_editor->getEditor() != $form_state->getValue(array('editor', 'editor'))) {
     $original_editor->delete();

Had a look at the form alter and submit, the fix is correct, but the code is pretty confusing as we have both 'editor' form values and
$form_state->get('editor'). Also getEditor() shouldn't exist, that should use id() instead. But neither of that is a problem of this issue, the fix is correct.

Fixed a small coding style issue, I think this is good to go.

Committed/pushed to 8.0.x, thanks!

Yes, there are a lot of cases of "unfortunate naming" in editor.module. This is because it was one of the first (if not the first) module using the Config system and an early user of the Plugin system. Since then, we added a lot more boilerplate and rules for both config entities and plugins. I'd love to improve naming, but that requires API breaks, with very little gain overall.