Problem/Motivation

Can not remove plugins from a config entity during a request because DefaultLazyPluginCollection::$pluginInstances acts as a static cache.

Proposed resolution

Remove unconfigured plugin instances when DefaultLazyPluginCollection::setConfiguration() is called.

Remaining tasks

Review

User interface changes

None

API changes

None

Original issue summary

One example is that image style effects are removed from an exported config file, the removal is not respected when the config file is re-imported.

For example, if I have a set of three image style effects in a file like this:

uuid: 5284900f-2425-4d75-8b49-732c50a2b11e
langcode: en
status: true
dependencies: {  }
name: large
label: 'Large (480×480)'
effects:
  ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
    uuid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
    id: image_scale
    weight: 0
    data:
      width: 480
      height: 480
      upscale: false
  4e5538ff-1350-4c00-a802-2a12d7556b02:
    uuid: 4e5538ff-1350-4c00-a802-2a12d7556b02
    id: image_crop
    weight: 2
    data:
      width: 100
      height: 100
      anchor: right-center
  75733cfc-728d-4553-8d07-7bfa15bca63a:
    uuid: 75733cfc-728d-4553-8d07-7bfa15bca63a
    id: image_desaturate
    weight: 3
    data: {  }
third_party_settings: {  }

...Then if I want to remove the Desaturate effect from the file, the import will not remove the effect in Drupal:

uuid: 5284900f-2425-4d75-8b49-732c50a2b11e
langcode: en
status: true
dependencies: {  }
name: large
label: 'Large (480×480)'
effects:
  ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
    uuid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
    id: image_scale
    weight: 0
    data:
      width: 480
      height: 480
      upscale: false
  4e5538ff-1350-4c00-a802-2a12d7556b02:
    uuid: 4e5538ff-1350-4c00-a802-2a12d7556b02
    id: image_crop
    weight: 2
    data:
      width: 100
      height: 100
      anchor: right-center
third_party_settings: {  }

So, if I re-export the image style effects, the Desaturate effect will remain.

CommentFileSizeAuthor
#32 allow_external_update-2350569-32.patch4.04 KBalexpott
#32 allow_external_update-2350569-32.test-only.patch2.99 KBalexpott
#17 interdiff.txt1.29 KBtim.plunkett
#17 allow_external_update-2350569-17.patch4.04 KBtim.plunkett
#15 2350569.15.patch3.93 KBalexpott
#11 remove-instance-defaultpluginbag-2350569-7-11.interdiff.txt1.09 KBwebflo
#11 remove-instance-defaultpluginbag-2350569-11.patch2.1 KBwebflo
#7 remove-image-effect-config-import-2350569-6.patch2.24 KBwebflo
#6 remove-image-effect-config-import-2350569-6.patch2.24 KBwebflo
#4 remove-image-effect-config-import-2350569.patch1.36 KBwebflo
#1 remove-image-effect-config-import-2350569.patch602 byteswebflo

Comments

webflo’s picture

webflo commented
Status: Active » Needs review
StatusFileSize
new remove-image-effect-config-import-2350569.patch602 bytes

The pluginBag acts like a static cache. This patch enforce rebuilding before entity save.

robertdbailey’s picture

robertdbailey commented
Title: remove image style effect during config import » respect plugin bag property changes during config import
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: remove-image-effect-config-import-2350569.patch, failed testing.

webflo’s picture

webflo commented
Status: Needs work » Needs review
StatusFileSize
new remove-image-effect-config-import-2350569.patch1.36 KB

This issues applied to all config entities with plugin bags. Here is a failing test to show the issue.

Status: Needs review » Needs work

The last submitted patch, 4: remove-image-effect-config-import-2350569.patch, failed testing.

webflo’s picture

webflo commented
Status: Needs work » Needs review
StatusFileSize
new remove-image-effect-config-import-2350569-6.patch2.24 KB
webflo’s picture

webflo commented
StatusFileSize
new remove-image-effect-config-import-2350569-6.patch2.24 KB

Go Testbot!

webflo’s picture

webflo commented
Title: respect plugin bag property changes during config import » Allow external update of ConfigEntity properties that are associated with a PluginBag

New title because all Config entities with PluginBags suffer from this issue.

webflo’s picture

webflo commented
Priority: Normal » Major
Issue tags: +Configurables
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginBag.php
@@ -131,6 +131,15 @@ public function getConfiguration() {
+    $this->pluginInstances = array();

This resetting feels really fragile, let's try to avoid it. On top of it, unit tests.

webflo’s picture

webflo commented
StatusFileSize
new remove-instance-defaultpluginbag-2350569-11.patch2.1 KB
new remove-instance-defaultpluginbag-2350569-7-11.interdiff.txt1.09 KB
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented 
+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginBag.php
@@ -131,8 +131,13 @@ public function getConfiguration() {
+      unset($instance_ids[$instance_id]);
+    }
+    foreach ($instance_ids as $instance_id) {
+      $this->removeInstanceId($instance_id);

It'd be great to have a simple code comment here explaining the unsetting and removal of remaining values.

Berdir queued 11: remove-instance-defaultpluginbag-2350569-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: remove-instance-defaultpluginbag-2350569-11.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2350569.15.patch3.93 KB

Rerolled and fixed up DefaultLazyPluginCollectionTest::testConfigurableSetConfiguration() to actually test what it is says it is testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Also whilst this issue will only appear when a config entity with plugins gets updated and used in the same request it is a pretty big gotcha so I agree with the major priority.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new allow_external_update-2350569-17.patch4.04 KB
new interdiff.txt1.29 KB

I think this makes perfect sense.
In response to #12, I clarified some of the comments, and chose to rename the variable to be as specific as possible.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs issue summary update

Though I don't know if the issue title accurately maps to what we're fixing. And not just because Bags are now Collections.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: allow_external_update-2350569-17.patch, failed testing.

tim.plunkett queued 17: allow_external_update-2350569-17.patch for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review 
Drupal\migrate_drupal\Tests\d6\MigrateFileTest
File /tmp/some-temp-file.jpg could not be copied to temporary://some-temp-file.jpg.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Allow external update of ConfigEntity properties that are associated with a PluginBag » Allow external update of ConfigEntity properties that are associated with a PluginCollection
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

RTBC again, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: allow_external_update-2350569-17.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
Test name	Pass	Fail	Exception
CollapsedDrupal\simpletest\Tests\SimpleTestBrowserTest	101	1	0
Message	Group	Filename	Line	Function	Status
"0 fails, 0 exceptions" found	Other	SimpleTestBrowserTest.php	146	Drupal\simpletest\Tests\SimpleTestBrowserTest->testTestingThroughUI()

An unrelated fail.

alexpott queued 17: allow_external_update-2350569-17.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: allow_external_update-2350569-17.patch, failed testing.

alexpott queued 17: allow_external_update-2350569-17.patch for re-testing.

alexpott queued 17: allow_external_update-2350569-17.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community

Back to green after all the testbot fun...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new allow_external_update-2350569-32.test-only.patch2.99 KB
new allow_external_update-2350569-32.patch4.04 KB

Test only patch as requested by @xjm.

The last submitted patch, 32: allow_external_update-2350569-32.test-only.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I had to spend a bit of time reading through the API docs to understand why this was necessary (turns out there are actually plugin API docs that cover this now!) but the test-only patch helps clarify that. Thanks @alexpott.

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultLazyPluginCollectionTest.php
@@ -224,16 +224,19 @@ public function testConfigurableGetConfiguration() {
+    $this->defaultPluginCollection->setConfiguration(['cherry' => ['value' => 'kiwi', 'id' => 'cherry']]);

Sounds delicious.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

  • xjm committed a6274c0 on 8.0.x 
    Issue #2350569 by webflo, alexpott, tim.plunkett: Allow external update...

Status: Fixed » Closed (fixed)

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