Dependencies from only one instance of a widget are used in display modes

The whole method looks messy. Take a look at it :

public function getPluginCollections() {
  $configurations = [];
  foreach ($this->getComponents() as $field_name => $configuration) {
    if (!empty($configuration['type']) && ($field_definition = $this->getFieldDefinition($field_name))) {
      $configurations[$configuration['type']] = $configuration + [
        'field_definition' => $field_definition,
        'view_mode' => $this->originalMode,
      ];
    }
  }

  return [
    'formatters' => new EntityDisplayPluginCollection($this->pluginManager, $configurations)
  ];

What we see is that if two fields use the same plugin the later will be the only one for which configuration of the field definition will be included.

I think here we have to use as key the field name and then at the second level the plugin ID or at first level something like field_name.plugin_id, but this will not be a BC change.

I ran into this issue while developing a new module, Entity Reference Facet Link. Basically, the module provides a field formatter that renders ER fields as links to a faceted search page. To do this, the entity display becomes dependent on one or more facet config entities.* If there is more than one field on an entity that uses this formatter plugin, then only the last one has its dependent facet entity added to the list of display dependencies due to this bug.

FYI, I tracked down the issue where EntityViewDisplay::getPluginCollections() was added: #2271419: Allow field types, widgets, formatters to specify config dependencies.

I'll do a review of the patch next.

*I have not committed the calculateDependencies() code yet because until now I wasn't sure if this was a problem with my implementation or a bug in core.

This all looks good to me. Great job writing these tests, @james.williams!

I don't think there's any issue with changing the array keys for a couple of reasons. First, the parent issue shows that the getPluginCollections() methods were added specifically to allow plugins to declare their dependencies to these entities. So updating them to fix a bug with that functionality shouldn't be an issue. Second, ConfigEntityBase::calculateDependencies() is the method that calls getPluginCollections() to incorporate those plugins' dependencies. It ignores the collection keys entirely.

1. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -270,6 +270,67 @@ public function testFieldComponent() {
   +        'dependent_module' => 'aggregrator',
   +      ],
   +    ]);
   +
   +    $dependencies = $display->calculateDependencies()->getDependencies();
   +    $this->assertArraySubset(['action', 'aggregrator'], $dependencies['module']);
   

   Minor nitpicks - "aggregator" is misspelled on lines 326 and 331.

2. +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -116,6 +116,67 @@ public function testFieldComponent() {
   +        'dependent_module' => 'aggregrator',
   +      ],
   +    ]);
   +
   +    $dependencies = $form_display->calculateDependencies()->getDependencies();
   +    $this->assertArraySubset(['action', 'aggregrator'], $dependencies['module']);
   

   It's misspelled twice here too.

I'm not going to hold up the issue by setting it back to NW or by rolling a new patch myself just for that though. This is something that could easily be fixed on commit. The tests only check for those array values to be the same. There's no reason to try and load those dependencies, which means those strings could have any value.

#9 gets an RTBC from me.

Oh. Yeah. For the record I tested to see if this definitely fixes the issue I was having with ERFL. It does. After applying the patch all of the facet dependencies get added.

Unrelated failure.

Applied the issue summary template and rewrote the problem section so that it will be easier to get this some attention.

Formatted the code section correctly. Sorry for the noise.

This qualifies as a major bug because it can result in a PHP error through the user interface. If a missing dependency is deleted then the form/view display will WSOD. The only workarounds to restore functionality that I know of are to re-add a plugin instance of the dependency with the same machine name or manually edit the configuration.

Test fail looks related to me - its to do with field config dependencies being in incorrect order

Looks like the test failed because multiple things had the same weight, which came out ordered differently in different environments / versions of PHP.

This seems likely. I did some digging and the plugin manager uses uasort() on the weight column. The sort order when two elements have the same value is undefined. Assuming the 8.4.x patch comes back green #25 gets an RTBC from me.

1. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -270,6 +270,67 @@ public function testFieldComponent() {
   +    $first_field_name = 'test_field';
   +    // Create a field storage and a field.
   +    $first_field_storage = FieldStorageConfig::create([
   +      'field_name' => $first_field_name,
   +      'entity_type' => 'entity_test',
   +      'type' => 'test_field'
   +    ]);
   +    $first_field_storage->save();
   +    $first_field = FieldConfig::create([
   +      'field_storage' => $first_field_storage,
   +      'bundle' => 'entity_test',
   +    ]);
   +    $first_field->save();
   +
   +    $second_field_name = 'test_field_2';
   +    // Create a field storage and a field.
   +    $second_field_storage = FieldStorageConfig::create([
   +      'field_name' => $second_field_name,
   +      'entity_type' => 'entity_test',
   +      'type' => 'test_field'
   +    ]);
   +    $second_field_storage->save();
   +    $second_field = FieldConfig::create([
   +      'field_storage' => $second_field_storage,
   +      'bundle' => 'entity_test',
   +    ]);
   +    $second_field->save();
   
   +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -116,6 +116,67 @@ public function testFieldComponent() {
   +    $first_field_name = 'test_field';
   +    // Create a field storage and a field.
   +    $first_field_storage = FieldStorageConfig::create([
   +      'field_name' => $first_field_name,
   +      'entity_type' => 'entity_test',
   +      'type' => 'test_field'
   +    ]);
   +    $first_field_storage->save();
   +    $first_field = FieldConfig::create([
   +      'field_storage' => $first_field_storage,
   +      'bundle' => 'entity_test',
   +    ]);
   +    $first_field->save();
   +
   +    $second_field_name = 'test_field_2';
   +    // Create a field storage and a field.
   +    $second_field_storage = FieldStorageConfig::create([
   +      'field_name' => $second_field_name,
   +      'entity_type' => 'entity_test',
   +      'type' => 'test_field'
   +    ]);
   +    $second_field_storage->save();
   +    $second_field = FieldConfig::create([
   +      'field_storage' => $second_field_storage,
   +      'bundle' => 'entity_test',
   +    ]);
   +    $second_field->save();
   

   Some duplication here that could be in a protected method called from both tests.

2. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -270,6 +270,67 @@ public function testFieldComponent() {
   +    // Check that providing no options results in default values being used.
   +    $display->setComponent($first_field_name, [
   +      'type' => 'field_test_dynamic_dependencies',
   +      'weight' => 0,
   +      'settings' => [
   +        // An arbitrary module that should not already be a dependency.
   +        'dependent_module' => 'action',
   +      ],
   +    ]);
   +
   +    $display->setComponent($second_field_name, [
   +      'type' => 'field_test_dynamic_dependencies',
   +      'weight' => 0,
   +      'settings' => [
   +        // Another arbitrary module that should not already be a dependency.
   +        'dependent_module' => 'aggregrator',
   +      ],
   +    ]);
   +
   +    $dependencies = $display->calculateDependencies()->getDependencies();
   +    $this->assertArraySubset(['action', 'aggregrator'], $dependencies['module']);
   
   +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -116,6 +116,67 @@ public function testFieldComponent() {
   +    // Check that providing no options results in default values being used.
   +    $form_display->setComponent($first_field_name, [
   +      'type' => 'test_field_widget_dynamic_dependencies',
   +      'weight' => 0,
   +      'settings' => [
   +        // An arbitrary module that should not already be a dependency.
   +        'dependent_module' => 'action',
   +      ],
   +    ]);
   +
   +    $form_display->setComponent($second_field_name, [
   +      'type' => 'test_field_widget_dynamic_dependencies',
   +      'weight' => 0,
   +      'settings' => [
   +        // Another arbitrary module that should not already be a dependency.
   +        'dependent_module' => 'aggregrator',
   +      ],
   +    ]);
   +
   +    $dependencies = $form_display->calculateDependencies()->getDependencies();
   +    $this->assertArraySubset(['action', 'aggregrator'], $dependencies['module']);
   

   same here

PHP Fatal error:  Uncaught Error: Cannot instantiate abstract class Drupal\Tests\field_ui\Kernel\EntityDisplayTestBase in /var/www/html/core/scripts/run-tests.sh:786
Stack trace:
#0 /var/www/html/core/scripts/run-tests.sh(67): simpletest_script_run_one_test('838', 'Drupal\\Tests\\fi...')
#1 {main}
  thrown in /var/www/html/core/scripts/run-tests.sh on line 786

I'm not sure if I've gone overboard here.

Personally, I would say "yes." I know you were asked to do this, but in fulfilling the request you ended up touching other code that is unrelated to this issue. That can be kind of bad. Now this patch might cause a dozen others to need to be rerolled. Maybe we don't care and duplicate code elimination is more important. I feel like I can't make that call. It would be nice if @larowlan would review it. I'm going to tag it for subsystem maintainer review and hope that gets it some attention from someone who can make the determination as to whether or not this should go forward.

+++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetDynamicDependencies.php
@@ -0,0 +1,81 @@
+ * Plugin implementation of the 'test_field_widget' widget.

Nitpick that may be able to be fixed on commit: the machine name of the plugin wasn't updated here.

Also, "aggregator" was still misspelled in the dependency test helper method, not that it has any effect at all.

a helper trait is probably best if they're different test classes

This seems ok to me. I don't think it's such a big deal to have the little bit of duplicated code in there, since it's mostly setting up the field names. In the future we might need to add another field to one, but not the other for some reason. Maybe. If that ever happened, then it would be a good idea to let the tests define their own field set.

Anyway, I'm more comfortable RTBCing this since it doesn't touch other code like the last patch did. I only have the same minor nitpicks that I've had before, which I hope can be fixed on commit:

1. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetDynamicDependencies.php
   @@ -0,0 +1,81 @@
   + * Plugin implementation of the 'test_field_widget' widget.
   

   This machine name wasn't updated.

2. +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -270,6 +273,27 @@ public function testFieldComponent() {
   +      'test_field_2' => 'aggregrator',
   
   +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
   @@ -116,6 +119,26 @@ public function testFieldComponent() {
   +      'test_field_2' => 'aggregrator',
   

   Misspelling that is totally irrelevant to the functionality of the patch.

Fixed the nit-picks in #36.

The changes are so minor that we can safely set this back to RTBC.

1. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTestTrait.php
   @@ -0,0 +1,76 @@
   +  public function addDefaultTestFields($field_names, $type = 'test_field') {
   

   I think this should be protected?

2. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTestTrait.php
   @@ -0,0 +1,76 @@
   +      $created[$field_name] = [
   +        $field_storage,
   +        $field,
   +      ];
   

   This always feels like a code smell - perhaps this should be two separate methods?

3. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTestTrait.php
   @@ -0,0 +1,76 @@
   +    return $created;
   
   +++ b/core/modules/field_ui/tests/src/Kernel/EntityDisplayTest.php
   @@ -270,6 +273,27 @@ public function testFieldComponent() {
   +    $this->addDefaultTestFields(array_keys($dependent_fields));
   

   Actually - doesn't look like we're using the return? So probably can just drop the return? If someone needs it in the future, then we cross that bridge then?

What happens with existing config here?
Do we need an update hook to ensure that all config dependencies are updated accordingly?
I'll ping some folks for the subsystem maintainer review.

I would be against option B (therefore in favour of option A, I guess, in the absence of an option C). Option B could make things (mysteriously) disappear while option A doesn't change the situation (so at least it doesn't make matters worse!).

Heh, I was writing a response to #41 and agreeing with @larowlan while these other comments were being posted.

I think that we do need an update function. Any existing configuration needs to be repaired. It would be really simple because all that needs to happen is re-saving the display entities, which will cause them to recalculate their dependencies. Something like this would probably work:

function field_update_80xx() {
  foreach (\Drupal::entityTypeManager()->getStorage('entity_form_display')->loadMultiple() as $form) {
    $form->save();
  }
  foreach (\Drupal::entityTypeManager()->getStorage('entity_view_display')->loadMultiple() as $view) {
    $view->save();
  }
}

Of course, it might not be as simple as that since it's likely that update tests will need to be written.

I tested this with @charlotte.b and we confirmed that the patch fixed the issue.

I followed the steps and was able to reproduce the missing dependency, after applying the patch the dependencies were correct.

I have marked this to needs work as it looks like more work is needed to update existing config.

The latest comments suggest this needs an update hook and related tests, so it looks like more than a simple reroll. I will update the issue summary.

rerolled the patch on #51 and the patch cleanly

I also stumbled about this issue while trying to implement a widget third-party setting and to get things forward, I gave it a try and rerolled the patch including the necessary test rewrites. I hope I didn't miss something.

Don't ask me what happened with the interdiff, but I can't get correct output for it - I guess it is because of the changed test file locations.

When this patch is reviewed successfully it can be used as a base for adding the update hook and the update test coverage.

Let's better hide the unreadable interdiff ;)

Here is an updated patch with updated order on expected select options. Let's see what happens...

Trigger testbot.

Okay, there were some more order fixes required.

So this looks much better and therefore the reroll should be completed. Remaining tasks are still the update hook to resave all displays to recalculate dependencies correctly and tests for this update operation.

Contrary to the text in the "Remaining tasks" of issue description, I think this problem also interferes with correct dependency calculation for third-party settings. In my case, I have a third party setting for a specific widget type that is just a checkbox. If checked, the third-party setting is added to the widget settings, but if not checked, the third-party setting gets removed from the widget settings in order to have no obsolete dependencies for a feature that is not enabled (implemented via hook_entity_presave() to clean up the third-party settings array where needed).
Without this patch this also results in incorrect dependencies if you use the corresponding widget twice in a form display - when the first widget sets the third-party setting and a latter one doesn't, the required dependency is not added, because the latter one does not register it anymore due to no related third-party setting.

To get things moving, here is an updated patch that includes the update hook to resave all entity view/form displays with recalculated dependencies. I'll investigate how to get test coverage for this, but if anybody is more familiar with tests like these, feel free to jump in ;)

Haven't read the full issue comments but the summary reads well.

Needs tests for the update hook and almost certainly needs a reroll.

Forgot the most important tag :P

This was triaged as part of the daily BSI triage targets.

Re-Rolled patch 2865710-65.patch for 10.1.x branch.

Left MR comments

I messed up that MR while trying to update it for 11.x. I'll sort it out tomorrow.

This is going to fail tests because I excluded all the entity_test lines from the fixture that I created. But I'm out of time to work on this right now and need to save the work. My plan is to move the test plugins out of the field_test module so that we don't have to depend on that anymore, reducing the amount of garbage that has to be in the fixture.

My plan is to move the test plugins out of the field_test module so that we don't have to depend on that anymore...

After evaluating how much work that would take I ended up putting all of the entity_test module stuff in the fixture. It's a pain, but it worked without having to refactor code to work without it.

Just one minor comment on the MR, I think its ok, but would be good for a second opinion

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I had to do a major reconfiguration of the tests. The test plugins were originally put into a module that had a dependency on entity_test. It seems like entity_test has been updated recently, which caused the fixture to be out of date. That would be a perpetual problem. We can't have that. So I moved the plugins to an isolated test module without a dependency.

The original reason I requested subsystem maintainer review over eight years ago is no longer relevant. There were extensive modifications being made to existing test classes. Those modifications were removed from the patches/MR. I've removed the tag.

Postponing on #3577684: uninstall-contact.php doesn't delete entity displays which prevents these update tests from working.

This just got unblocked.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.