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.

Files: 
CommentFileSizeAuthor
#32 allow_external_update-2350569-32.patch4.04 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,807 pass(es). View
#32 allow_external_update-2350569-32.test-only.patch2.99 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,823 pass(es), 2 fail(s), and 0 exception(s). View
#17 interdiff.txt1.29 KBtim.plunkett
#17 allow_external_update-2350569-17.patch4.04 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,805 pass(es). View
#15 2350569.15.patch3.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,222 pass(es). View
#11 remove-instance-defaultpluginbag-2350569-7-11.interdiff.txt1.09 KBwebflo
#11 remove-instance-defaultpluginbag-2350569-11.patch2.1 KBwebflo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-instance-defaultpluginbag-2350569-11.patch. Unable to apply patch. See the log in the details link for more information. View
#7 remove-image-effect-config-import-2350569-6.patch2.24 KBwebflo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,906 pass(es). View

Comments

webflo’s picture

Status: Active » Needs review
FileSize
602 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,844 pass(es), 48 fail(s), and 11 exception(s). View

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

robertdbailey’s picture

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

Status: Needs work » Needs review
FileSize
1.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
FileSize
2.24 KB
webflo’s picture

FileSize
2.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,906 pass(es). View

Go Testbot!

webflo’s picture

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

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

+++ 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

FileSize
2.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-instance-defaultpluginbag-2350569-11.patch. Unable to apply patch. See the log in the details link for more information. View
1.09 KB
jhedstrom’s picture

+++ 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.

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,222 pass(es). View

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

alexpott’s picture

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

Status: Needs review » Reviewed & tested by the community
FileSize
4.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,805 pass(es). View
1.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

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’s picture

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

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

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

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’s picture

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’s picture

Status: Needs work » Reviewed & tested by the community

Back to green after all the testbot fun...

alexpott’s picture

FileSize
2.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,823 pass(es), 2 fail(s), and 0 exception(s). View
4.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,807 pass(es). View

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

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.