Hmmm... we seems to have broken image effect ordering...

Steps to reproduce:

  1. Create a new image style called "test"
  2. Add an effect to crop 300 x 200 and anchor it in the top left corner
  3. Add an effect to rotate 90 degress

You see...
Screenshot_29_07_2013_14_27.png
Then...

  1. Drag the rotate effect above the crop effect
  2. Update the style

You see...
Screenshot_29_07_2013_14_27.png
I think this is wrong because rotation is configured take place before the crop.

Then...

  1. Drag the rotate effect below the crop effect
  2. Update the style

You see...
Screenshot_29_07_2013_14_28.png
Which I think is now wrong because rotation should take place after the crop.

From now on it'll alternate if you swap the ordering.

CommentFileSizeAuthor
#23 2052787-image-derivative-23.patch3.71 KBandypost
#23 interdiff.txt647 bytesandypost
#20 interdiff.txt1016 bytestim.plunkett
#20 image-2052787-20-FAIL.patch1.03 KBtim.plunkett
#20 image-2052787-20-PASS.patch3.36 KBtim.plunkett
#17 2052787_17.patch2.64 KBslashrsm
#13 image-2052787-13.patch2.8 KBtim.plunkett
#10 image-2052787-10.patch4.31 KBtim.plunkett
#7 interdiff.txt665 bytesandypost
#7 2052787-image-derivative-7.patch3.79 KBandypost
#5 2052787-image-derivative-5.patch3.79 KBandypost
#3 2052787-image-derivative-3.patch3.04 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Image style effect ording » Image style effect ordering exhibits some odd behaviour

Fixing title

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea commented

I would investigate this but I'm in holiday right now.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
2052787-image-derivative-3.patch3.04 KB

That's because effects applied in different order.

Suppose using sort() method breaks encapsulation and misleading so let's hide it

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

proper tags

Related #2033383: Provide a default plugin bag

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Priority: Normal » Major
Issue tags: +Needs tests
FileSize
2052787-image-derivative-5.patch3.79 KB

Move sorting to constructor

Status: Needs review » Needs work

The last submitted patch, 2052787-image-derivative-5.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2052787-image-derivative-7.patch3.79 KB
interdiff.txt665 bytes

constructor should sort $configuration

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett

I have to test a couple more things first. Thanks @andypost!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Postponed

My concerns will be handled by #2062367: Prevent PluginBags from reordering the export based on their sort, but I worry this will conflict in intention somewhat, I'd rather have that fix and test coverage in first.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » andypost
Status: Postponed » Needs review
FileSize
image-2052787-10.patch4.31 KB

Actually it doesn't need to be postponed, this should be fine.

Here's a different approach.
No interdiff because the old one didn't apply.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: -Needs tests

#10: image-2052787-10.patch queued for re-testing.

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

The last submitted patch, image-2052787-10.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
image-2052787-13.patch2.8 KB

Rerolled. Still needs tests.

The last submitted patch, image-2052787-13.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review

13: image-2052787-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: image-2052787-13.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2052787_17.patch2.64 KB

Reroll. Let's see if passes and fix tests then.

Status: Needs review » Needs work

The last submitted patch, 17: 2052787_17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2052787_17.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
image-2052787-20-PASS.patch3.36 KB
image-2052787-20-FAIL.patch1.03 KB
interdiff.txt1016 bytes

We don't actually need tests, we just need to remove the ->sort() calls from the tests, so that they test the right thing.

The last submitted patch, 20: image-2052787-20-FAIL.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Assigned: andypost » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good to me. PASS patch passed and FAIL patch failed as expected. Code also looks good.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
interdiff.txt647 bytes
2052787-image-derivative-23.patch3.71 KB

And another place, RTBC++

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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