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.

Files: 
CommentFileSizeAuthor
#23 2052787-image-derivative-23.patch3.71 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,955 pass(es). View
#23 interdiff.txt647 bytesandypost
#20 interdiff.txt1016 bytestim.plunkett
#20 image-2052787-20-FAIL.patch1.03 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,980 pass(es), 1 fail(s), and 0 exception(s). View
#20 image-2052787-20-PASS.patch3.36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,005 pass(es). View

Comments

alexpott’s picture

Title:Image style effect ording» Image style effect ordering exhibits some odd behaviour

Fixing title

claudiu.cristea’s picture

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

andypost’s picture

Status:Active» Needs review
FileSize
3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 57,633 pass(es). View

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

Priority:Normal» Major
Issue tags:+Needs tests
FileSize
3.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,290 pass(es), 2 fail(s), and 0 exception(s). View

Move sorting to constructor

Status:Needs review» Needs work

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

andypost’s picture

Status:Needs work» Needs review
FileSize
3.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,545 pass(es). View
665 bytes

constructor should sort $configuration

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

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

tim.plunkett’s picture

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

Assigned:tim.plunkett» andypost
Status:Postponed» Needs review
FileSize
4.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-2052787-10.patch. Unable to apply patch. See the log in the details link for more information. View

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

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

Status:Needs work» Needs review
FileSize
2.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-2052787-13.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled. Still needs tests.

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

andypost’s picture

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

Issue summary:View changes
Status:Needs work» Needs review
FileSize
2.64 KB
FAILED: [[SimpleTest]]: [MySQL] 59,113 pass(es), 1 fail(s), and 0 exception(s). View

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

Status:Needs work» Needs review
FileSize
3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,005 pass(es). View
1.03 KB
FAILED: [[SimpleTest]]: [MySQL] 58,980 pass(es), 1 fail(s), and 0 exception(s). View
1016 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

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

FileSize
647 bytes
3.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,955 pass(es). View

And another place, RTBC++

catch’s picture

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.