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.pngI 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.pngWhich 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
StatusFileSize
new3.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
StatusFileSize
new3.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
StatusFileSize
new3.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,545 pass(es).
[ View ]
new665 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
StatusFileSize
new4.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
StatusFileSize
new2.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
StatusFileSize
new2.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
StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,005 pass(es).
[ View ]
new1.03 KB
FAILED: [[SimpleTest]]: [MySQL] 58,980 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1016 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

StatusFileSize
new647 bytes
new3.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.