Problem/Motivation

Discovered in #2796173-160: Add experimental Field Layout module to allow entity view/form modes to switch between layouts

Steps to reproduce:

  1. Add a comment field to any entity, and set it to be displayed with the default formatter.
  2. Add a third party setting to the comment's entity display (and the target entity's entity display?).
  3. Uninstall the module providing the third party setting.

Expected:
The entity with the comment field is unaffected.

Actual:
The comment field is hidden on the entity.

Proposed resolution

\Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() tracks which dependencies were "fixed" by \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval()

It currently does not update the list of dependencies when one is fixed, continuing to pass the original list to subsequent entities.
If an entity has a dependent that was marked for deletion and has since been fixed, in HEAD it will incorrectly be told it is still up for deletion.
In the case of entity displays, this could lead to the incorrect removal of field components, causing data loss.

Remaining tasks

Update the list of dependencies being passed to onDependencyRemoval() with the information already gathered

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
5.34 KB

@effulgentsia asked this be filed as a critical, but I'm not sure it is since it seems to only affect comment fields, and only when dealing with third party settings.

tim.plunkett’s picture

Relevant code for this:

\Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval()
\Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::calculateDependencies()

effulgentsia’s picture

Priority: Major » Critical

From https://www.drupal.org/core/issue-priority#critical-bug:

Cause loss/corruption of stored data. (Lost user input, e.g. a failed form submission, is not the same thing as data loss and in most cases is major).

What's surfaced here is not loss of form submission data. It's full-fledged data loss. Configuration that has nothing to do with the module undergoing uninstallation gets lost with no warning in the UI about that being a possibility. All the uninstallation UI says is: "The following modules will be completely uninstalled from your site, and all data from these modules will be lost!", but Comment module is not what's being uninstalled.

Therefore, raising to Critical, but not tagging as triaged until it is.

Status: Needs review » Needs work

The last submitted patch, 2: 2843074-thirdparty-2.patch, failed testing.

andypost’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDisplayBaseTest.php
@@ -54,4 +58,97 @@ public function testPreSave() {
+      'content' => [
+        'test_field' => ['type' => 'comment_default', 'region' => 'content', 'settings' => ['view_mode' => 'default'], 'label' => 'hidden', 'third_party_settings' => []],
+      ],
+      'third_party_settings' => [
+        'entity_test_third_party' => [
+          'key' => 'value',
+        ],
+      ],
...
+    $expected_component = [
+      'type' => 'comment_default',
+      'region' => 'content',
+      'settings' => ['view_mode' => 'default'],
+      'label' => 'hidden',
+      'third_party_settings' => [],
+    ];
+    $entity_display->getComponent('test_field');
+    $this->assertEquals($expected_component, $entity_display->getComponent('test_field'));

I think this TPS should live in 'test_field' component, but you provide them in display
Maybe I'm missing the point but probably better to test both kinds of TPS (display and field) once on it

effulgentsia’s picture

From #2796173-164: Add experimental Field Layout module to allow entity view/form modes to switch between layouts:

Seems at this time to be isolated to comment fields, specifically when using CommentDefaultFormatter.

Well, that's the only comment field formatter in core, right? But wouldn't another formatter that also has a dependency on an EVD entity run into this too? Core's comment widget isn't running into this problem, because it doesn't have a dependency on an EFD entity, but I'm guessing a widget with such a dependency would also run into this bug.

I thought I could reproduce this for something other than Comments by making the Tags field use the "Rendered Entity" formatter. But for some reason, doing that does not add a config dependency on the corresponding EVD entity. I wonder if that's a bug though with that formatter, since wouldn't the lack of that dependency cause config deployment problems?

effulgentsia’s picture

I wonder if that's a bug though with that formatter, since wouldn't the lack of that dependency cause config deployment problems?

Hm, maybe not. Because it is possible to invoke viewMultiple() and pass in a view mode for which there's no corresponding EVD. I.e., you can use the "Rendered Entity" formatter, and specify the "Full" view mode, even if you're not using a custom display for that view mode (just allowing it to fallback to default instead).

So, I wonder if the bug is really that CommentDefaultFormatter::calculateDependencies() is adding the dependency on the EVD in the first place. For example, if you do check the box to use a custom display for the Full view mode for comments, then configure your Article node type to display comments in the Full view mode, you end up with a dependency on that comment.comment.full EVD. So then if you go back to the Manage Display UI for comments, and uncheck the box to use a custom display for it, it results in the Article EVD auto-updating to set the comments to hidden. But that doesn't make sense: you didn't say you want to hide the comments, or that you no longer want to display them with the "full" view mode. All you did was decide that for the moment, you don't need to customize the display of the full view mode separately from the display of the default view mode.

The real dependency should be on the view mode, not the display.

effulgentsia’s picture

#8 deals with EVD specifically, because that's what got surfaced in #2796173: Add experimental Field Layout module to allow entity view/form modes to switch between layouts.

But I wonder if there's also a more general bug for any config entity A that has a dependency on some other config entity B, where B has third party settings. And where the uninstallation of the module with B's third party settings ends up causing unnecessary removal of data from A.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
2.02 KB

This behavior has been introduced by me in #2562107: EntityDisplayBase should react on removal of its components dependencies. I agree that when resolving the dependencies the 3rd party settings should be used. But when disabling the component, only the plugin settings dependencies should be taken into account.

This is only a PoC patch.

To be discussed: Should we add the new methods, PluginSettingsBase::calculatePluginDependencies() and ::calculateThirdPartyDependencies() to the PluginSettingsInterface or we add a new interface? In the first case, PluginSettingsInterface is deprecated. How do we deal with this?

One thing I'm sure: we need to be able to calculate the main plugin dependencies and separate the 3rd party dependencies. They are not equal citizens.

claudiu.cristea’s picture

FileSize
706 bytes
7.34 KB
xjm’s picture

@catch, @cilefen, @alexpott, @effulgentsia, @Cottser, and I discussed this issue today and agreed that this is a critical issue because of the dat integrity/data loss risk. Even if it turns out to be a problem with a particular implementation, the fact that apparently valid code could cause data loss is critical.

@alexpott also said he would try to investigate this more. Tagging for subsystem maintainer review.

Thanks @effulgentsia, @tim.plunkett, @andypost, and @claudiu.cristea!

claudiu.cristea’s picture

@andypost is right in #6, the 3rd party settings should be added to field component Then, the entity display will aggregate the components dependencies and only the dependency will be added to the entity display on save.

tim.plunkett’s picture

(all of the below was written before comment #10 and on were posted)

The $dependencies param passed to \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval() is documented as such:

An array of dependencies that will be deleted keyed by dependency type.

However, this is not true.

\Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() loops through and marks certain dependencies as "fixed", but does not update the array being passed to each onDependencyRemoval() method.
Therefore EntityDisplayBase is being passed incorrect information, and is acting on it.

If we remove the "fixed" dependents from the array of dependencies, everything works as expected.

The display vs view mode point from #8 is a separate question, and possibly it's own normal bug in CommentDefaultFormatter.

Interdiff is against #2

tim.plunkett’s picture

@claudiu.cristea and I discussed this on IRC, and the conclusion was

claudiu_cristea: timplunkett: in this case my patch is only hiding the problem but is not fixing it

The last submitted patch, 10: 2843074-10.patch, failed testing.

The last submitted patch, 11: 2843074-11.patch, failed testing.

alexpott’s picture

Nice work @tim.plunkett - this is the area where I feared the issue might lie. The fix looks sensible I'll review more tomorrow.

alexpott’s picture

The fix looks great. Given that we're manipulating $original_dependencies we can simplify looking for the unchanged dependencies - ie the ones that are neither fixed nor deleted. This occurs because sometimes fixing a dependency results in the dependency chain changing.

I think we should avoid iterating constantly over the $original_dependencies inside the dependency fixing loop - we can just store a map before we start.

@tim.plunkett excellent work on the tests it is nice to see a patch that makes changes one method - \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() - have such extensive coverage.

alexpott’s picture

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

My changes in #19 are largely cosmetic and just moving things around. As the subsystem maintainer I'm probably best placed to rtbc this. The patch has excellent test coverage. The changes make sense and are minimal and should not have a performance impact on the highly iterative logic.

tim.plunkett’s picture

I had been messing around with a couple different iterations (hah!) like that, but tried to keep the changes minimal.
However the interdiff makes the resulting code MUCH clearer IMO, and it's worth the extra changes.

tim.plunkett’s picture

Title: Uninstalling a third-party settings provider that affects comment fields hides the field on all entity displays » Stale dependencies passed to onDependencyRemoval() results in data loss on uninstallation
Component: entity system » configuration system
Issue summary: View changes

The current issue title describes one problem this bug could cause, came up with a better one with @xjm

claudiu.cristea’s picture

Running also a 8.2.x test. I think it should be committed in 8.2.x as well.

xjm’s picture

Title: Stale dependencies passed to onDependencyRemoval() results in data loss on uninstallation » Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation
Version: 8.3.x-dev » 8.2.x-dev

I scanned the patch and nothing looked patch-release-sensitive on a quick scan. Since this can cause data loss, backporting would indeed be beneficial. I also automatically thought about whether we would need an upgrade path, but of course any data already lost from previous uninstallations is already gone. As far as I understand it wouldn't change the structure of existing valid config.

So setting this for backport to 8.2.x, unless anyone can think of a potential disruption I missed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2843074-19.patch, failed testing.

alexpott’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: +needs backport to 8.2.x

Unfortunately #19 doesn't apply to 8.2.x so we'll have to backport it once we've landed this in 8.3.x. #19 just failed on 8.3.x for a known random fail.

xjm’s picture

The test-only patch is in #2. The name and comment don't indicate this but the patch itself clearly is mostly the same as the tests in the latest patch, and the demonstrated coverage is here: https://www.drupal.org/pift-ci-job/571582

denutkarsh’s picture

Lets test this patch for drupal 8.2.x.

tim.plunkett’s picture

8.2.x patch looks great, thanks @denutkarsh

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 2843074-29.patch, failed testing.

alexpott’s picture

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

Re-uploading the 8.3.x patch - since that is rtbc. Let's get that in first before working on 8.2.x.

xjm’s picture

I wanted to refer to the handbook docs to review this, but https://www.drupal.org/docs/8/api/configuration-api/configuration-entity... is still not complete, nor does that handbook include any third-party settings documentation other than indirect documentation for their schemata on https://www.drupal.org/docs/8/api/configuration-api/configuration-schema.... Oops. (I tried to locate issues for this missing documentation, but could not.) The change records I found for third-party settings also don't contain much.

xjm’s picture

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... is probably the best source for docs on dependency management in general, but I still haven't found more than passing references for third-party settings.

xjm’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -305,6 +305,13 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
+    // Create a map of UUIDs to $original_dependencies key so that we can remove
+    // fixed dependencies.

@@ -340,8 +347,9 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
+          // Remove the fixed dependency from the list of original dependencies.
+          unset($original_dependencies[$uuid_map[$dependent->uuid()]]);

Does "fixed" dependencies here mean "repaired" or "enforced"?

alexpott’s picture

It means - updated so that they are no longer in the dependency chain of the thing (module, config, theme, content) that is being removed.

xjm’s picture

  • xjm committed b7ff214 on 8.3.x
    Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh,...
xjm’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -needs backport to 8.2.x

Alright, I was finally able to understand how this small patch is fixing what it's fixing based on the explanation in the summary. The overall docs for this method and the related ones are very confusing (and of course it's a complicated process in itself), but we can address that in the followup issues.

I also tested manually and confirmed it resolves the behavior described in #2796173-160: Add experimental Field Layout module to allow entity view/form modes to switch between layouts.

Committed and pushed to 8.3.x. Thanks! Setting to 8.2.x for backport.

alexpott’s picture

Status: Patch (to be ported) » Needs review
FileSize
14.53 KB

Backported the fix and the test coverage

Status: Needs review » Needs work

The last submitted patch, 40: 2843074-8.2-40.patch, failed testing.

denutkarsh’s picture

Status: Needs work » Needs review
FileSize
14.63 KB
912 bytes

Nit, I think we can break this array in

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDisplayBaseTest.php
@@ -0,0 +1,115 @@
+        'test_field' => ['type' => 'comment_default', 'settings' => ['view_mode' => 'default'], 'label' => 'hidden', 'third_party_settings' => []],

I have attached the new patch

Status: Needs review » Needs work

The last submitted patch, 42: 2843074-8.2-42.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
329 bytes
14.13 KB

My pesky .htaccess customisation got in the way of #40. I think we should keep the patch as similar as possible to what was committed in #32 so not including #42.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good... like the 8.3.x one :)

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.2.x. Thanks!

@denutkarsh, you can file separate issues for additional improvements if needed, since we want to focus here on fixing the critical and keeping 8.3.x and 8.2.x in sync. Thanks for helping out with the backport!

  • xjm committed 5fc68ed on 8.2.x
    Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh,...

  • xjm committed b7ff214 on 8.4.x
    Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh,...

  • xjm committed b7ff214 on 8.4.x
    Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh,...

Status: Fixed » Closed (fixed)

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

denutkarsh’s picture