When configuration is deleted the static cache of configuration is not cleared properly from the config factory.

Files: 
CommentFileSizeAuthor
#12 2395515.12-interdiff.txt593 bytesBerdir
#12 2395515.12.patch5.49 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,783 pass(es). View
#11 2395515.11.patch5.47 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,754 pass(es). View
#11 1-11-interdiff.txt729 bytesalexpott
#1 2395515.1.patch4.76 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,700 pass(es), 2 fail(s), and 0 exception(s). View
#1 2395515.1-test-only.patch1.29 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,540 pass(es), 1 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
1.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,540 pass(es), 1 fail(s), and 0 exception(s). View
4.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,700 pass(es), 2 fail(s), and 0 exception(s). View

Adding the event listener means that deleted configuration objects are always removed from the static cache - so we can improve so of the ConfigFactory code to have a little less complexity.

The last submitted patch, 1: 2395515.1-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2395515.1.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

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.

Gábor Hojtsy’s picture

Priority: Major » Critical
Issue tags: +Configuration system

Status: Needs work » Needs review

catch queued 1: 2395515.1.patch for re-testing.

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 1: 2395515.1.patch, failed testing.

Wim Leers’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
729 bytes
5.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,754 pass(es). View

So editor_form_filter_admin_format_submit() was comparing apples with oranges. This meant it was calling delete on an object but then continuing to use the same object - this was fine whilst the config factory was keeping a stash of the object in the static cache but once this was removed the bug became uncovered.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,783 pass(es). View
593 bytes
+++ 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.

alexpott’s picture

@Berdir thanks!

I think id() and getEditor() methods are different.

  /**
   * {@inheritdoc}
   */
  public function id() {
    return $this->format;
  }

  /**
   * {@inheritdoc}
   */
  public function getEditor() {
    return $this->editor;
  }

Basically what is happening here is that if the Editor configuration entity is configuring a different editor (eg. ckeditor or aloha) then the submit handler is deleting the existing editor config entity for the filter format (eg. full_html, filtered_html) you are configuring.

  • catch committed d05559a on 8.0.x
    Issue #2395515 by alexpott, Berdir: Config static cache is not cleared...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

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.