Problem/Motivation

It turned out in #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles that widgets and formatters settings may depend on other entities. When the EntityFormDisplay and EntityViewDisplay are reacting on dependency removal they are not asking components (widgets/formatters) if they like to take some action.

Proposed resolution

  • Add code in EntityDisplayBase::calculateDependencies() that iterates through the display components to collect dependencies from widgets/formatters. This is already in place. Right now, by implementing calculateDependencies() in a formatter/widget the dependency is added to the display list of dependencies.
  • Add code in EntityDisplayBase::onDependencyRemoval() that iterates through the display components and allow them to respond with their own onDependencyRemoval() action. Check also, on each component, if its plugin provides unresolved dependencies and disable that component if so. A component having settings that depend on removed dependencies (config entities, content entities, uninstalled modules, etc.) is broken and should will be disabled. Log the fact that the component has been disabled.
  • Add a new onDependencyRemoval() method in \Drupal\Core\Field\PluginSettingsInterface. This needs discussion to fix the point in class hierarchy where to add this method.

Remaining tasks

None.

User interface changes

None.

API changes

A new interface method onDependencyRemoval() for \Drupal\Core\Field\PluginSettingsInterface (?)

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because components cannot act on dependency removal to update their configuration.
Issue priority Not critical because for specific cases you can still apply workarounds to update components settings on dependency removal.
Disruption No disruption.
CommentFileSizeAuthor
#77 interdiff.txt4.14 KBclaudiu.cristea
#77 2562107-77.patch23.19 KBclaudiu.cristea
#73 interdiff.txt1.64 KBclaudiu.cristea
#73 2562107-73.patch23.16 KBclaudiu.cristea
#71 entitydisplaybase-2562107-70.patch22.92 KBjibran
#71 interdiff.txt1.16 KBjibran
#62 interdiff.txt1.77 KBclaudiu.cristea
#62 entitydisplaybase-2562107-62.patch22.92 KBclaudiu.cristea
#58 interdiff.txt1.77 KBclaudiu.cristea
#58 entitydisplaybase-2562107-58.patch22.92 KBclaudiu.cristea
#57 interdiff.txt3.2 KBclaudiu.cristea
#57 entitydisplaybase-2562107-57.patch23.02 KBclaudiu.cristea
#54 interdiff.txt1 KBclaudiu.cristea
#54 entitydisplaybase-2562107-54.patch20.82 KBclaudiu.cristea
#52 interdiff.txt5.59 KBclaudiu.cristea
#52 2562107-52.patch20.82 KBclaudiu.cristea
#49 interdiff.txt13 KBclaudiu.cristea
#49 2562107-49.patch19.32 KBclaudiu.cristea
#46 interdiff.txt1.22 KBclaudiu.cristea
#46 2562107-46.patch15.45 KBclaudiu.cristea
#44 interdiff.txt6.97 KBclaudiu.cristea
#44 2562107-44.patch15.44 KBclaudiu.cristea
#33 2562107-33.patch14.07 KBclaudiu.cristea
#28 interdiff.txt4.35 KBclaudiu.cristea
#28 2562107-28.patch14.06 KBclaudiu.cristea
#26 interdiff.txt3.12 KBclaudiu.cristea
#26 entitydisplaybase-2562107-26.patch14.1 KBclaudiu.cristea
#22 interdiff.txt4.29 KBclaudiu.cristea
#22 2562107-22.patch13.98 KBclaudiu.cristea
#21 interdiff.txt1.33 KBclaudiu.cristea
#21 2562107-21.patch13.99 KBclaudiu.cristea
#18 2562107-18.patch13.97 KBclaudiu.cristea
#18 interdiff.txt5.16 KBclaudiu.cristea
#16 interdiff.txt9.18 KBclaudiu.cristea
#16 2562107-16.patch12.41 KBclaudiu.cristea
#14 2562107-14.patch9.44 KBclaudiu.cristea
#14 interdiff.txt3.04 KBclaudiu.cristea
#13 interdiff.txt2.29 KBclaudiu.cristea
#13 2562107-13.patch8.9 KBclaudiu.cristea
#6 2562107-6.patch8.87 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Title: EntityFormDispaly and EntityViewDisplay should ask their components for dependencies » EntityDisplayBase should ask his components for dependencies
claudiu.cristea’s picture

Category: Task » Bug report

I think that this is a bug while it prevents us to react on a dependency change.

jibran’s picture

Title: EntityDisplayBase should ask his components for dependencies » EntityDisplayBase doesn't add components dependencies

Now it seems like a bug report ;-)

andypost’s picture

Title: EntityDisplayBase doesn't add components dependencies » EntityDisplayBase should add dependencies from its components

like a challenge, bug that dependencies are not calculated

claudiu.cristea’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
8.87 KB

Updated IS with the finding that dependencies defined by components are added to the display. This is in place.

This patch is supposing to fix the issue but for some reason when deleting the dependency added by a component, the whole display entity gets deleted. I read carefully the docs (from \Drupal\Core\Config\Entity\ConfigDependencyManager) that states:

 * When uninstalling a module or a theme, configuration entities that are
 * dependent will also be removed. This default behavior can lead to undesirable
 * side effects, such as a node view mode being entirely removed when the module
 * defining a field or formatter it uses is uninstalled. To prevent this,
 * configuration entity classes can implement
 * \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval(),
 * which allows the entity class to remove dependencies instead of being deleted
 * themselves.

And in the attached test I did that: onDependencyRemoval() returns TRUE but the entity still gets deleted. Please look at the patch and test scenario. I tried to read the code from dependency manager but I'm out of ideas and inspiration. I really need some help here to debug this dependency issue, which IMO seems like a bug.

claudiu.cristea’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 6: 2562107-6.patch, failed testing.

claudiu.cristea’s picture

Title: EntityDisplayBase should add dependencies from its components » EntityDisplayBase should react on removal of its components dependencies

Update title to reflect the IS.

Status: Needs work » Needs review

claudiu.cristea queued 6: 2562107-6.patch for re-testing.

claudiu.cristea’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 6: 2562107-6.patch, failed testing.

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.9 KB
2.29 KB

Thank you @Berdir for pointing me in the right direction with the dependency stuff on this issue.

Added beta evaluation.

claudiu.cristea’s picture

Issue summary: View changes
FileSize
3.04 KB
9.44 KB

Small tweaks.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -428,13 +428,21 @@ public function onDependencyRemoval(array $dependencies) {
+
+        // Give this component the opportunity to react on dependency removal.
+        if ($component_changed = $renderer->onDependencyRemoval($dependencies)) {
+          // Update component settings to reflect changes.
+          $component['settings'] = $renderer->getSettings();
+          $this->setComponent($name, $component);
+          $changed = TRUE;
+        }

That looks good I think.

What we should do is check all formatters that provide dependencies and implmement this method, if possible.Not sure if here or in follow-ups, implementing it is probably not hard, but writing tests and deciding what to do might result in complications. But we should at least have a list of follow-ups.

One thing I mentioned in IRC is, what if a plugin provides a dependency and doesn't remove it. If we don't do anything, then the whole display will be dropped ,that seems bad.

I think a better way out is to remove that component, if there is any overlap between the module or config dependencies that are being removed. You'll have to call the method and array_diff() it. If we do that, then it also seems to be a lesser problem to fix the various formatters in follow-ups.

One interesting case is going to be the image formatter. If you delete an image style, you can select a replacement. I really have no idea what exactly happens there, we should definitely test that extensively. We might be deleteting the dependencies before we have a chance to update the configuration, not sure. Or maybe it is a separate hook, then it probably works in the UI but not the API?

claudiu.cristea’s picture

FileSize
12.41 KB
9.18 KB

@Berdir,

This is a patch based on #15 and IRC discussion. At least this was my understanding. But right now the test makes PHP to exhaust all the memory and crash with a fatal error. I'm out of ideas why this happens and where is, maybe, an infinite loop.

Status: Needs review » Needs work

The last submitted patch, 16: 2562107-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
13.97 KB

Ok, ok, the test had exhausted the memory by using $this->assertNoNull() on an entity. It seems that this assertion doesn't work on objects?

Added also the logging of the component disabling.

claudiu.cristea’s picture

Issue summary: View changes

Fixing the IS to reflect the idea from #15.

claudiu.cristea’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
+        // If there are unresolved deleted dependencies left, disable this
+        // component to avoid the removal of the entire display entity.
+        elseif (static::isUnresolved($renderer->calculateDependencies(), $dependencies)) {

Now I'm asking myself if this should be an if () or an elseif ()? I tried with if () but in that case the test fails. And the test scenario seems correct.

claudiu.cristea’s picture

FileSize
13.99 KB
1.33 KB

Small docs fixes.

claudiu.cristea’s picture

FileSize
13.98 KB
4.29 KB

Here's the reply to #20. In fact I was confused about the array_diff term used. So it's an intersect, not a diff.

Status: Needs review » Needs work

The last submitted patch, 22: 2562107-22.patch, failed testing.

claudiu.cristea queued 22: 2562107-22.patch for re-testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

Some small fixes.

Added #2565513: Replace ImageStyle removal workaround with the entity display native behavior as followup for the image style removal case. Now we have 2 use cases of this bug fixing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
@@ -122,4 +122,23 @@ public function calculateDependencies() {
+   * {@inheritdoc}
+   */
+  public function hasUnresolvedDependencies(array $removed_dependencies) {
+    foreach ($this->calculateDependencies() as $type => $dependencies) {
+      if (array_intersect($dependencies, array_keys($removed_dependencies[$type]))) {
+        return TRUE;
+      }
+    }
+    return FALSE;

I don't think this method should be on the plugins, there is no need for them to customize this?

claudiu.cristea’s picture

FileSize
14.06 KB
4.35 KB

I knew it :)

claudiu.cristea queued 28: 2562107-28.patch for re-testing.

claudiu.cristea’s picture

Issue tags: +ER check for RC

Tagging.

Status: Needs review » Needs work

The last submitted patch, 28: 2562107-28.patch, failed testing.

lokapujya’s picture

Priority: Normal » Major
claudiu.cristea’s picture

Assigned: Unassigned » yched
Status: Needs work » Needs review
Issue tags: +rc target, +missing config dependencies
FileSize
14.07 KB

Rerolling, tagging. I'm assigning this to @yched for review as we've discussed in Barcelona.

nlisgo’s picture

Just read through the patch manually for any errors. This is what I found.

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
+   * @param array[] $plugin_dependencies
+   *   A list of dependencies having the same structure as the return value of
+   *   ConfigEntityInterface::calculateDependencies().
+   * @param array[] $removed_dependencies
+   *   A list of dependencies having the same structure as the input argument of

should be just array without the square brackets.

claudiu.cristea’s picture

@nlisgo, no. You can specify that there's an array there by suffixing the type with square brackets. That is read as "$removed_dependencies is an array of arrays". See https://www.drupal.org/coding-standards/docs#types.

nlisgo’s picture

Just had a chat with @thewilkybarkid about this and it might be justified here but I could not find many other uses (3 only) in the codebase but this could be used to indicate an array of array's. I will look a bit more into it and comment again but ignore for now unless anyone else wants to clear things up :)

Ignore previous comment. This appears to be supported.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
    -      if (isset($component['type']) && $definition = $this->pluginManager->getDefinition($component['type'], FALSE)) {
    -        if (in_array($definition['provider'], $dependencies['module'])) {
    +      if ($renderer = $this->getRenderer($name)) {
    

    One consequence of this change is that we will now be instanciating Formatter/Widget objects for all fields in the Display each time one of the Display's dependencies gets removed. This might add a non-negligible CPU cost on a full config sync with lots of removals.

    I don't think there's a way around that though, right ? The moment some dependencies of the Display disappear, we have to invoke each Formatter to let it react if it needs (and we don't track which formatter added which dependency)

    [edit : discussed with @alexpott : yeah, no way around that. Please ignore that comment]

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
    

    The weird thing is I'm not sure I see where the calculateDependencies() method of each Formatter/Widget actually gets called at the moment.

    We're allowing Formatter/Widget to react to one of their dependencies being removed, but I don't see where we collect their dependencies in the first place... I'm probably missing something :-)

    [edit : discussed with @alexpott : OK, that is done in ConfigEntityBase, because EntityDisplays use a LazyPluginCollection. Please ignore that comment]

  3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
    +        if ($component_changed = $renderer->onDependencyRemoval($dependencies)) {
    

    looks like the $component_changed var is actually not used ?

  4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
    +          $this->logger->warning("@display '@id': Component '@name' was disabled because its settings depend on removed dependencies.", $arguments);
    

    Is that something we typically do in other similar cases ? Should we ask @alexpott for feedback on that ?

  5. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,70 @@ public function onDependencyRemoval(array $dependencies) {
    +  protected function checkUnresolvedDependencies(array $plugin_dependencies, array $removed_dependencies) {
    

    This is called only once, so maybe inlining it could save the burden of having to write a phpdoc ? ;-)

  6. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
    @@ -95,4 +95,29 @@ public function getThirdPartySetting($module, $key, $default = NULL);
    +   * Runs update actions when this plugin is asked to react on removal of its
    +   * dependencies.
    

    Exceeds 80 chars. Based on other existing implementaions of onDependencyRemoval() (like in ConfigEntityInterface or FieldItemInterface), I'd suggest something like :

    "Informs the plugin that some configuration it might depend on will be deleted."

    The rest of the phpdoc is pretty neat :-)

  7. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
    @@ -95,4 +95,29 @@ public function getThirdPartySetting($module, $key, $default = NULL);
    +   * pointing to an arbitrary config entity. When that config entity is deleted,
    +   * this method is called by the view display to react on the dependency
    +   * removal by updating its configuration.
    

    minor : s/react on/react to

yched’s picture

Also, we need to take care of third_party_settings.

Each module providing a "third party setting" for the widget/formatter gets automatically added as a dependency of the Display :

  PluginSettingsBase::calculateDependencies() {
    if (!empty($this->thirdPartySettings)) {
      // Create dependencies on any modules providing third party settings.
      return array(
        'module' => array_keys($this->thirdPartySettings)
      );
    }
    return array();
  }

So by default, and unless we take some explicit steps about that in onDependencyRemoval(), then the Display will be deleted when one of those modules get uninstalled, right ? That seems pretty bad :-)

ConfigEntityBase::onDependencyRemoval() takes care of that for the third_party_settings of the ConfigEntity (basically, just remove the third_party_setting provided by the module being removed) :

if (!empty($this->third_party_settings)) {
  $old_count = count($this->third_party_settings);
  $this->third_party_settings = array_diff_key($this->third_party_settings, array_flip($dependencies['module']));
  $changed = $old_count != count($this->third_party_settings);
}

PluginSettingsBase::onDependencyRemoval() should do the same for the third_party_settings of the Formatter/Widget

yched’s picture

Otherwise, forgot to add : awesome work, @claudiu.cristea :-)

yched’s picture

One thing to consider, too :

We're not tracking which formatter added which dependency in its calculateDependencies(), we're merging them all in a single list of dependencies of the Display.

This means that, in Display::onDependencyRemoval(), all we know is that *at least one of the formatters* depended on the dependency (that's actually not even sure, since the Display has dependencies of its own that don't necessarily come from a Formatter).
And we call Formatter::onDependencyRemoval() for each of them, passing dependencies they might actually not depend on. If they return FALSE in that case, then they will get "hidden", while they were in fact not affected at all by the dependencies that are being removed.

So the "hide the field if Formatter::onDependencyRemoval() returns FALSE" might be a bit dangerous ?
Or we should document it as "Formatter::onDependencyRemoval() should return TRUE unless they *really* want to become hidden" ?

yched’s picture

Discussed #40 ("we call Formatter::onDependencyRemoval() for each of them, passing dependencies they might actually not depend on") with @alexpott

He said that Display::onDependencyRemoval($dependencies) should do (pseudo-code) :

foreach ($formatter) {
  $formatter_dependencies = $formatter->calculateDependencies();
  $formatter_dependencies_being_removed = array_intersect($dependencies, $formatter_dependencies);
  if ($formatter_dependencies_being_removed) {
    $formatter_remains_valid = $formatter->onDependecyRemoval($formatter_dependencies_being_removed);
  }
}

Meaning : call $formatter->calculateDependencies() again before calling its onDependecyRemoval(), so that we can pass dependencies that we know are actual dependencies of that formatter, and do not bug the formatter when the dependencies being removed are ones it doesn't care about.

claudiu.cristea’s picture

Assigned: yched » claudiu.cristea
Priority: Major » Critical

@yched, Thank you for the detailed review. I will jump today on this.

Meanwhile, there are two *major* issues blocked on this and due to the nature of this bug I think that this is critical:

Berdir’s picture

Priority: Critical » Major

That's not how this works :)

Blocking majors doesn't make an issue a critical it just makes it a major too.

claudiu.cristea’s picture

FileSize
15.44 KB
6.97 KB

#37.3: OK.

#37.4: We need to inform the site builder about the fact that he just disabled a formatter/widget. I don't see other way, but open to any suggestion.

#37.5: That I merged with #41. Anyway, I put the code in a new function not for code reusing but for clarity of onDependencyRemoval().

#37.6: OK.

#37.7: OK.

#38: Good catch. I missed that. Added.

#41: Implemented.

claudiu.cristea’s picture

Assigned: claudiu.cristea » yched

Green. Passing for review.

claudiu.cristea’s picture

Assigned: yched » claudiu.cristea
FileSize
15.45 KB
1.22 KB

Docs fixes.

claudiu.cristea’s picture

Assigned: claudiu.cristea » yched

Ouch, reassigning.

yched’s picture

Status: Needs review » Needs work

Thanks @claudiu.cristea - last adjustements / nitpicks :-)

  1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,91 @@ public function onDependencyRemoval(array $dependencies) {
    +            $component['settings'] = $renderer->getSettings();
    

    We should also update $component['third_party_settings'] now ?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,91 @@ public function onDependencyRemoval(array $dependencies) {
    +   * - The two arguments doesn't have the same structure.
    +   * - $removed_dependencies has already sane defaults. All the types of events
    +   *   are filled in, even with empty arrays.
    

    s/doesn't/do not
    s/has already/already has

    + I'm not sure "events" is the right word ? (Not sure that second point about defaults and empty arrays is really needed actually)

  3. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,91 @@ public function onDependencyRemoval(array $dependencies) {
    +  protected function getPluginRemovedDependencies(array $plugin_dependencies, array $removed_dependencies) {
    

    OK, fine as a helper method, because that intersect logic could be worth moving up to ConfigEntityBase at some point, as generic logic useful for any ConfigEntity that uses a PluginCollection (we discussed that with @alexpott at the post-con sprint, part of what this patch is solving here is a more generic issue than just EntityDisplays).

    Separate issue though, let's keep it here for now :-)

  4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,91 @@ public function onDependencyRemoval(array $dependencies) {
    +        // Build a list of removed dependency names for each dependency type.
    

    That comment with that wording ("for each" ;-) ) would belong just above the 1st foreach().

  5. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,91 @@ public function onDependencyRemoval(array $dependencies) {
    +        $names = $is_entity_dependency ? array_keys($removed_dependencies[$type]) : $removed_dependencies[$type];
    +        foreach ($dependencies as $name) {
    +          if (in_array($name, $names)) {
    +            if ($is_entity_dependency) {
    +              $intersect[$type][$name] = $removed_dependencies[$type][$name];
    +            }
    +            else {
    +              $intersect[$type][] = $name;
    +            }
    +          }
    +        }
    

    Ouch, I didn't expect entity deps and module deps would need different logic :-/ I suspect this could be streamlined a bit though:

    if ($is_entity_dependency) {
      // Entity dependencies use the dependecy names as keys.
      $intersect[$type] = array_intersect_key($removed_dependencies[$type], array_flip($dependencies));
    }
    else {
      // Module and theme dependencies are indexed arrays of dependency names.
      $intersect[$type] = array_values(array_intersect( $removed_dependencies[$type], $dependencies));
    }
    

    ?

    [edit: and then we can inline the $is_entity_dependency var]

  6. +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
    @@ -95,4 +95,29 @@ public function getThirdPartySetting($module, $key, $default = NULL);
    +   * Informs the plugin that some configuration it might depend on will be
    +   * deleted.
    

    We can now remove the "might". With the logic we added, it's really "configuration it depends on" (might put us back under 80 chars ?)

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.32 KB
13 KB

OK. Fixed all points, I think. Changed the test to deal also with third party stuff.

Status: Needs review » Needs work

The last submitted patch, 49: 2562107-49.patch, failed testing.

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -428,19 +436,80 @@ public function onDependencyRemoval(array $dependencies) {
    +        if ($removed = $is_entity_dependency ? array_intersect_key($removed_dependencies[$type], array_flip($dependencies)) : array_values(array_intersect($removed_dependencies[$type], $dependencies))) {
    +          $intersect[$type] = $removed;
    +        }
    

    That's now really long for a ternary :-) An actual if/else would be more readable...
    And we don't need the $is_entity_dependency var

  2. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
    @@ -97,6 +97,16 @@ public function setSetting($key, $value) {
    +  public function getThirdPartySettings($module = NULL) {
    +    if ($module) {
    +      return isset($this->thirdPartySettings[$module]) ? $this->thirdPartySettings[$module] : NULL;
    +    }
    +    return $this->thirdPartySettings;
    +  }
    
    +++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
    @@ -82,6 +82,20 @@ public function setSetting($key, $value);
    +   * Returns all third-party settings values.
    +   *
    +   * @param string|null $module
    +   *   (optional) If passed, only settings provided by that module will be
    +   *   returned. If omitted all settings will be returned. Defaults to NULL.
    +   *
    +   * @return array|null
    +   *   All third-party settings, if the module is omitted. Only settings
    +   *   provided by the module, if a module is passed. If a module has been
    +   *   passed but that module has no settings, NULL will be returned.
    +   */
    +  public function getThirdPartySettings($module = NULL);
    

    Oh, weird, we didn't have that in PluginSettingsBase already ?

    Then I'd suggest PluginSettingsInterface should extend ThirdPartySettingsInterface, for consistency, and PluginSettingsBase should implement methods it currently misses.

    The interface has no "get all settings for all modules", though only getThirdPartySettings($module)

    Our calling code in onDependencyRemoval() could do

    $component['third_party_settings'] = [];
    foreach ($renderer->getThirdPartyProviders() as $module) {
      $component['third_party_settings'][$module] = $renderer->getThirdPartySettings($module);
    }
    

    A bit tedious, but sticking with the generic ThirdPartySettingsInterface is best IMO.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
20.82 KB
5.59 KB

Here we go.

But starting with #49 I'm getting that test failure and I cannot explain it.

Status: Needs review » Needs work

The last submitted patch, 52: 2562107-52.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
20.82 KB
1 KB

OK, got it. The configurables have internally third_party_settings while PluginSettings have thirdPartySettings.

Status: Needs review » Needs work

The last submitted patch, 54: entitydisplaybase-2562107-54.patch, failed testing.

yched’s picture

Interdiff #54 looks wrong. PluginSettingsBase does use thirdPartySettings for its internal property name, but it gets populated by the 'third_party_settings' entry in the widget/formatter configuration array. See FormatterBase::__construct() and FormatterPluginManager::createInstance(), and same on the Widget side.

So the entry we need to write in the config array for the component is 'third_party_settings' (see also core.entity_view_display.*.*.*: schema in core.entity.schema.yml)

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.02 KB
3.2 KB

PluginSettingsBase does use thirdPartySettings for its internal property name, but it gets populated by the 'third_party_settings' entry ...

Yes, I found that before. It was \Drupal\field_ui\Tests\ManageDisplayTest that needed a change. In HEAD, testFormatterUI() expected that a formatter with 3rd-party settings is disabled when the provider is uninstalled. But that was a wrong expectation. In reality the whole Display was removed. But because EntityViewDisplayEditForm::getEntityDisplay() is using entity_get_display(), that recreates the whole entity display creating the false illusion that the formatter has been disabled. That piece from the test was wrong.

claudiu.cristea’s picture

The last submitted patch, 57: entitydisplaybase-2562107-57.patch, failed testing.

The last submitted patch, 57: entitydisplaybase-2562107-57.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: entitydisplaybase-2562107-58.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
22.92 KB
1.77 KB

The last submitted patch, 58: entitydisplaybase-2562107-58.patch, failed testing.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good !
Thanks a ton for sticking with this @claudiu.cristea :-)

The last submitted patch, 49: 2562107-49.patch, failed testing.

The last submitted patch, 52: 2562107-52.patch, failed testing.

The last submitted patch, 54: entitydisplaybase-2562107-54.patch, failed testing.

jibran’s picture

Assigned: yched » alexpott

RTBC +1. It is related to config dependencies so I think it make sense to let @alexpott review it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 62: entitydisplaybase-2562107-62.patch, failed testing.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.16 KB
22.92 KB

That was an easy fix.

The last submitted patch, 62: entitydisplaybase-2562107-62.patch, failed testing.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
23.16 KB
1.64 KB

Thank you, @jibran. If you touched that I saw that we're implementing custom assertions there. Let's add then custom messages that are more descriptive :)

Please re-RTBC if OK.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #64.

alexpott’s picture

Issue tags: -rc target +rc deadline
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. This is looking good - it is nice that plugins can respond to dependency removal - can we add a followup (if it does not exist) to generalise this code.
  2. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -132,6 +139,7 @@ public function __construct(array $values, $entity_type) {
    +    $this->logger = \Drupal::logger('system');
    

    I don't like the idea of constructing the logger service every time we create an entity display. And for it only to be used on dependency removal. Let's just add a getLogger() helper that does a return \Drupal::logger('system');

  3. +++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
    @@ -470,4 +475,172 @@ public function testGetDisplayModeOptions() {
    +  protected function assertDependency($type, $key, EntityDisplayInterface $display) {
    ...
    +  protected function assertNoDependency($type, $key, EntityDisplayInterface $display) {
    ...
    +  protected function assertDependencyHelper($assertion, $type, $key, EntityDisplayInterface $display) {
    

    Let's make all of these return a bool that this TRUE on success and FALSE on fail.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.19 KB
4.14 KB

Implemented #76. While this has been already RTBC-ed in #64, I keep this assigned to @alexpott for committing, unless there are other reviews.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good (and big +1 on not creating the logger each time, I should've spotted that)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 77: 2562107-77.patch, failed testing.

The last submitted patch, 77: 2562107-77.patch, failed testing.

claudiu.cristea queued 77: 2562107-77.patch for re-testing.

claudiu.cristea’s picture

That test passes locally. I think is the first inconsistent test that I see on the new bot.

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

OK. In the bot test log, this message has been logged:

18:15:34 Segmentation fault (core dumped)
18:15:34 FATAL Drupal\user\Tests\Migrate\MigrateUserProfileEntityDisplayTest: test runner returned a non-zero error code (139).

But on my local machine the test passes. Out of ideas.

claudiu.cristea’s picture

EDIT: Duplicate. Clicked twice.

Status: Needs review » Needs work

The last submitted patch, 77: 2562107-77.patch, failed testing.

claudiu.cristea’s picture

I tested the patch from #77 on:

- MacBook Pro (El Capitan): PHP 5.5.29 (with Zend OPcache v7.0.6-dev)
- Ubuntu (14.04.3 LTS): PHP 5.5.9-1ubuntu4.13 (with Zend OPcache v7.0.3)

On both machines the tests are completing without errors. I tried on both from the command line & UI.

yched’s picture

Status: Needs work » Reviewed & tested by the community

MigrateUserProfileEntityDisplayTest passes locally for me as well. The test summary in #77 shows mixed results, but the two most recent ones are green.

Temptatively setting back to RTBC ?

claudiu.cristea’s picture

The 2 green are on PHP 5.5 and 5.6

claudiu.cristea’s picture

I opened a ticket for the test inconsistency from #77. Maybe that deserves an investigation #2579543: Random segfault in MigrateUserProfileEntityDisplayTest.

lokapujya’s picture

If this gets rolled again:

+++ b/core/modules/field_ui/src/Tests/EntityDisplayTest.php
@@ -470,4 +475,181 @@ public function testGetDisplayModeOptions() {
+    // The config object should not depend on none of the two $roles.

s/none/any

Also, I think there might be a change regarding third party settings that is worth mentioning in the issue summary?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d86856a and pushed to 8.0.x. Thanks!

Can someone open a followup to generalise the behaviour of asking plugins about removing dependencies? It is likely that there will be other use-cases.

  • alexpott committed d86856a on 8.0.x
    Issue #2562107 by claudiu.cristea, jibran, yched, Berdir:...
claudiu.cristea’s picture

claudiu.cristea’s picture

Status: Fixed » Closed (fixed)

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