Problem/Motivation
Discovered in #2796173-160: Add experimental Field Layout module to allow entity view/form modes to switch between layouts
Steps to reproduce:
- Add a comment field to any entity, and set it to be displayed with the default formatter.
- Add a third party setting to the comment's entity display (and the target entity's entity display?).
- 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
Comment | File | Size | Author |
---|---|---|---|
#44 | 2843074-8.2-44.patch | 14.13 KB | alexpott |
#44 | 40-44-interdiff.txt | 329 bytes | alexpott |
#42 | interdiff.txt | 912 bytes | denutkarsh |
#42 | 2843074-8.2-42.patch | 14.63 KB | denutkarsh |
#40 | 2843074-8.2-40.patch | 14.53 KB | alexpott |
Comments
Comment #2
tim.plunkett@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.
Comment #3
tim.plunkettRelevant code for this:
\Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval()
\Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::calculateDependencies()
Comment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom https://www.drupal.org/core/issue-priority#critical-bug:
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.
Comment #6
andypostI 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
Comment #7
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom #2796173-164: Add experimental Field Layout module to allow entity view/form modes to switch between layouts:
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?
Comment #8
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHm, 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 thatcomment.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.
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commented#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.
Comment #10
claudiu.cristeaThis 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.
Comment #11
claudiu.cristeaComment #12
xjm@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!
Comment #13
claudiu.cristea@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.
Comment #14
tim.plunkett(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:
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
Comment #15
tim.plunkett@claudiu.cristea and I discussed this on IRC, and the conclusion was
Comment #18
alexpottNice work @tim.plunkett - this is the area where I feared the issue might lie. The fix looks sensible I'll review more tomorrow.
Comment #19
alexpottThe 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.
Comment #20
alexpottComment #21
alexpottMy 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.
Comment #22
tim.plunkettI 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.
Comment #23
tim.plunkettThe current issue title describes one problem this bug could cause, came up with a better one with @xjm
Comment #24
claudiu.cristeaRunning also a 8.2.x test. I think it should be committed in 8.2.x as well.
Comment #25
xjmI 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.
Comment #27
alexpottUnfortunately #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.
Comment #28
xjmThe 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
Comment #29
denutkarsh CreditAttribution: denutkarsh as a volunteer commentedLets test this patch for drupal 8.2.x.
Comment #30
tim.plunkett8.2.x patch looks great, thanks @denutkarsh
Comment #32
alexpottRe-uploading the 8.3.x patch - since that is rtbc. Let's get that in first before working on 8.2.x.
Comment #33
xjmI 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.
Comment #34
xjmhttps://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.
Comment #35
xjmDoes "fixed" dependencies here mean "repaired" or "enforced"?
Comment #36
alexpottIt means - updated so that they are no longer in the dependency chain of the thing (module, config, theme, content) that is being removed.
Comment #37
xjmI posted a couple docs followups:
Comment #39
xjmAlright, 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.
Comment #40
alexpottBackported the fix and the test coverage
Comment #42
denutkarsh CreditAttribution: denutkarsh as a volunteer commentedNit, I think we can break this array in
I have attached the new patch
Comment #44
alexpottMy 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.
Comment #45
claudiu.cristeaLooks good... like the 8.3.x one :)
Comment #46
xjmCommitted 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!
Comment #51
denutkarsh CreditAttribution: denutkarsh as a volunteer and at Google Code-In, Google Summer of Code commented