Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 inThis is already in place. Right now, by implementingEntityDisplayBase::calculateDependencies()
that iterates through the display components to collect dependencies from widgets/formatters.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 ownonDependencyRemoval()
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
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. |
Comment | File | Size | Author |
---|---|---|---|
#77 | interdiff.txt | 4.14 KB | claudiu.cristea |
#77 | 2562107-77.patch | 23.19 KB | claudiu.cristea |
Comments
Comment #2
claudiu.cristeaComment #3
claudiu.cristeaI think that this is a bug while it prevents us to react on a dependency change.
Comment #4
jibranNow it seems like a bug report ;-)
Comment #5
andypostlike a challenge, bug that dependencies are not calculated
Comment #6
claudiu.cristeaUpdated 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:
And in the attached test I did that:
onDependencyRemoval()
returnsTRUE
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.Comment #7
claudiu.cristeaComment #9
claudiu.cristeaUpdate title to reflect the IS.
Comment #11
claudiu.cristeaComment #13
claudiu.cristeaThank you @Berdir for pointing me in the right direction with the dependency stuff on this issue.
Added beta evaluation.
Comment #14
claudiu.cristeaSmall tweaks.
Comment #15
BerdirThat 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?
Comment #16
claudiu.cristea@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.
Comment #18
claudiu.cristeaOk, 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.
Comment #19
claudiu.cristeaFixing the IS to reflect the idea from #15.
Comment #20
claudiu.cristeaNow I'm asking myself if this should be an
if ()
or anelseif ()
? I tried withif ()
but in that case the test fails. And the test scenario seems correct.Comment #21
claudiu.cristeaSmall docs fixes.
Comment #22
claudiu.cristeaHere's the reply to #20. In fact I was confused about the array_diff term used. So it's an intersect, not a diff.
Comment #25
claudiu.cristeaComment #26
claudiu.cristeaSome 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.
Comment #27
BerdirI don't think this method should be on the plugins, there is no need for them to customize this?
Comment #28
claudiu.cristeaI knew it :)
Comment #30
claudiu.cristeaTagging.
Comment #32
lokapujyaComment #33
claudiu.cristeaRerolling, tagging. I'm assigning this to @yched for review as we've discussed in Barcelona.
Comment #34
nlisgo CreditAttribution: nlisgo commentedJust read through the patch manually for any errors. This is what I found.
should be just array without the square brackets.
Comment #35
claudiu.cristea@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.
Comment #36
nlisgo CreditAttribution: nlisgo commentedJust 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.
Comment #37
yched CreditAttribution: yched commentedOne 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]
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]
looks like the $component_changed var is actually not used ?
Is that something we typically do in other similar cases ? Should we ask @alexpott for feedback on that ?
This is called only once, so maybe inlining it could save the burden of having to write a phpdoc ? ;-)
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 :-)
minor : s/react on/react to
Comment #38
yched CreditAttribution: yched commentedAlso, 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 :
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) :
PluginSettingsBase::onDependencyRemoval() should do the same for the third_party_settings of the Formatter/Widget
Comment #39
yched CreditAttribution: yched commentedOtherwise, forgot to add : awesome work, @claudiu.cristea :-)
Comment #40
yched CreditAttribution: yched commentedOne 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" ?
Comment #41
yched CreditAttribution: yched commentedDiscussed #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) :
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.
Comment #42
claudiu.cristea@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:
Comment #43
BerdirThat's not how this works :)
Blocking majors doesn't make an issue a critical it just makes it a major too.
Comment #44
claudiu.cristea#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.
Comment #45
claudiu.cristeaGreen. Passing for review.
Comment #46
claudiu.cristeaDocs fixes.
Comment #47
claudiu.cristeaOuch, reassigning.
Comment #48
yched CreditAttribution: yched commentedThanks @claudiu.cristea - last adjustements / nitpicks :-)
We should also update $component['third_party_settings'] now ?
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)
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 :-)
That comment with that wording ("for each" ;-) ) would belong just above the 1st foreach().
Ouch, I didn't expect entity deps and module deps would need different logic :-/ I suspect this could be streamlined a bit though:
?
[edit: and then we can inline the $is_entity_dependency var]
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 ?)
Comment #49
claudiu.cristeaOK. Fixed all points, I think. Changed the test to deal also with third party stuff.
Comment #51
yched CreditAttribution: yched commentedThat'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
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
A bit tedious, but sticking with the generic ThirdPartySettingsInterface is best IMO.
Comment #52
claudiu.cristeaHere we go.
But starting with #49 I'm getting that test failure and I cannot explain it.
Comment #54
claudiu.cristeaOK, got it. The configurables have internally
third_party_settings
while PluginSettings havethirdPartySettings
.Comment #56
yched CreditAttribution: yched commentedInterdiff #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)
Comment #57
claudiu.cristeaYes, 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 becauseEntityViewDisplayEditForm::getEntityDisplay()
is usingentity_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.Comment #58
claudiu.cristeaTypos.
Comment #62
claudiu.cristea#57 failed because of #2577569: HEAD broken! system_update_8009() twice.
Comment #64
yched CreditAttribution: yched commentedLooks good !
Thanks a ton for sticking with this @claudiu.cristea :-)
Comment #68
jibranRTBC +1. It is related to config dependencies so I think it make sense to let @alexpott review it.
Comment #71
jibranThat was an easy fix.
Comment #73
claudiu.cristeaThank 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.
Comment #74
claudiu.cristeaBack to RTBC as per #64.
Comment #75
alexpottComment #76
alexpottI 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');
Let's make all of these return a bool that this TRUE on success and FALSE on fail.
Comment #77
claudiu.cristeaImplemented #76. While this has been already RTBC-ed in #64, I keep this assigned to @alexpott for committing, unless there are other reviews.
Comment #78
yched CreditAttribution: yched commentedLooks good (and big +1 on not creating the logger each time, I should've spotted that)
Comment #82
claudiu.cristeaThat test passes locally. I think is the first inconsistent test that I see on the new bot.
Comment #83
claudiu.cristeaComment #84
claudiu.cristeaOK. In the bot test log, this message has been logged:
But on my local machine the test passes. Out of ideas.
Comment #85
claudiu.cristeaEDIT: Duplicate. Clicked twice.
Comment #87
claudiu.cristeaI 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.
Comment #88
yched CreditAttribution: yched commentedMigrateUserProfileEntityDisplayTest 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 ?
Comment #89
claudiu.cristeaThe 2 green are on PHP 5.5 and 5.6
Comment #90
claudiu.cristeaI opened a ticket for the test inconsistency from #77. Maybe that deserves an investigation #2579543: Random segfault in MigrateUserProfileEntityDisplayTest.
Comment #91
lokapujyaIf this gets rolled again:
s/none/any
Also, I think there might be a change regarding third party settings that is worth mentioning in the issue summary?
Comment #92
alexpottCommitted 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.
Comment #94
claudiu.cristeaHere's the followup #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed. Hope that I understood the request.
Comment #95
claudiu.cristeaHere's also the first use-case of this issue #2479487: ImageStyles can be deleted while having dependent configuration..