Problem/Motivation
In some cases, we don't want to delete the whole dependency tree if one configuration entity is being removed.
The Drupal core example are view and form displays.
Assume that you have a module that provides a field (as default configuration, or by providing the field type plugin). That field is added to node articles, together with 15 other fields. The view and form display modes are configured to display that field with a specific formatter/widget.
Right now, if you uninstall that module, it will forcefully remove all your display configurations that contain that field. But that's not what you want, you still want to display all the other fields with their configuration.
Another example is the Search API module, which is ported to 8.x here: https://drupal.org/sandbox/daeron/2091893
It has search servers/backends, for example database or solr. Then you have search indexes which use a specific server, have data sources with fields (can be configurable fields and then need to depend on them) and processors, which are plugins, so we need to depend on all those things to make sure that it's installed/synced in the right order. This is happening here: https://drupal.org/node/2257081.
But if you remove the module that provides the server, we want to update the index to disable itself and remove the dependency on the server, and if you remove a plugin, we want to remove just that, and not delete the whole index, and views that depend on it and so on.
Proposed resolution
Add ConfigEntityInterface::onDependencyRemoval()
to allow configuration entities to respond if their any of dependencies is going to be removed (ie. if an entity is going to be deleted or a module uninstalled). This method allows configuration entities to remove dependencies instead of being deleted themselves. When a module is being uninstalled configuration entities can use this method to avoid being unnecessarily deleted. Implementations should save the entity if dependencies have been successfully removed.
For example, entity displays remove references to widgets and formatters if the plugin that supplies them depends on a module that is being uninstalled.
Remaining tasks
Review patch
User interface changes
The message about deleting configuration entities as a result of module uninstall is changed. As a result of this patch we don't know if they will be deleted or just updated so we list affected entities.
API changes
Add ConfigEntityInterface::onDependencyRemoval()
Original discussion of a resolution
Discussed with @alexpott, what we should do is introduce an API that lets us ask a config entity if can continue to exist without that dependency and heal itself. If so, we let it do it, otherwise we delete it.
Deletion happens top down, if you have a search server, a search index that depends on that server and a view against that index, we first need to ask the index what we should do if the server is removed, and let it self-heal, otherwise we'd delete the view because the view can not self-heal itself from the index being removed before we'd learn that we don't have to delete the index.
Not sure if we need two separate methods to check if it can remove the dependency or if we just have on that does it and returns TRUE or FALSE?
We will probably also at least update the uninstall page to explain that not all config would be deleted? But it would be better if we'd already know then what will be deleted and what not, would be very confusing/fragile/scary to silently rely on this. But then we certainly need two methods...
Comment | File | Size | Author |
---|---|---|---|
#38 | 2260457.38.patch | 24.07 KB | alexpott |
#34 | 2260457.34.patch | 23.88 KB | alexpott |
#34 | 32-34-interdiff.txt | 1.14 KB | alexpott |
#32 | 2260457.32.patch | 23.88 KB | alexpott |
#32 | 30-32-interdiff.txt | 6.34 KB | alexpott |
Comments
Comment #1
BerdirComment #2
BerdirComment #3
alexpottI think this is a critical issue since unexpectedly and unnecessarily deleting configuration entities is going to cause significant headaches. But since this is not changing data structures and is an API addition I feel this should only be a beta target.
Comment #4
catchSo self-heal means in the case of a display mode we'd remove the field configuration from it rather than deleting? If so completely agreed on this.
Comment #5
yched CreditAttribution: yched commentedOoh yes. Deleting the whole EntityDisplay if one of its field disappears is pretty serious - do we really do that currently ?
Comment #6
BerdirYeah, that's exactly what we do.
Comment #7
catchRe-titling and the current behaviour means we should probably treat this as a bug.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedfirst cut patch, still much to do.
worked on this with alexpott at pre-Drupalcon code sprint.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #12
alexpottLet's add a test and fix some of the failures.
Comment #14
alexpottAnd now fixing the exceptions and updating UI text. Unfortunately I can't see a sensible way of informing the user which entities are going to be "healed" and which are going to be deleted. We could stored what we've done on the ConfigManager and then use a drupal_set_message in ModulesUninstallConfirmForm::submitForm() but this message will quickly become unwieldy if there are lots of entities involved.
The only example I can think of where we need to do this is core is entity displays but at the moment this can never happen since field instances have to be deleted before a module can be installed that would removed them :). This also means that the implementation in EntityTestBase is impossible to test. Potentially we could fix a view that depends on a field - but I'm not sure this is the correct behaviour since this could result in information disclosure depending on what the view is being used for.
Comment #16
alexpottI thought there was a test for that text somewhere :)
Comment #17
alexpottWe also can remove components from entity display if they (the component) have a dependency on the module that is being uninstalled.
Comment #18
yched CreditAttribution: yched commentedIn D7 and D8 so far, we fall back to the "default widget/formatter for the field type" when the configured widget or formatter is unavailable.
If we add strict dependencies, then we should remove that fallback mechanism in Widget/FormatterPluginManager::getInstance().
Comment #19
mtiftThis won't ever get called because there is no
$changed = TRUE;
Comment #20
BerdirSomething went wrong here ;)
This added a new method, kept the descripion of the one below and then added a new one for the existing method.
Comment #21
xjmNotes on this issue from DrupalCon Austin:
https://docs.google.com/a/acquia.com/document/d/18R3-k0saR_RCZBYnggM0B8b...
Action items:
https://docs.google.com/a/acquia.com/document/d/18R3-k0saR_RCZBYnggM0B8b...
Comment #22
alexpottAdded extensive testing of falling back to default widgets and formatters when a module that provides one that's in use is uninstalled.
Changed method name from
preDeleteFixDependencies()
toonDependencyRemoval
after discussing at Austin.Comment #25
alexpottRerolled.
Comment #27
alexpottUpdated patch for field_config -> field_storage_config rename.
Comment #28
alexpottRerolled
Comment #29
BerdirCan we explain the reason for array_reverse() in the comment?
Can we explain a bit more here what config entities are supposed to do in here? (remove references + save itself)
nitpick: see should be below the @params I think, separated with an empty line.
In case it already is the default, this would then not remove the dependency and we would still be removed.
I think that is working correctly with this logic, maybe add a comment to explain this?
Interesting that we can uninstall node twice ;) It's really "uninstallConfig(), so it's ok.
Would it be easier to understand if we would make this two test methods?
Double ' ;)
I also find those "Plugin implementation of the 'X' formatter." weird and useless, but it's everywhere else, so fine ;)
Do we really need a @see when we're extending from it? I think that's only useful for things which are not already linked explicitly through code?
Hm, so we obviously don't know at this point. Unfortunate, but I'm sure you would have found a way if there would be one ;)
"This might or might not be deleted. Please confirm." ;)
The issue summary is still my original one, and it describes that we hoped to implement, a system where we can ask a config entity if it can continue to live, so that we could explicitly separate those here. We should update that.
Comment #30
alexpott$config_manager->uninstall('module', 'node');
twice in the same test since that is precisely the code under test.Will update the issue summary soon.
Comment #31
dawehnerOne thing I realized while reading the IS is that it would be nice to have that feature on the actual API level.
So you remove a field instance (via API or UI), it should also adapt the configuration entities much like it now should happen on uninstall.
As discussed with berdir this could be even leveraged to build also a nice UI for the delete confirm forms, but for sure
this is NOT part of the issue. (adapted the issue title to be sure about that).
Once we do things on the actual API level, we would not longer have to do that code in uninstall but it would just bubble up when you remove the entity itself.
I don't really think we should couple the interface to those three dependency types. As berdir said, we hardcode that in multiple places probably already, though there is no reason to hardcode it here. getDependencies() itself also already returns all of them, so the onDependencyRemoval should not try to special case things.
It would be great to add a todo somewhere, maybe here, to explain that this doesn't happen if an entity is deleted but just if the module is uninstalled atm.
Just a general note, in case we just can depend on config entities we should also rename the variable so that this is clear as well.
I assume this @param is a leftover or you were to lazy to implement it.
Let's use FieldInstanceConfigInterface instead
Is there a reason why removeComponent also doesn't drop it from $this->hidden?
Comment #32
alexpottI updated the issue summary too.
Comment #33
swentel CreditAttribution: swentel commentednitpick, could probably use a comma after uninstalled, got confused when reading this
Comment #34
alexpottSwitched order of the sentence around.
Comment #35
dawehnerSeems legit.
Comment #38
alexpottRerolled... minor conflict in EntityDisplayBase due to #2333501: Implement ThirdPartySettingsInterface in EntityView|FormDisplay - I've reordered use statements to be alphabetical. Setting back to rtbc as per #35
Comment #39
catchCommitted/pushed to 8.0.x, thanks!
Comment #42
colanShould this prevent #3160900: Entire view is deleted on module uninstallation after depending display is deleted? It doesn't appear to.