Entities with plugin collections should be updated before serialization

I'm roughly +10000000000 to fixing this. We're having to code around this limitation in page_manager work right now, and it was simple once we figured it out, but then again, most things are.

Eclipse

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -365,6 +365,31 @@ public function preSave(EntityStorageInterface $storage) {
   +        foreach ($vars as $key => $value) {
   +          if ($plugin_collection === $value) {
   +            $keys_to_unset[] = $key;
   +          }
   +        }
   

   This could be replaced by array_filter() easily.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -365,6 +365,31 @@ public function preSave(EntityStorageInterface $storage) {
   +    foreach ($keys_to_unset as $key) {
   +      unset($vars[array_search($key, $vars)]);
   +    }
   

   Can't you use an array_diff here?

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -365,6 +365,32 @@ public function preSave(EntityStorageInterface $storage) {
   +        $keys_to_unset += array_filter($vars, function ($value) use ($plugin_collection) {
   +          return $plugin_collection === $value;
   +        });
   ...
   +    if (!empty($keys_to_unset)) {
   +      $vars = array_diff($vars, array_keys($keys_to_unset));
   +    }
   

   This doesn't look tested. I'm missing something, I can't see why it is here.

2. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
   @@ -308,6 +311,37 @@ public function testCalculateDependenciesWithThirdPartySettings() {
   +    $this->assertFalse(in_array('pluginCollection', $entity_keys));
   

   $this->assertContains() has a better message when it goes wrong.

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityBaseUnitTest.php
@@ -308,6 +311,36 @@ public function testCalculateDependenciesWithThirdPartySettings() {
+    // Ensure the plugin collection is not stored.
+    $this->assertNotContains('pluginCollection', $entity->__sleep());

Should we call serialize() here instead?

Shouldn't this be extracted into a helper method that \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() can then also call?

@Wim Leers I wondered the smae thing put see #4

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -365,6 +365,32 @@ public function preSave(EntityStorageInterface $storage) {
   +      foreach ($this->getPluginCollections() as $plugin_config_key => $plugin_collection) {
   +        // Save any changes to the plugin configuration to the entity.
   +        $this->set($plugin_config_key, $plugin_collection->getConfiguration());
   

   So this portion is the same in ::sleep() and ::preSave().

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -365,6 +365,32 @@ public function preSave(EntityStorageInterface $storage) {
   +        // If the plugin collections are stored as properties on the entity,
   +        // mark them to be unset.
   +        $keys_to_unset += array_filter($vars, function ($value) use ($plugin_collection) {
   +          return $plugin_collection === $value;
   +        });
   

   It's this portion that is different.

Note that in #4, the unset() calls were happening in the logic itself. Now it's just being returned, to be unset later. So I think it is possible to have it in a shared helper method. In the ::__sleep() method, you use the return value to unset those entity properties, in the ::preSave() method you just don't do anything with that return value.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -355,6 +349,44 @@ public function preSave(EntityStorageInterface $storage) {
+  }
+
+
+  /**

Oops, one newline too many here.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -355,6 +349,44 @@ public function preSave(EntityStorageInterface $storage) {
+      $vars = get_object_vars($this);
...
+        $keys_to_unset += array_filter($vars, function ($value) use ($plugin_collection) {
+          return $plugin_collection === $value;
+        });

Well now we're doing this on every save - for no reason...

Solved.

I agree with @tim.plunkett

it was cleaner and easier to understand by duplicating the code...

Alright. Re-uploading #11 unchanged.

At first I had NFI what this patch was doing, now I do, and I now agree that the duplicated complexity is minimal.

Sorry for the detour!

I tested the patch in #23 with the test-only patch in #2393387-32: Add test for editing image effect when configuration form is Ajax enabled, and it seems that it fixes that issue too. Bumping to major as the other issue is triaged major.

I think this is eligible for 8.0.x since implementing a magic method is not an API change. Also this is fixing a core bug and unblocking contrib.

Committed 9bd6902 and pushed to 8.0.x and 8.1.x. Thanks!

I can confirm #2393387: Add test for editing image effect when configuration form is Ajax enabled was fixed by this commit, great!

Would appreciate a review of #2393387-49: Add test for editing image effect when configuration form is Ajax enabled to decide whether to close the issue or still add some test to prevent regressions.

For the record, if anyone else ends up looking at this.

This did break one of our modules because we had this logic duplicated. But then the key already didn't exist in it anymore, so array_search() returned FALSE, it casted that to 0 and removed the id from the serialized entity: #2693501: Fix failing ProcessingWebTest

The code here could have the same problem, although I guess we can rely on those being set in $vars. We might still want to make it type safe and check that the return of array_search() is not FALSE.