Prevent PluginBags from reordering the export based on their sort [#2062367]

Updated: Comment #0

Problem/Motivation

DefaultPluginBag provides sorting, which is used by image style/effects, text format/filters, tours/tips, and views/displays.
When reordering the plugins, currently the ordering of the YAML file changes as well. This leads to very messy diffs.
For example, adding a new image style while reordering them produces this

diff --git a/image.style.large.yml b/image.style.large.yml
index e8e62f1..3188e77 100644
--- a/image.style.large.yml
+++ b/image.style.large.yml
@@ -4,6 +4,11 @@ uuid: 102aa409-2b96-42cb-8361-978108189098
 status: '1'
 langcode: en
 effects:
+  2ed54e24-3e08-4593-9c37-554c4b8262f7:
+    uuid: 2ed54e24-3e08-4593-9c37-554c4b8262f7
+    id: image_desaturate
+    weight: '-10'
+    data: {  }
   ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:
     uuid: ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d
     id: image_scale
@@ -12,8 +17,10 @@ effects:
       width: '480'
       height: '480'
       upscale: '1'
-  2ed54e24-3e08-4593-9c37-554c4b8262f7:
-    uuid: 2ed54e24-3e08-4593-9c37-554c4b8262f7
-    id: image_desaturate
-    weight: '-10'
-    data: {  }
+  2f6458fe-5879-48c0-bf00-cdb634f52876:
+    uuid: 2f6458fe-5879-48c0-bf00-cdb634f52876
+    id: image_resize
+    weight: '4'
+    data:
+      width: '200'
+      height: '100'

It is difficult to tell what is being added and what is being moved.

Proposed resolution

Prevent the natural sorting of the plugins from affecting the exported plugin bag. This will make the above change produce a diff like this:

diff --git a/image.style.large.yml b/image.style.large.yml
index e8e62f1..aa6c437 100644
--- a/image.style.large.yml
+++ b/image.style.large.yml
@@ -15,5 +15,12 @@ effects:
   2ed54e24-3e08-4593-9c37-554c4b8262f7:
     uuid: 2ed54e24-3e08-4593-9c37-554c4b8262f7
     id: image_desaturate
-    weight: '-10'
+    weight: '8'
     data: {  }
+  2f6458fe-5879-48c0-bf00-cdb634f52876:
+    uuid: 2f6458fe-5879-48c0-bf00-cdb634f52876
+    id: image_resize
+    weight: '4'
+    data:
+      width: '200'
+      height: '100'

Now it is clear what is being added and what is being moved.

Remaining tasks

Write tests

User interface changes

N/A

API changes

N/A

#2052787: Image style effect ordering exhibits some odd behaviour

Comment	File	Size	Author
#9	plugin-2062367-9.patch	9.73 KB	tim.plunkett
#9
#9	interdiff.txt	1.24 KB	tim.plunkett
#5	plugin-2062367-5-FAIL.patch	7.25 KB	tim.plunkett
#5
#5	plugin-2062367-5-PASS.patch	8.76 KB	tim.plunkett
#5
#5	interdiff.txt	2.86 KB	tim.plunkett
#3	plugin-2062367-3.patch	7.9 KB	dawehner
#3
#3	interdiff.txt	807 bytes	dawehner
#1	plugin-2062367-1.patch	1.77 KB	tim.plunkett
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 10 August 2013 at 20:31

Status:

Active

» Needs review

File	Size
plugin-2062367-1.patch	1.77 KB

This is the patch I used to generate the diff in the "proposed resolution" section.
I don't believe this constitutes as an API Change, merely a bugfix.

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 10 August 2013 at 20:45

@@ -44,6 +44,13 @@ class DefaultPluginBag extends PluginBag {
+  protected $originalOrder;

This should probably be = array() to prevent any errors later.

@@ -58,6 +65,8 @@ public function __construct(PluginManagerInterface $manager, array $configuratio
       $this->instanceIDs = MapArray::copyValuesToKeys(array_keys($configurations));
...
+      $this->originalOrder = $this->instanceIDs;

Should I have done $this->instanceIDs = $this->originalOrder = MapArray? I don't know if we usually do that, or if this is better (especially with the comment)

```
@@ -103,6 +112,12 @@ public function sortHelper($aID, $bID) {
+    $this->instanceIDs = $this->originalOrder + $this->instanceIDs;
```
Probably should rewrite this as $this->instanceIDs = $this->originalOrder + $current_order; for clarity. Also, PHP++ for this working so easily.

Comment #3

dawehner

German

CreditAttribution: dawehner commented 10 August 2013 at 22:09

File	Size
interdiff.txt	807 bytes
plugin-2062367-3.patch	7.9 KB

I am currently not really sure how to really setup a DefaultPluginBag so the new behavior of sorting can be tested, maybe it is also too late.

Comment #4

10 August 2013 at 23:24

Status:

Needs review

» Needs work

The last submitted patch, plugin-2062367-3.patch, failed testing.

Comment #5

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 19 August 2013 at 17:47

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

File	Size
interdiff.txt	2.86 KB
plugin-2062367-5-PASS.patch	8.76 KB

plugin-2062367-5-FAIL.patch	7.25 KB

This works for me.

Comment #6

dawehner

German

CreditAttribution: dawehner commented 19 August 2013 at 18:25

Status:

Needs review

» Reviewed & tested by the community

Perfect ... this issue already adds more test coverage than we need, but for sure there is potential for even more in the future.

Comment #7

19 August 2013 at 19:17

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, plugin-2062367-5-PASS.patch, failed testing.

Comment #8

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 19 August 2013 at 19:19

Assigned:

Unassigned

» tim.plunkett

On it.

Comment #9

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 19 August 2013 at 19:38

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.24 KB
plugin-2062367-9.patch	9.73 KB

Forgot to clear out the originalOrder instance ID in removeInstanceID(), and FilterAPITest had the filter types in 1,0 order, but it should have been 0,1.

Comment #10

EclipseGc CreditAttribution: EclipseGc commented 19 August 2013 at 19:46

I sort of don't like that we have to do this, but the solution is pretty elegant. I'm generally ++ if it comes back green.

Eclipse

Comment #11

EclipseGc CreditAttribution: EclipseGc commented 19 August 2013 at 21:55

Status:

Needs review

» Reviewed & tested by the community

Looks kosher to me.

Eclipse

Comment #12

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 20 August 2013 at 10:44

Status:

Reviewed & tested by the community

» Needs review

If I understand this correctly this problem is not actually reproducable with DefaultPluginBag as that sorts by name already. ImageEffectBag sorts by weight and thus produces the situation described in the OP. Is that correct?

Also, with this patch if I use DefaultPluginBag and have already exported 'aaa', 'bbb' and 'ddd' and then add a 'ccc' plugin, then that is added to the end, correct? Now that's certainly not the end of the world, but I would find that quite unintuitive.

Can't we just add a sortExport() method that by default just calls sort() internally and let ImageEffectBag handle this by simply sorting by UUID or whatever instead of weight. Even though Eclipse is correct that this code is quite elegant in how it solves this problem in minimal LOC added, I don't think it is very elegant architecturally. Having to store something like $originalOrder feels like code smell. Thoughts?

Comment #13

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 20 August 2013 at 17:09

Status:

Needs review

» Reviewed & tested by the community

This is stored this way by design. The original ordering of the YAML file should never change. It is even more drastically illustrated with Views displays, since they are 90% of the export file.

If a contrib module wants to completely destroy any hopes for a sane diff when reordering their config, that can be overridden. I see no reason to complicate the PluginBag code with Yet Another Sort method in order to provide flexibility for a YAML file no one should be viewing outside of diffs.

Comment #14

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 21 August 2013 at 08:28

The original ordering of the YAML file should never change.

Well you get a nice diff as well even if additional items get added in between others. It's only when existing ones change order that the diff gets messed up. So as long as sortExport() is deterministic the diff is sane either way.

Also IMHO this would not actually increase complexity vs. the current patch, but hey...

in order to provide flexibility for a YAML file no one should be viewing outside of diffs

I don't care about flexibility, I care about consistency of my YAML files. And having them ordered 'aaa', 'ccc', 'bbb' just feels strange.

But, again, it's not the end of the world and I'll shut up now.

Comment #15

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 22 August 2013 at 03:15

I don't care about flexibility, I care about consistency of my YAML files.

That is the opposite of the design goal here. If you care about the output of CMI, don't use dynamic CMI objects (ConfigEntity).

If you read the OP, preventing diffs like that is essential. Reordering them to be alphabetical helps non one.

Comment #16

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 22 August 2013 at 07:26

I said I was gonna shut up, but:

If you care about the output of CMI, don't use dynamic CMI objects (ConfigEntity).

Sorry, but you literally can not be serious saying that. If we don't care about the output of our CMI files (yes, including "dynamic" ones) then we might as well have not started CMI in the first place...

Comment #17

EclipseGc CreditAttribution: EclipseGc commented 22 August 2013 at 13:29

All this patch is doing is preventing interdiffs from exploding in size due to unnecessary programatic reordering. Re-affirming the RTBC.

Eclipse

Comment #18

xjm

she/her

English

CreditAttribution: xjm commented 28 August 2013 at 22:51

Site builders and are not going to sit around reading whole YAML files. They are going to look at diffs either through the UI or through VCS. This patch reduces needless, confusing noise in said diffs, and therefore will result in better user and developer experience. So, I also think this issue is moving us in the correct direction.