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

Files: 
CommentFileSizeAuthor
#9 plugin-2062367-9.patch9.73 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,207 pass(es). View
#9 interdiff.txt1.24 KBtim.plunkett
#5 plugin-2062367-5-FAIL.patch7.25 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,931 pass(es), 1 fail(s), and 0 exception(s). View
#5 plugin-2062367-5-PASS.patch8.76 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,211 pass(es), 6 fail(s), and 0 exception(s). View
#5 interdiff.txt2.86 KBtim.plunkett
#3 plugin-2062367-3.patch7.9 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 55,681 pass(es), 138 fail(s), and 9 exception(s). View
#3 interdiff.txt807 bytesdawehner
#1 plugin-2062367-1.patch1.77 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,789 pass(es), 138 fail(s), and 9 exception(s). View

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.77 KB
FAILED: [[SimpleTest]]: [MySQL] 55,789 pass(es), 138 fail(s), and 9 exception(s). View

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.

tim.plunkett’s picture

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

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

  2. @@ -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)

  3. @@ -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.

dawehner’s picture

FileSize
807 bytes
7.9 KB
FAILED: [[SimpleTest]]: [MySQL] 55,681 pass(es), 138 fail(s), and 9 exception(s). View

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.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.86 KB
8.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,211 pass(es), 6 fail(s), and 0 exception(s). View
7.25 KB
FAILED: [[SimpleTest]]: [MySQL] 57,931 pass(es), 1 fail(s), and 0 exception(s). View

This works for me.

dawehner’s picture

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.

Status: Reviewed & tested by the community » Needs work

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

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

On it.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
9.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,207 pass(es). View

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.

EclipseGc’s picture

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

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Looks kosher to me.

Eclipse

tstoeckler’s picture

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?

tim.plunkett’s picture

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.

tstoeckler’s picture

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.

tim.plunkett’s picture

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.

tstoeckler’s picture

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

EclipseGc’s picture

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

Eclipse

xjm’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 358de91 and pushed to 8.x. Thanks!

This is very similar to #1785560: Remove the sorting of configuration keys as sorting just does not help users wrt to CMI files.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

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