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:

  1. Create a new image style
  2. Add a scale and crop effect to it (or whatever)
  3. Make sure it's saved ...
  4. Edit the effect you just added
  5. Change one or both of the dimensions, and save again
  6. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan Evans’s picture

Priority: Major » Normal

Probably a more apt priority.

webchick’s picture

Issue tags: +Configuration system

I'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.

marcingy’s picture

Issue tags: +Needs tests
FileSize
864 bytes

Initial 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.

marcingy’s picture

Status: Active » Needs review
gdd’s picture

This 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).

Alan Evans’s picture

Thanks 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.

Alan Evans’s picture

swentel’s picture

Status: Needs review » Needs work

Patch 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.

sun’s picture

Thanks 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.

gdd’s picture

We 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.

disasm’s picture

reroll of #7. Placed testEditEffect function that was in image.test into ImageAdminStylesTest class.

disasm’s picture

oops, uploaded wrong file.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
2.35 KB

attached is a patch and a test only patch that adds the test described in #8.

disasm’s picture

FileSize
1.43 KB

requested interdiff attached.

Status: Needs review » Needs work

The last submitted patch, drupal-1145080-image_style_dupe-13.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
4.4 KB

I added a check to see if the ieid was already used, if so a new random one is created (using user_password()).

attiks’s picture

#9: Regarding the weight, don't we need it anyway? For the moment it's impossible to re-order the effects.

attiks’s picture

FileSize
3.99 KB

without the watchdog

xjm’s picture

Issue tags: +Novice

Few cleanups to work on during core mentoring:

  1. +++ b/core/modules/image/image.admin.incundefined
    @@ -376,6 +376,12 @@ function image_effect_form_submit($form, &$form_state) {
    +  // Are we editing an exiting style?
    

    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."

  2. +++ b/core/modules/image/image.moduleundefined
    @@ -1124,7 +1124,13 @@ function image_effect_save($style_name, $effect) {
    +    // Make sure the ieid is unique
    

    Let's put a period on the end of this comment. Also, it would be better to say "the image effect ID" in text.

  3. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
    @@ -258,4 +258,34 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
    +    // Check the previous effect is not also retained on this style after editing.
    

    This comment is a little confusing and over 80 characters.

  4. +++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
    @@ -258,4 +258,34 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
    +    // Add scale effect, edit effect, add new effect that matches original scale
    +    // effect, check that both effects still exist
    

    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.

  5. Let's add some whitespace and inline comments to the test method (such that each step forms a scannable "paragraph").
  6. Finally, let's upload the test in a patch by itself, followed by the combined patch in the same comment, to expose the test failure without the fix in the current patch.

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!

attiks’s picture

We also need to solve the weight problem, so users can re-order the image effects.

catch’s picture

Priority: Normal » Critical
Issue tags: +Regression
attiks’s picture

Assigned: Unassigned » attiks
FileSize
4.08 KB
2.37 KB

Addressed all points in #20

Are we going to solve the weight problem in a follow-up?

Jelle_S’s picture

FileSize
4.67 KB

As 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 to FALSE on the weight dropdown, and when #access was FALSE, the draggable class wasn't added to this row). Since the config system was introduced, this reasoning became obsolete.

There are two ways of fixing it:

  1. Always set #access to TRUE in image_style_form
  2. Change theme_image_style_effects to always add the draggable 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.

Status: Needs review » Needs work

The last submitted patch, i1508872-23.patch, failed testing.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

Got a file permissions change (my settings.php) in that patch somehow, here's the correct one:

Niklas Fiekas’s picture

Issue tags: -Needs tests

Thank you! I can confirm the patch fixes the problem. Also the tests look pretty solid.

+++ b/core/modules/image/image.moduleundefined
@@ -1124,8 +1124,15 @@ function image_effect_save($style_name, $effect) {
+      $new_ieid .= user_password();

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.

attiks’s picture

I chose user_password() because AFAIK there's nothing better, but I admit it looks strange.

About t(): you mean:

$this->assertText(t('Scale 12x19'));

should become

$this->assertText(t('Scale !dim', array('!dim' => '12x19')));
Niklas Fiekas’s picture

(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().

attiks’s picture

Status: Needs review » Needs work

1a) 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

sun’s picture

Title: Regression (from D7): Image effects duplicate on edit » Image effects duplicate on edit
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
8.14 KB

I'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.

andypost’s picture

a 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]

gdd’s picture

Status: Needs review » Needs work

I 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.)

xjm’s picture

Meanwhile, some minor docs fixes we're working on during office hours.

+++ b/core/modules/image/image.admin.incundefined
@@ -326,7 +326,14 @@ function image_style_delete_form_submit($form, &$form_state) {
+  // If no configuration for this image effect, return to the image style page.

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.

+++ b/core/modules/image/image.moduleundefined
@@ -1103,33 +1104,30 @@ function image_effect_load($ieid, $style_name) {
+  // Generate a unique ieid for a new effect.

"Generate a unique image effect ID..."

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -258,4 +258,41 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
+   * Verify that editing an image effect does not cause it to duplicate.

I'd say "Verifies that editing an image effect does not cause it to be duplicated."

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -258,4 +258,41 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
+    // Check the previous effect is replaced.

I think this should say "Check that the previous effect is replaced"?

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -258,4 +258,41 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
+    // Edit the last added scale effect.

How about, "Edit the scale effect that was just added"?

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -258,4 +258,41 @@ class ImageAdminStylesTest extends ImageFieldTestBase {
+    // Add another scale effect and make sure both exists.

"...make sure both exist."

gdd’s picture

Note 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.

attiks’s picture

Assigned: attiks » Unassigned
Status: Needs work » Needs review
FileSize
8.18 KB

new patch for comments in #33

attiks’s picture

FileSize
6.19 KB

interdiff, but i diffed both patches ;-)

gdd’s picture

We 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

attiks’s picture

@heyrocker I'll need some more info, AFAIK the test are good
You want me to add UUID into the config files?

attiks’s picture

FileSize
9.46 KB
954 bytes

New patch for the uuid, let me know if you need more tests.

sun’s picture

Status: Needs review » Needs work

Thanks! Almost done :)

+++ b/core/modules/image/config/image.style.large.yml
@@ -1,6 +1,6 @@
-    image_scale_480_480_1:
+    ddd73aa7-4bd6-4c85-b600-bdf2b1628d1d:

+++ b/core/modules/image/config/image.style.medium.yml
@@ -1,6 +1,6 @@
-    image_scale_220_220_1:
+    bddf0d06-42f9-4c75-a700-a33cafa25ea0:

+++ b/core/modules/image/config/image.style.thumbnail.yml
@@ -1,6 +1,6 @@
-    image_scale_100_100_1:
+    1cfec298-8620-4749-b100-ccb6c4500779:

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
977 bytes
9.94 KB

New patch

disasm’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

webchick’s picture

Oh, 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.

attiks’s picture

@webchick I discussed this briefly with @heyrocker, but in short nobody really mind the 'ugly' url, because it's never used directly.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 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!

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