Add a "changes not applied until saved" warning when changing widget/formatter settings

This appears to need an issue summary update.

This was a spinoff task so what is being addressed here?
What is the proposed solution?
Remaining tasks?
Etc.

Thanks.

Running into some inconsistency between the tests an local that I haven't fully figured out yet. In some cases changing formatter settings and clicking the "update" button saves those changes, in other cases it doesn't.

Scenario: Article content type, update the Trimmed limit of the Body field with the summary or trimmed formatter in the default view mode

Formatter setting changes save after clicking Update, no need to save the manage display form as well

• Umami install profile

Formatter setting changes do not save after clicking Update. The manage display form must be saved for the changes to stick

• Automated tests
• Standard Profile
• A body field in a new content type created in Umami profile

Since this worked in Umami and not in Standard, I tried starting with a Standard profile install and enabling modules one by one until it matched Umami, and Standard continued to not save the settings while Umami did.

Because a Body field in a new content type in Umami doesn't save the settings, while Article does, I compared the field config and there's no differences aside from the names.

There's probably other config in Umami that could be impacting this. Trying to narrow down what that could be.

Talked to @effulgentsia about this and it sounds like the not-Umami (the majority of the scenarios) is the expected behavior, so that is a step towards understanding this. It would be good to understand why, though, as there may be a broader need to distinguish saving-changes from non-saving ones so the warning only appears for the latter.

The inconsistency between Standard and Umami install profiles is created by Layout Builder module. Specifically, Layout Builder overrides enabled in Umami is causing this.

This is because we have some code in \Drupal\field_ui\Form\EntityDisplayFormBase::buildFieldRow to ensure that any fields that have no applicable widgets, are disabled. Something about this logic leads into the entity display being re-saved every time the page is loaded. 🤯

No functional test coverage was written when the following check was introduced in #2552799: FieldType with no available widget causes Fatal error → , so we'll need to write some:

    // Disable fields without any applicable plugins.
    if (empty($this->getApplicablePluginOptions($field_definition))) {
      $this->entity->removeComponent($field_name)->save();
      $display_options = $this->entity->getComponent($field_name);
    }

I believe the fix is a straightforward one, but I'm going to work on tests first.

Code is NR, still needs IS update

I think I pushed too quickly, hopefully this patch will fail correctly

The last submitted patch, 63: 857312-62-fail.patch, failed testing. View results →

Tested this manually and it seems to work pretty well. Something that is a bit off is that every time something is changed on the form, the message above the table flashes. I'm wondering if that's something we would be able to address?

Fixed #65 with this commit.

Applied MR 3562
Went to the form display.
Changed a formatter option and confirmed the warning.
Changed a formatter setting and saved. Confirmed the warning.

Not noticing the flicker mentioned in #65.

Looks good

There are issues when dragging rows from the active region to the disabled region in the field UI.

To reproduce:

1. go on a content type's manage display page: /admin/structure/types/manage/page/display/teaser
2. The active region has "Body" and "Links" fields.
3. Drag the Moderation control row from the disabled region, and add it below "Links"
4. The "Links" row is pushed down to the disabled region

I was able to remove all the rows from the active region by dragging things around a bit switching rows from active to disabled region. so definitely a timing issue somewhere.

The warning is added when reordering rows but it gets hidden is a row is moved from active to disabled, even though nothing is saved, the warning should be kept.

Additionally the use of the ajaxStart/Success event is a bit problematic since we have our own command processing on top of the jQuery ajax calls this adds a dependency on a workaround in core, which isn't ideal. That might be one of the thing that makes the UI unreliable (haven't looked closely). See #3324062: [Regression] Changes to Drupal.ajax in 9.5.x have caused regressions in paragraphs_features module → .

Umm now the row disapearing/moving to the disabled region doesn't happen. would be good for someone else to try and move things around and see if some unexpected rows gets moved to the disabled region.

The changed warning removed when dragging from active to disabled region is still an issue though (because things are not actually saved).

Nice, this solves the issues when dragging rows.

There a minor inconsistency from the removed ->save(), the changed marker (*) gets removed from all rows when dragging a row across the active/disabled region, ideally it shouldn't be removed but the main warning is still displayed. Information that a save is needed is still there it's just not clear what changed anymore.

couple of details in the MR comments

Good feedback from @nod_

Open threads still it seems.

Seems to have build errors

Running PHPStan on *all* files.
------ ------------------------------------------------------------------------
Line core/modules/field_ui/src/Form/EntityDisplayFormBase.php
------ ------------------------------------------------------------------------
708 Ignored error pattern #^Variable \$updated_rows might not be
defined\.$# in path
/var/www/html/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
is expected to occur 1 time, but occurred 2 times.
720 Variable $updated_rows might not be defined.
------ -------------------------------------------------------

Will need to look into the test failures.

Like the commit message "do not double dip change handlers"

Retested following

Went to the form display.
Changed a formatter option and confirmed the warning.
Changed a formatter setting and saved. Confirmed the warning.

Posted review on the MR

Confirmed that all feedback was addressed and saw that all worked as expected when testing manually

Committed 71613c2 and pushed to 10.1.x. Thanks!

We should write a CR for the new ajax command. Moving to NW for that.

Change record added.

lauriii → closed merge request !3562

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

I'm seeing a possibly unexpected side effect from this in our behat tests, it looks like it's no longer possible to change the region select and edit settings at the same time:

   Scenario: Create a redirect when a taxonomy term has a representative node
     Then I am at "admin/structure/types/manage/article/display"
     And I select "Content" from "fields[field_tags][region]"
+    And I press "Save"
     And I click the element "input[name='field_tags_settings_edit']"
     And I check the box "Link label to the referenced entity"

The settings_edit click no longer works, I need to save first. Not an issue when moving it up with drag and drop, but I can reproduce manually when I switch to displaying row weights.

It could be another issue, but this seems very related.

Another side effect: if we have a custom field settings form element with an ajax callback, and we click any input after that callback run, the entire form refreshes and we loose our input values.

This breaks for example Display Suite:
- Go to manage display
- for any field, edit settings and change "Field Template" from "Master" to "Minimal"
- click for example the "Show label colon" or any other field
-> the entire form refreshes and we loose all values we entered.

Either ds_field_ui_display_overview_multistep_js() needs to be updated to add the ajax-new-content class (similar to how EntityDisplayFormBase::multistepAjax does) or is this be considered as a core bug/regression?