I couldn't find this in "image" defects against D8. The issue has been verified as new to D8, I have tested on D7.
Steps to reproduce:
- Create a new image style
- Add a scale and crop effect to it (or whatever)
- Make sure it's saved ...
- Edit the effect you just added
- Change one or both of the dimensions, and save again
- Note the image style now has 2 effects, but only one was intentionally added
Only one effect was added to this style at 10x10 and edited to then be 100x100: https://skitch.com/e-alanevans/8qjdr/edit-test-multiple-style-testd85.lo...
The problem appears to be that D7 referred to effects by integer Ids, D8 refers to them by a function-based description, so essentially all the attributes are included, producing mostly a unique name per effect. If you don't actually change anything, then the effects do not multiply.
Comment | File | Size | Author |
---|---|---|---|
#41 | i1508872-image-effect-duplicate-edit.patch | 9.94 KB | attiks |
#41 | interdiff.txt | 977 bytes | attiks |
#39 | interdiff.txt | 954 bytes | attiks |
#39 | i1508872-39.patch | 9.46 KB | attiks |
#36 | fake_interdiff.txt | 6.19 KB | attiks |
Comments
Comment #1
Alan Evans CreditAttribution: Alan Evans commentedProbably a more apt priority.
Comment #2
webchickI'm guessing this has something to do with the CMI conversion. I can't think of anything else that'd be different between D7 and D8.
Comment #3
marcingy CreditAttribution: marcingy commentedInitial patch still need to work on tests but basically we never pass in ieid to image_effect_save, I wonder if the approach in this patch is the best approach as it is going to end up potentially with strangely named configs and also the possibility of over writing. We might want to check the existing config for the key that was provided, delete that configuration key and then populate based on the new item. This approach will build on this patch anyway as we need to pass in ieid as a starting point.
Comment #4
marcingy CreditAttribution: marcingy commentedComment #5
gddThis is also of course an artifact of the fact that we can't use IDs for keys in the XML files (see #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML).
Comment #6
Alan Evans CreditAttribution: Alan Evans commentedThanks for the patch, Marc. This works well and I don't see any problems with the approach.
I thought this would make a nice self-contained (pseudo-)test-driven issue (pseudo because the patch is already done, but I hadn't applied it before writing the failing test). So here's the newly-created failing test: https://skitch.com/e-alanevans/8qaad/test-result-testd85.localhost ...
The pass after applying Marc's patch: https://skitch.com/e-alanevans/8qaas/test-result-testd85.localhost ... https://skitch.com/e-alanevans/8qaa3/test-result-testd85.localhost
And full patch is attached.
There are a number of things that are hard-coded in the test which could be randomized a bit more. Any thoughts whether that's worth doing? Given that we're creating a new effect in the test, I wouldn't have thought randomizing the test values would add much value.
Comment #7
Alan Evans CreditAttribution: Alan Evans commentedComment #8
swentel CreditAttribution: swentel commentedPatch works fine in that sense that the effect is not duplicated anymore, however, there's another 'funny' thing that's happening now:
- add a scale effect 12x19: the id should be this: image_scale_12_19_0
- edit that effect to 24x19: the id should become image_scale_24_19_0, but it stays the same - you can easily see it hovering the edit link or looking into the configuration file. The data is fine though.
- add a new scale effect 12x19 and you lose the previous configuration.
Comment #9
sunThanks for reminding about that.
Yes, we probably want to replace the effect keys with integer IDs. Which means that we need a 'weight' property on each effect and manually sort them after retrieval. I'm not sure whether this has any other implications right now (thinking of cases where the current keys might be actively used as CSS classes or whatever), but I hope not.
Comment #10
gddWe definitely need to change the effect keys with integer IDs, the question is does it make sense to approach that in this patch or to open another issue?
The current keys were created by me in the initial CMI patch and never existed before, so they have no other utility in any other part of Drupal. Also, the effects already have weights that ARE used throughout core, so that isn't a concern either. So all that needs to happen is
- The effect keys need to change to integers
- The code to refer to the effects needs to be modified to acknowledge this
- The code used to generate machine names needs to be removed.
I don't really have a preference about where this change happens.
Comment #11
disasm CreditAttribution: disasm commentedreroll of #7. Placed testEditEffect function that was in image.test into ImageAdminStylesTest class.
Comment #12
disasm CreditAttribution: disasm commentedoops, uploaded wrong file.
Comment #13
disasm CreditAttribution: disasm commentedattached is a patch and a test only patch that adds the test described in #8.
Comment #14
disasm CreditAttribution: disasm commentedrequested interdiff attached.
Comment #16
attiks CreditAttribution: attiks commentedI added a check to see if the ieid was already used, if so a new random one is created (using user_password()).
Comment #17
attiks CreditAttribution: attiks commented#9: Regarding the weight, don't we need it anyway? For the moment it's impossible to re-order the effects.
Comment #18
attiks CreditAttribution: attiks commentedwithout the watchdog
Comment #19
xjmFew cleanups to work on during core mentoring:
I believe this should say "existing" rather than "exiting". Also, it would be better to state what happens rather than asking this question, i.e., "If we are editing an existing style, do blah."
Let's put a period on the end of this comment. Also, it would be better to say "the image effect ID" in text.
This comment is a little confusing and over 80 characters.
Rather than having this information on one terse line, let's add each part of this comment to the line above the relevant method calls.
As Cottser has pointed out, improving the test documentation would be a good novice/mentoring task. Please include an interdiff with your updated patch. Thanks!
Comment #20
attiks CreditAttribution: attiks commentedWe also need to solve the weight problem, so users can re-order the image effects.
Comment #21
catchComment #22
attiks CreditAttribution: attiks commentedAddressed all points in #20
Are we going to solve the weight problem in a follow-up?
Comment #23
Jelle_SAs for the weight problem:
The problem is that in D7, image styles could be provided by modules (in code) and thus not in the database. If they weren't in the database, they could not be edited. If they couldn't be edited, that meant their weight could not be changed either (
#access
was set toFALSE
on the weight dropdown, and when#access
wasFALSE
, thedraggable
class wasn't added to this row). Since the config system was introduced, this reasoning became obsolete.There are two ways of fixing it:
#access
toTRUE
inimage_style_form
theme_image_style_effects
to always add thedraggable
class, regardless of#access
In the attached patch I chose the last solution, as
image_style_form
is the only place in Drupal core where the theme function is called (I checked), so this change won't affect anything else.It's a simple fix, and it removes redundant code (image styles are managed by the config system, and thus always editable, doesn't matter if they were provided by a module or custom made by a user). The first fix would be just as simple, but I don't see any reason to keep that code around in the theme function.
Comment #25
Jelle_SGot a file permissions change (my settings.php) in that patch somehow, here's the correct one:
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you! I can confirm the patch fixes the problem. Also the tests look pretty solid.
Don't we have a better way of adding some random stuff to it?
After that, I believe this is good to go. Edit: Maybe except for my objections against t() in the patch, where we add new translatables by using concrete values for the sizes.
Comment #27
attiks CreditAttribution: attiks commentedI chose user_password() because AFAIK there's nothing better, but I admit it looks strange.
About t(): you mean:
should become
Comment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commented(1) Maybe hash('sha256', drupal_random_bytes(64)) or just increasing a counter until unique? Are there limits to the length in characters? Is there a difference between appending a random token and just using the token directly?
(2) Mhh ... upon looking I see that even some translatable like that isn't used in core right now. So probably just without t().
Comment #29
attiks CreditAttribution: attiks commented1a) is think both of them can work
1b) I don't think the length is limited, but not sure
1c) I think we can use a complete random string, the ieid is just a key, maybe use http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Uuid%...
2) so needs a reroll
Comment #30
sunI'm sorry to put it into these words, but we need to stop the duct-taping on image effects. Let me explain the background and overall problem space:
- Image styles contain image effects.
- Image styles are dynamic configuration. Image effects are dynamic configuration. Both can be edited from the UI. Normally, this wouldn't be special.
- However, image effects are special. Image module provides an own, individual, and separated API for each one of them. And even an entirely separated UI.
- It is the separate API that causes the problem in the first place. This API allows developers to manage a certain sub-set (nested key) of a configuration object.
- That's a bit insane, because this sub-set requires specially crafted additional information to retain the information of where and how it is located in the "outer" configuration object/set. If that information is lost, then the API no longer knows from where the effect config originated from, and where to save it to.
- The proper fix we have to do for D8 is to remove that separate image effect API entirely. The only data object that is supposed to be passed around is an image $style. Everything else is architectural garbage.
However, that proper fix is different from this regression, so let's create a follow-up issue for that.
In any case, we need a more radical change to fix this bug and regression in a proper way. I'm attaching a proposal for how to fix it. I did not touch the tests.
Also cleaning up the issue title; the "regression" issue tag and issue priority is sufficient.
Comment #31
andyposta style object needs to maintain a weighted list of effect implementation classes anyway
each effect class could have a own config-container then could be attached to style
style: thumb
effects:
[weight]:[class-name]
effect-config:
[class_name+weight]:[config]
Comment #32
gddI do like leveraging UUIDs more than user_password() but it does make URLs tied to these effects a lot uglier. I don't know if I care (example admin/config/media/image-styles/edit/test/effects/4bd1c937-f530-456f-a218-241d7bfd40b8) but its something to think about.
We will have to modify the default YAML files to use uuid-based effects (although that does make them no longer truly universally unique then but whatever.)
Comment #33
xjmMeanwhile, some minor docs fixes we're working on during office hours.
This isn't a complete sentence -- How about "if there is no configuration for this image effect"? The comment will then need to be rewrapped as well.
"Generate a unique image effect ID..."
I'd say "Verifies that editing an image effect does not cause it to be duplicated."
I think this should say "Check that the previous effect is replaced"?
How about, "Edit the scale effect that was just added"?
"...make sure both exist."
Comment #34
gddNote that this and any followups that rewrite the effect/style APIs are holding up another critical - #1464554: Upgrade path for image style configuration since that upgrade path can't be properly written until these APIs settle down.
Comment #35
attiks CreditAttribution: attiks commentednew patch for comments in #33
Comment #36
attiks CreditAttribution: attiks commentedinterdiff, but i diffed both patches ;-)
Comment #37
gddWe talked a bit about this today, and nobody has any issues with the UUID implementation. The URL is just for admins so I don't think its a problem. The above patch still needs some testing and cleanup, I think the tests need to updated right? and the default YAML files need updating
Comment #38
attiks CreditAttribution: attiks commented@heyrocker I'll need some more info, AFAIK the test are good
You want me to add UUID into the config files?
Comment #39
attiks CreditAttribution: attiks commentedNew patch for the uuid, let me know if you need more tests.
Comment #40
sunThanks! Almost done :)
Unless I'm mistaken, the UUID needs to be repeated for the ieid property for each effect.
I'd suggest to just simply do that for now. We can figure out whether we want/need to keep the ieid property in the long run, in a separate issue.
Comment #41
attiks CreditAttribution: attiks commentedNew patch
Comment #42
disasm CreditAttribution: disasm commentedI looked over the patch, and everything looks good. I don't see any glaring syntax/coding style issues. Did some manual testing and the issue is fixed. With sun's review above, I would say this is ready for RTBC!
Comment #43
webchickOh, rock! So happy to see this fixed. This is a major WTF when you're testing out CMI.
I don't suppose there's a way of /just/ making ieid a messy UUID, and not futzing with the human-friendly machine-readable name (and URL)? I did not immediately see where this was discussed between #30 and #39.
Comment #44
attiks CreditAttribution: attiks commented@webchick I discussed this briefly with @heyrocker, but in short nobody really mind the 'ugly' url, because it's never used directly.
Comment #45
webchickOk, talked my concerns over with heyrocker in IRC.
I was getting confused about effects vs. styles. We don't currently have human-specified names for the intersection between image effects + image styles, so this doesn't represent a regression. The URL for editing those is currently just like admin/config/media/image-styles/edit/medium/effects/1; "1" (which doesn't mean anything) would just become a UUID here. I was concerned about this spilling into other things people are trying to turn into Configurables (like content types), but that's a different situation because there we have machine names.
Therefore, this looks good to me. Committed and pushed to 8.x. Yay!