Problem/Motivation

Not sure if I am alone, but when I try to add an effect to an existing image style, the form returns to the list of styles instead of the effect configuration form.

Tested on simplytest.me

Steps:
1. Edit the 'Thumbnail' style
2. Select a 'Convert' effect
3. Click the 'Add' button close to the effect

I'd expect at this stage the image effect configuration form, but in fact the list of image styles appears instead.

As a workaround, I found that if you edit an existing effect and click on 'Update effect', then you can add further effects.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 2940165-11.patch2.12 KBmondrake
#11 interdiff_9-11.txt632 bytesmondrake
#9 interdiff_3-9.txt787 bytesmondrake
#9 2940165-9.patch2.1 KBmondrake
#3 destination-image-style-edit-2940165-3.patch1.34 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: Cannot add effects to image style » [regression] Cannot add effects to image style via the UI
Version: 8.6.x-dev » 8.5.0-alpha1
Priority: Major » Critical

Checked again on simplytest.me -

with 8.4.4 - works as expected
with 8.5.0-alpha1 - fails

So it seems some regression slipped in the 8.5.x codebase. Bumping to critical to get exposure - there's a workaround (see IS) but it's bad UX.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Version: 8.5.0-alpha1 » 8.5.x-dev
Status: Active » Needs review
FileSize
destination-image-style-edit-2940165-3.patch1.34 KB

Nice find.

I guess I broke this :)

The reason is the destination argument in the URL, which overrides the destination set on the URL. This was introduced by #2767857: Add destination to edit, delete, enable, disable links in entity list builders, which adds them automatically to all edit and delete operations. This can cause issues like this if the page then expects to do another redirect.

So we have two options to fix it, Either remove the destination for image styles in \Drupal\image\ImageStyleListBuilder::getDefaultOperations() or unset the destination query argument in that specific submit callback.

Here's a failing test.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Note: A regression is not automatically critical but I think this is severe enough that it makes sense that this is critical, this is a pretty common UI operation and it will not be obvious at all that it didn't work .

Status: Needs review » Needs work

The last submitted patch, 3: destination-image-style-edit-2940165-3.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

So we have two options to fix it, Either remove the destination for image styles in \Drupal\image\ImageStyleListBuilder::getDefaultOperations() or unset the destination query argument in that specific submit callback.

But then what we have here is a BC break, no? I am facing a similar behavior in the Pagerer module - where we have an edit form for a config entity, and a form that is invoked from the edit form to configure a part of the config object. Same as here for image styles where the image style is the config entity and the effects are sub-elements of the configuration.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

In a way yes, but IMHO it is a change that's worth it. For content entities, views operations already had an option to force the destination link so those already had to be able to handle it. The destination parameter and how it behaves is a general feature, anyone could for some reason build a link to your form with that.

Many forms these days use ajax/dialogs for use cases like that, core has been working for a while now to use it e.g. in the workflows UI, which has similar workflows (and is not affected by this, just tested). Then it's much less of a problem.

I see your argument, there are quite often side effects when we add new features/improvements and it can be annoying when updating, seen this many times myself, but hopefully the 8.x -> 9.x update will be far less complicated than 7.x -> 8.x to make up for that ;)

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Version: 8.5.x-dev » 8.6.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review
FileSize
2940165-9.patch2.1 KB
interdiff_3-9.txt787 bytes

Trying removing the destination for image styles in \Drupal\image\ImageStyleListBuilder::getDefaultOperations().

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer and at PreviousNext commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
@@ -422,9 +422,20 @@ public function testEditEffect() {
+      if (((string) $row->td[0]) == 'Test style scale edit scale') {

nit === here, but its not worth knocking this back on that alone

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
interdiff_9-11.txt632 bytes
2940165-11.patch2.12 KB

Fixed the nit in #10.

  • catch committed 8361c94 on 8.6.x 
    Issue #2940165 by mondrake, Berdir: [regression] Cannot add effects to...

  • catch committed e0577f7 on 8.5.x 
    Issue #2940165 by mondrake, Berdir: [regression] Cannot add effects to...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

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