Problem/Motivation

When i go to admin/config/content/diff/fields and hit save right after install, the configuration changes.

Specifically, if i visit node/8/revisions/view/9/10/split_fields
suddenly the field Revision timestamp gets visible.

The current settings form for selection of plugins doesn't work correctly, when a field has hidden selected, go again to the settings form, save again, the hidden selection is gone. Also, when we select a plugin for a field and we add some special config to it, go to the form settings again, update another field (without changing anything on the previous changed field) the previous configured field gets all the config back to default.

I guess multiple orther fields are also enabled.
This is really bad UX.

Proposed resolution

Investigate delta, fix.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#42 changes_when_saving-2826873-42-interdiff.txt8.64 KBBerdir
#42 changes_when_saving-2826873-42.patch23.1 KBBerdir
#38 interdiff-2826873-36-38.txt5.52 KBModernMantra
#38 changes_when_saving-2826873-38.patch17.28 KBModernMantra
#36 interdiff-2826873-32-36.txt2.13 KBModernMantra
#36 chnages_when_saving-2826873-36.patch14.02 KBModernMantra
#32 changes_when_saving-2826873-32-interdiff.txt3.06 KBBerdir
#32 changes_when_saving-2826873-32.patch12.7 KBBerdir
#31 interdiff-2826873-29-31.txt5.79 KBjohnchque
#31 changes_when_saving-2826873-31.patch12.85 KBjohnchque
#29 interdiff-2826873-26-29.txt2.29 KBjohnchque
#29 changes_when_saving-2826873-29.patch10.98 KBjohnchque
#26 interdiff-2826873-23-26.txt2.3 KBjohnchque
#26 changes_when_saving-2826873-26.patch10.2 KBjohnchque
#23 interdiff-2826873-20-23.txt4.8 KBjohnchque
#23 changes_when_saving-2826873-23.patch10.07 KBjohnchque
#21 changes_when_saving-2826873-20.patch6.01 KBjohnchque
#13 config_missing_on_save-2826873-13.patch3.45 KBipumpkin
#12 config_missing_on_save-2826873-12.patch3.45 KBipumpkin
#10 config_missing_on_save-2826873-10.patch2.86 KBipumpkin
#9 interdiff-2826873-7-9.txt1.31 KBjohnchque
#9 changes_when_saving-2826873-9.patch2.93 KBjohnchque
#7 changes_when_saving-2826873-7.patch3.01 KBjohnchque
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker created an issue. See original summary.

johnchque’s picture

We decided to implement this when we added the logic to check if we have some settings saved or not. If we don't have settings saved then we check the visibility in the form display of each field and show it if available. If we already have settings saved then diff take those. Which means that timestamp is not in the form display that is why is enabled but when saving the diff config we set a diff plugin to the timestamp field so it won't check the form display anymore and show all the fields that have a setting set.

miro_dietiker’s picture

@yongt9412 Sure, if settings are stored, those count.
As a fallback, if no settings are present, we should identify feasible default. Those defaults should similarly apply for diff comparision and for the settings form preselected default values.

From what you describe, it seems the form default values have a different fallback than when performing the diff. That's a bug and that doesn't make any sense.

Berdir’s picture

Yes, it might be a bug, but one that is *very* hard to resolve.

In settings, we are on the field storage level, and when diffing an actual entity, we are on the field (per bundle) level. If we have a bundle, then we have better options available for the defaults. Defaults that might even *conflict* between different bundles.

So even if we'd loop over bundles to figure out better defaults, what if two different bundles have different form display settings for some fields. No matter what you do, we can not rule out that saving the global settings without changing it change the defaults of one bundle.

The only really reliable thing to do would be to drop field storage level settings and do it per bundle. Which would make the settings page 10x longer with 10 bundles and 100x longer with 100 bundles and also very painful to update for all bundles. Fun times.

Other than that, all we can try is to better guesses. If we have a base field definition, we can check its default settings. and/or, we can loop over bundles and see if they agree or pick the most common visibility setting.

miro_dietiker’s picture

I'd say we should simply make it visible if any of the bundles has it visible.

johnchque’s picture

Assigned: Unassigned » johnchque

Ok, gonna do this.

johnchque’s picture

Status: Active » Needs review
FileSize
3.01 KB

The logic behind is interesting. Now the defaults are better and when saving the settings form there are no changes in the diff.

Berdir’s picture

  1. +++ b/src/DiffBuilderManager.php
    @@ -172,17 +172,43 @@ class DiffBuilderManager extends DefaultPluginManager {
    +        foreach ($storage->loadByProperties(['targetEntityType' => $field_definition->getTargetEntityTypeId()]) as $value) {
    

    for field definitions we only look at default form displays, now we look at all of them. should we add a view_mode => default filter?

  2. +++ b/src/DiffBuilderManager.php
    @@ -172,17 +172,43 @@ class DiffBuilderManager extends DefaultPluginManager {
    +          $visible = (bool) $value->getComponent($field_definition->getName());
    +          if (!$visible) {
    +            return ['type' => 'hidden'];
    +          }
    +        }
    +      }
    +      else {
    +        foreach ($field_definition->getBundles() as $value) {
    +          if ($display = $storage->load($field_definition->getTargetEntityTypeId() . '.' . $value . '.default')) {
    +            $visible = (bool) $display->getComponent($field_definition->getName());
    +          }
    +          if (!$visible) {
    +            return ['type' => 'hidden'];
    +          }
    

    Isn't this the opposite of what miro said?

    You return hidden if you have one where it isn't visible, Miro said to make it visible if it is shown for at least one of the bundles.

johnchque’s picture

Yes, that's right, changes made. :)

ipumpkin’s picture

When i click save once again, i will lose all saved config data and can't select the diff builder type 'hidden'.
I has some modify and generate a patch ,please review ,thanks.

Status: Needs review » Needs work

The last submitted patch, 10: config_missing_on_save-2826873-10.patch, failed testing.

ipumpkin’s picture

ipumpkin’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
johnchque’s picture

@ipumpkin If you have a bug about losing your data, please open an issue, this is for offering a default config.

johnchque’s picture

[Double post]

ipumpkin’s picture

@yongt9412 thinks your notice.

miro_dietiker’s picture

Status: Needs review » Needs work

So #9 looks better, but is looping over the bundles now a performance problem?

+++ b/src/DiffBuilderManager.php
@@ -172,17 +172,40 @@ class DiffBuilderManager extends DefaultPluginManager {
+        foreach ($storage->loadByProperties(['targetEntityType' => $field_definition->getTargetEntityTypeId(), 'mode' => 'default']) as $value) {
+          $visible = (bool) $value->getComponent($field_definition->getName());
...
+        foreach ($field_definition->getBundles() as $value) {
...
+            $visible = (bool) $display->getComponent($field_definition->getName());

The last bundle always wins now. That's still not as proposed..

It seems the logic isn't healthy enough to make it self explaining, thus we need more commenting.
I think i would try to do this type=hidden a special case for early exit and hope effective logic gets more simple.

Berdir’s picture

  1. +++ b/src/DiffBuilderManager.php
    @@ -172,17 +172,40 @@ class DiffBuilderManager extends DefaultPluginManager {
    +    if ($all_bundles) {
    +      $storage = $this->entityTypeManager->getStorage('entity_form_display');
    +      $visible = FALSE;
    +      if ($field_definition instanceof BaseFieldDefinition) {
    +        foreach ($storage->loadByProperties(['targetEntityType' => $field_definition->getTargetEntityTypeId(), 'mode' => 'default']) as $value) {
    +          $visible = (bool) $value->getComponent($field_definition->getName());
    +        }
    +      }
    +      else {
    +        foreach ($field_definition->getBundles() as $value) {
    +          if ($display = $storage->load($field_definition->getTargetEntityTypeId() . '.' . $value . '.default')) {
    +            $visible = (bool) $display->getComponent($field_definition->getName());
    +          }
    +        }
    +      }
    +      if (!$visible) {
    +        return ['type' => 'hidden'];
    +      }
    +    }
    

    So, discussed this a while with Miro.

    Given the following conditions:

    1. Settings are per field storage, not per field (bundle).
    2. We want the defaults to be consistent with what is shown in the edit form.

    Given those, there is one simple conclusion:

    getSelectedPluginForFieldDefinition() must go or be simplifed to a one-line wrapper for this method. We can't have per-bundle default logic if we want it to be consistent.

    This also means that the all_bundles option will go away, we always need to do this but *only* if we don't have a configured explicit selection.

    The logic in this method then needs to look like this:

    1. Load allowed types and config
    2. If there is config and it is an allowed type OR hidden, return.
    3. else, figure out if there is at least one view display that has the field as visible, if so, return the first type as default. Maybe have this in a separate method, something like getDefaultPluginForFieldStorageDefinition().

    For 3, I found an entity query that should do what we need, you just need to pass it the entity type and optionally a bundles condition and if it returns something, show it, otherwise don't:

    $storage->query()->condition('targetEntityType', 'node')->condition('bundle', ['article', 'page'], 'IN')->exists('content.field_image.type')->range(0, 1)->execute();

  2. +++ b/src/DiffBuilderManager.php
    @@ -172,17 +172,40 @@ class DiffBuilderManager extends DefaultPluginManager {
         if (!$selected_plugin || !in_array($selected_plugin['type'], array_keys($plugin_options))) {
    -      if (!empty($plugin_options)) {
    +      if (!empty($plugin_options) && $selected_plugin['type'] != 'hidden') {
             $selected_plugin['type'] = array_keys($plugin_options)[0];
    

    This is actually a separate bugfix, but it is necessary to avoid overriding an explicitly configured hidden with a default, which is important for performance.

    The fact that this didn't fail indicates that we have probably at least two cases which don't have test coverage related to selecting hidden for a field that has something by default. So make sure that you can set such a field to hidden and that it a) respects and shows that setting again on the settings page, and b) also that the field is actually hidden when diffing.

johnchque’s picture

About 2. there is an issue for it. :) with test coverage #2837896: Field configuration data missing on save

Berdir’s picture

Right, but then I'd suggest you merge it in here, because the fix is now even more important and the other patch will conflict.

johnchque’s picture

Status: Needs work » Needs review
FileSize
6.01 KB

Addressed changes of comments above. One thing I noticed is that with the current code, the title field doesn't have any plugin selected by default, not really sure how to fix that.

Status: Needs review » Needs work

The last submitted patch, 21: changes_when_saving-2826873-20.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review
FileSize
10.07 KB
4.8 KB

Added tests and fixes from the other issue. :)

Berdir’s picture

+++ b/src/DiffBuilderManager.php
@@ -160,7 +162,12 @@
+      else {
+        $selected_plugin = $field_definition->getDisplayOptions('view');
+      }

this is not the selected plugin. It's just a flag for whether the available plugin for this type should be used or not. Meaning, it belongs inside getDefaultPluginForFieldStorageDefinition(), like the existing check.

+++ b/src/Tests/DiffAdminFormsTest.php
@@ -98,9 +98,33 @@ class DiffAdminFormsTest extends DiffTestBase {
+    $edit = [
+      'fields[node.sticky][plugin][type]' => 'hidden',
+    ];
+    $this->drupalPostForm(NULL, $edit, t('Save'));
+    $this->assertFieldByName('fields[node.sticky][plugin][type]', 'hidden');

hm, it seems only this part is related, not the others. fine I guess.

Status: Needs review » Needs work

The last submitted patch, 23: changes_when_saving-2826873-23.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review
FileSize
10.2 KB
2.3 KB

I added the fix for missing data on save, it basically overrides over and over the defaults when a field with custom settings hasn't been opened.

Status: Needs review » Needs work

The last submitted patch, 26: changes_when_saving-2826873-26.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review

I think there are some problems with the if, when checking if it is a BaseFieldDefinition and is display configurable we always return true, what if the region is set as hidden?
Test fails are related with the fact that a body field is hidden in entity display, but it is not hidden when diffing due to that if problem. Not sure how to continue.

johnchque’s picture

Let's see if this works. :)

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/src/DiffBuilderManager.php
    @@ -125,7 +127,7 @@ class DiffBuilderManager extends DefaultPluginManager {
        */
       public function createInstanceForFieldDefinition(FieldDefinitionInterface $field_definition, $bundle = NULL) {
    -    $selected_plugin = $this->getSelectedPluginForFieldDefinition($field_definition, $bundle);
    +    $selected_plugin = $this->getSelectedPluginForFieldStorageDefinition($field_definition->getFieldStorageDefinition(), $bundle);
         if ($selected_plugin && $selected_plugin['type'] != 'hidden') {
    

    the bundle argument needs to go.

  2. +++ b/src/DiffBuilderManager.php
    @@ -139,40 +141,40 @@ class DiffBuilderManager extends DefaultPluginManager {
    +      $selected_plugin['type'] = $this->getDefaultPluginForFieldStorageDefinition($field_definition, $bundles);
    +      if ($selected_plugin['type'] === 'hidden') {
    +        return ['type' => 'hidden'];
    +      }
    
    @@ -184,26 +186,29 @@ class DiffBuilderManager extends DefaultPluginManager {
    +    if (($field_storage_definition instanceof BaseFieldDefinition && $field_storage_definition->isDisplayConfigurable('view')) || $field_storage_definition instanceof FieldStorageConfig) {
    ...
    +    }
    +    else {
    +      $view_options = (bool) $field_storage_definition->getDisplayOptions('view');
    +      return $view_options ? '' : 'hidden';
         }
    

    this if/else is still a bit weird and the else doesn't really make sense.

    Yo return 3 different results, empty string, 'hidden' and $result. But what you really do then is basically treat hidden as false and everything else as true.

    What I had in mind is to move the logic about picking the first available type in here, then the method name makes sense.

    What you have could be OK too, but then just make it a true/false response and name it accordingly.. something like $this->isFieldStorageDefinitionDisplayed()

  3. +++ b/src/DiffBuilderManager.php
    @@ -184,26 +186,29 @@ class DiffBuilderManager extends DefaultPluginManager {
    +      if ($bundles) {
    +        $query->condition('bundle', (array) $bundles, 'IN');
           }
    

    bundles is not input then, you need to get it from the field if it is a FieldStorageConfig, if it's not then, no bundles filter.

johnchque’s picture

Status: Needs work » Needs review
FileSize
12.85 KB
5.79 KB

Made those changes.
Didn't do the first suggested approach because that would create problems when checking that a plugin is still applicable.
This should work. :)
Wondering, so this means that if we don't have any plugin stored for body, it will always be displayed unless it is hidden in view display of page and article content types right?

Berdir’s picture

I think the logic is fine now.

I did clean up the code a bit so we need fewer conditions and nesting, there is a single if/elseif/else now. I also make sure that we always return a settings key, then we can save another if/else in the createInstance() call.

And improved docs of the return value.

johnchque’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

That logic is indeed nicer, don't have anything to complain about. It also passes all the tests of losing data. :)
Updated IS.

johnchque’s picture

This actually works fine, there are no changes anymore and the defaults are consistent, the only problem with this patch, is that when we save the field settings form without changing anything it set all the plugins back to the default config. Previously it would reset the field that hasn't been updated when other was being updated. Not sure if that should be fixed in a follow-up.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

No, IMHO we should fix that as it's another severe regression.
And if it would be a follow-up, it should have been created prior to committing / RTBC this.

ModernMantra’s picture

Created an issue as suggested by Berdir and made possible fix. Locally i have some failures in tests, however in UI the patch fixes the problem

Status: Needs review » Needs work

The last submitted patch, 36: chnages_when_saving-2826873-36.patch, failed testing.

ModernMantra’s picture

Status: Needs work » Needs review
FileSize
17.28 KB
5.52 KB

Worked together with Sascha, here is new patch that possibly fixes test failures.

Status: Needs review » Needs work

The last submitted patch, 38: changes_when_saving-2826873-38.patch, failed testing.

johnchque’s picture

It seems it doesn't use the stored config? Because the show id is checked, but when diffing is not, its default is FALSE.

Berdir’s picture

Assigned: johnchque » Berdir

Yeah. On the first time we submit the form, field_image is processed but then not in the final config. Something is going wrong in a very weird way.

Looking into it.

Berdir’s picture

Ok, cleaned the config saving up and now everything works fine as far as I see ;)

Follow-up would be to move validate and submit of plugin settings form to when you actually click Update, so we can actually show you those validation errors *when* you still see the form fields. Wouldn't that be amazing ;) Of course there is no plugin that even needs validation, so not very important.

  • miro_dietiker committed 672bf6c on 8.x-1.x authored by Berdir
    Issue #2826873 by yongt9412, Berdir, ModernMantra, miro_dietiker:...
miro_dietiker’s picture

Assigned: Berdir » Unassigned
Status: Needs review » Fixed

Party party - so much beautification in a single issue... ;-) Committed, hope the dragons and monsters are gone now!

Status: Fixed » Closed (fixed)

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