Problem/Motivation

Image style configuration can have sorting issues that result in configuration diffs that have unnecessary changes.

Proposed resolution

Sort effects using the new schema sorting.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

Image style config sorted correctly - will need a post update to resave image config to fix this.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
426 bytes

Let's see if anything breaks.

alexpott’s picture

Here's a post update - now we need tests

The last submitted patch, 2: 2866421-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2866421-3.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
6.37 KB

Here's some tests. Found a related bug with adding effect not forcing a re-order. Simply nulling the plugin collection on save does the trick. This forces it to be reinitialised with the correct config.

alexpott’s picture

Maybe the plugin collection clearing should be a feature of config entity base... it's a bit tricky. \Drupal\Core\Config\Entity\ConfigEntityBase::set has code to ensure config sets are mapped to the plugin but adding a new effect can affect the order. Maybe the fix should be in \Drupal\image\ImageEffectPluginCollection().

alexpott’s picture

Let's keep this simple and just sort when we add. I'm not sure about other configuration because how they work can be different.

Patch also adds an update path test.

The last submitted patch, 6: 2866421-6.patch, failed testing.

Munavijayalakshmi’s picture

+++ b/core/modules/image/src/Tests/Update/ImageUpdateEffectOrderTest.php
@@ -0,0 +1,58 @@
+    // Plugin collection will bne osrted correctly because it is sorted by

Minor typo errors, fixed it, attached new patch.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

Status: Needs review » Needs work

I have a few nitpicks - but this looks solid overall.

  1. +++ b/core/modules/image/tests/src/Kernel/ImageStyleEffectOrderTest.php
    @@ -0,0 +1,124 @@
    + * Tests the integration of ImageStyle with the core.
    

    /s/with the core/with core/?

  2. +++ b/core/modules/image/tests/src/Kernel/ImageStyleEffectOrderTest.php
    @@ -0,0 +1,124 @@
    +  public function testEffectOrder() {// Create two image styles.
    

    The comment here should be on the next line.

rajibmp’s picture

Status: Needs work » Needs review
FileSize
9.39 KB
+++ b/core/modules/image/tests/src/Kernel/ImageStyleEffectOrderTest.php
@@ -0,0 +1,124 @@
+ * Tests the integration of ImageStyle with the core.

I think this comment is inline with other comments like: http://cgit.drupalcode.org/drupal/tree/core/modules/image/tests/src/Kern...

+++ b/core/modules/image/tests/src/Kernel/ImageStyleEffectOrderTest.php
@@ -0,0 +1,124 @@
+  public function testEffectOrder() {// Create two image styles.

This comment is on new line now.

Status: Needs review » Needs work

The last submitted patch, 13: 2866421-13.patch, failed testing. View results

rajibmp’s picture

Status: Needs work » Needs review

Setting to needs review so that the testbot can pick this patch up.

The latest tests were passing but doesn't show up here but visible in: https://www.drupal.org/pift-ci-job/803892

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.