Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hmmm... we seems to have broken image effect ordering...
Steps to reproduce:
- Create a new image style called "test"
- Add an effect to crop 300 x 200 and anchor it in the top left corner
- Add an effect to rotate 90 degress
You see...
Then...
- Drag the rotate effect above the crop effect
- Update the style
You see...
I think this is wrong because rotation is configured take place before the crop.
Then...
- Drag the rotate effect below the crop effect
- Update the style
You see...
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2052787-image-derivative-23.patch | 3.71 KB | andypost |
#23 | interdiff.txt | 647 bytes | andypost |
#20 | interdiff.txt | 1016 bytes | tim.plunkett |
#20 | image-2052787-20-FAIL.patch | 1.03 KB | tim.plunkett |
#20 | image-2052787-20-PASS.patch | 3.36 KB | tim.plunkett |
Comments
Comment #1
alexpottFixing title
Comment #2
claudiu.cristeaI would investigate this but I'm in holiday right now.
Comment #3
andypostThat's because effects applied in different order.
Suppose using sort() method breaks encapsulation and misleading so let's hide it
Comment #4
andypostproper tags
Related #2033383: Provide a default plugin bag
Comment #5
andypostMove sorting to constructor
Comment #7
andypostconstructor should sort $configuration
Comment #8
tim.plunkettI have to test a couple more things first. Thanks @andypost!
Comment #9
tim.plunkettMy 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.
Comment #10
tim.plunkettActually 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.
Comment #11
andypost#10: image-2052787-10.patch queued for re-testing.
Comment #13
tim.plunkettRerolled. Still needs tests.
Comment #15
andypost13: image-2052787-13.patch queued for re-testing.
Comment #17
slashrsm CreditAttribution: slashrsm commentedReroll. Let's see if passes and fix tests then.
Comment #20
tim.plunkettWe don't actually need tests, we just need to remove the ->sort() calls from the tests, so that they test the right thing.
Comment #22
slashrsm CreditAttribution: slashrsm commentedLooks good to me. PASS patch passed and FAIL patch failed as expected. Code also looks good.
Comment #23
andypostAnd another place, RTBC++
Comment #24
catchCommitted/pushed to 8.x, thanks!