Problem/Motivation
Forum provides default configuration and content including fields, a node-type, a vocabulary and a taxonomy term.
Until now we were unsure about what should happen to that when you uninstall forum.
Proposed resolution
We've settled on what should happen is:
- For correct dependency management, and to unblock #2140511: Configuration file name collisions silently ignored for default configuration , all of the forum module's supplied configuration should be removed when the module is uninstalled. This means its vocabulary, its node type, and its field.
- The taxonomy field should have a dependency on the taxonomy vocabulary, ensuring that the field data must be purged first, then the field deleted, then the vocabulary deleted. If the field data has not been purged, then the module cannot be uninstalled.
- If other config entities have added the taxonomy field, they too are subject to the same rules.
- Forum nodes must be deleted before the module is uninstalled (same as #2).
- Forum taxonomy terms must be removed before the vocabulary can be deleted (same as #2 - as the 'forum_container' field will dictate this too).
- Configuration entities can have enforced dependencies. This means that we can ensure that node.type.forum will be removed when the forum module is uninstalled. We've implemented a generic solution so that all configuration entities can do this.
Remaining tasks
Review patch
User interface changes
Can't uninstall forum whilst content exists.
API changes
None
Original report by @beejeebus
over in #1776830-100: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API, we decided on the rules for what should happen when a module is uninstalled.
this patch should make forum behave that way - only remove configuration entities during uninstall if there is a dependency issue.
geez i wish pings worked, oh hai larowlan.
Comment | File | Size | Author |
---|---|---|---|
#95 | 2224581-2.95.patch | 40.86 KB | alexpott |
#94 | interdiff.txt | 4.67 KB | jhodgdon |
#91 | 2224581-2.91.patch | 40.03 KB | alexpott |
#91 | 89-91-interdiff.txt | 3.5 KB | alexpott |
#89 | 2224581-2.89.patch | 39.29 KB | alexpott |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #2
larowlanHai
Where is the patch
Comment #3
larowlanexpecting this to fail cause sure there's some tests around this like moduleenabledisabletest or somesuch
Comment #5
xtfer CreditAttribution: xtfer commentedComment #6
andypostThere's no consensus about data to to leave in DB when forum uninstalled
Suppose We just need to drop all data and config on uninstall!
Comment #7
andypostdiscussed with @alexpott
Comment #8
xjmThis is at least major.
Comment #9
xjmPer discussion with @alexpott and @larowlan, this blocks #2140511: Configuration file name collisions silently ignored for default configuration, so bumping to critical.
Comment #10
xjmAnd rescoping to the needed resolution, per #7 and #2140511: Configuration file name collisions silently ignored for default configuration.
With #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference, what will happen is that forum cannot be uninstalled while you have forum data. Then, when all your forum data is purged, you can uninstall the forum module and remove all the configuration it provides, including the forum node type and vocabulary. (Just like with field-providing modules.)
Comment #11
xjmComment #12
alexpott+1 on the new direction - if you install Forum you get the forum functionality - if you uninstall it you need to not be using the functionality it provides. Seems sane.
Comment #13
xjmFrom #2032699: Preserve taxonomy_forums field when uninstalling forum module:
Comment #14
larowlanComment #15
larowlanding ding
Comment #16
larowlanNo interdiff, clean start.
Comment #17
andypostVocabulary is populated with a new term on install so it would be good to point user to vocabulary list page from here if user has access
Comment #19
larowlan#17 great idea
Comment #20
larowlanAddress #17 and tries to sort some failing tests.
Comment #21
larowlanInterdiff for #20
Comment #22
larowlanMinor cleanup, self-reviews++
Comment #25
larowlanSo forum was the guinea pig for the whole locked/non-locked node-type persistence stuff.
If we drop that (and just kill the node-type) then we need another guinea pig.
So i nominated book and added that logic there.
Comment #26
pwolanin CreditAttribution: pwolanin commentedtypo/grammar:
I'm not clear why book is useful example for this - it's kind of a generic node type and you can make something else the default.
Reading the issue summary, I don't even understand how a "locked" node type is related to the goal of this issue.
Comment #27
xjmCan has testbot?
Comment #28
larowlandoh
Comment #30
larowlanSo the BookTest failed, which means my pushing the responsibilities to book module are wrong.
So what to do with the node-type persistence tests.
If the intent of this patch is to delete the forum node-type on uninstall, I assume that is true for all node types provided by default config?
So should we just remove the node type persistence test?
Or should I provide a new test module that adds a locked node-type and unlocks it on uninstall and use that for the tests instead?
Comment #31
catchComment #32
xjmTagging as a priority for a pre-AMS beta sprint.
Comment #33
larowlanAlexpott pointed out that forum should add thirdpartysettings for forum node type to enforce the dependency
Comment #34
alexpottI think we should remove the NodeTypePersistenceTest since this test was ensuring that when users made customisations to a node type defined in code these where moved to the database and persisted through a module uninstall. This is no longer needed now we have the configuration system and dependencies. If the node type depends on the module that creates it just add the module to the third party settings and a dependency will exist on the module. If it does not then when you uninstall that module the configuration will still be there. This is tested in Drupal\config\Tests\ConfigDependencyTest::testConfigEntityUninstall().
Patch attached removes NodeTypePersistenceTest, makes forum's node type depend on the forum module and adds testing around node type locking and forum uninstalling?
Comment #35
andypostthere's no way to delete them automatically?
please use count for entity query
Comment #37
alexpott@andypost nope we don't have automatic content entity clean up yet - this would need to be batched :)
Patch fixes tests and uses entity query's count.
Comment #38
andypostLooks good except routing and access, once we in context of vocabulary better use entity access
is not needed for count
$vocabulary->access('view')
$vocabulary->url('overview-form')
Comment #39
alexpottFixed stuff from #38
Comment #40
larowlanooh nice
+1 RTBC @alexpott++
Comment #41
alexpottFixed docs and could not see why this change requires the new testCommentFieldDelete test.
Comment #42
andypostrtbc if green
Comment #43
larowlanIt was moved here from forum module tests, see hunk below
this bit
Comment #44
larowlanie we need that test method back
Comment #45
catchIf we only want to check existence, range(0, 1) is plenty.
Same here for range(0, 1)
Not sure I like the view access check here. If you can uninstall modules but can't administer the taxonomy vocabularies, that seems like a serious edge case. If I'm trying to do that and forgot to assign myself the permission, then either an actual prompt to assign the permission, or just the 403 seems preferable?
Comment #46
alexpottFixed everything from #43 and #45 - just added the link - if it 403's so be it.
Comment #47
larowlanThanks alexpott, beat me to it
these can go now we use range right?
Comment #49
larowlanFixes fails and #47
Comment #50
xjmComment #51
larowlanBased on 42 to 45, I think this is back to rtbc
Comment #52
andypost@catch #45-3
I'd suggest to use that to prevent edge cases like custom access for vocabularies
we have /admin/content - once we point to taxonomy pages suppose it makes sense to provide a link here too
Comment #53
xjmI did a quick skim of the patch and this looks like exactly the right solution all around. Nice work!
I don't particularly care if we do the access check or not, but let's do it in both places, or neither?
Comment #54
xjmOh one other thing jumped out at me. This seems like a weird, unintuitive hack. Why isn't declaring the dependency on the module sufficient to enforce this? Seems like a bug.
Comment #55
BerdirDependency from what on what?
The problem is that config entity dependencies are re-calculated on the fly, based on the available data. So every dependency needs to be based on some data in the entity, otherwise it is already gone by the time it is imported on module install (because that does save + calculateDependencies())
Comment #56
BerdirThat said, what I said in #2235363-50: Document config dependencies at the end.
Isn't third party configuration optional by-design and should be removed automatically? If so, then this way of enforcing a dependency would not work anymore.
Comment #57
alexpottThen we need to add a provider key to node type and perhaps all configuration entities - I've been toying with this idea for a while - this will allow modules that provide configuration entities that are managed by other modules to say this view / node type / vocabulary needs me.
The other option is to make dependencies not recalculated on save - this would then introduce problems with removing existing dependencies and this problem could be very hard to solve.
Comment #58
marcus7777 CreditAttribution: marcus7777 commentedthe patch #49 will not apply.
Comment #59
mgiffordThanks @marcus7777 - here's a straight re-roll.
Comment #60
alexpottSo we can add fixed dependencies... like so.
Comment #62
larowlanlooks good, fixes fail
think this is ready
Comment #63
alexpottImproved test coverage of fixed dependencies. Basically
Drupal\config\Tests\ConfigDependencyTest
had implemented a custom version of this for ConfigTest entities so I've removed all of that and used fixed dependencies for the test.Next task is to add documentation of this feature somewhere - we don't want this to become another task for #2235363: Document config dependencies
The advantage of a fixed dependency system over adding a module key or something similar to the node type definition is that all configuration entity types can take advantage of this. One thing that kind of sucks is that after doing this when you export the forum node type config to yaml it looks like this:
Basically the fixed dependencies are duplicated in the calculated dependencies - I can't think of a way around this.
Comment #64
vijaycs85Nice way of reusing elements. would be good fit, if we rename config_dependencies_calculated as config_dependencies_base.
Comment #65
vijaycs85Comment #66
alexpottNow with added documentation and @vijaycs85's suggestion from #64
Comment #67
vijaycs85Looks good to me. Setting it as RTBC, doesn't mean "don't review anymore!" :)
As this pattern applies to almost all modules that provide plugins as part of install, would it be worth checking hook_system_info_alter for the modules of this category and inform (i.e. on site report) that the perticular module doesn't really garbage collect/ leave footprint, even after uninstall?
Further more, we might need to add some documentation for standards around this and make sure all core modules follow them. Like the issue summary has some generic rules (delete node type after delete all nodes after delete all fields after delete all field instances).
Does it mean I'm dreaming, if I say "nice to automate this for all modules"? :)
Comment #68
dawehnerI like that we secure the data of the users until the administrator decided to drop it!
Some small bits.
Should we also add entity into that documentation here?
Maybe you should check that the comment field doesn't appear there? (just to be sure and add a bit more semantic here)
I don't nitpick here, explicit
Did we considered on the longrun to be able to validate uninstall using
hook_requirements()
?Comment #69
alexpottThanks for the review @dawehner.
Comment #70
alexpott@Gábor Hojtsy, @mtift, @cliefen and I discussed this on the CMI bi-weekly meeting call.
We discussed the pros and cons of the fixed implementation. Which are:
Cons
ConfigEntityBase::calculateDependencies()
to get this functionalityPros
I think the Pros outweigh the Cons - I don't think the duplication will be that bad as I reckon fixed is only ever likely to contain a single - but can contain more if there is a use case. Calling
ConfigEntityBase::calculateDependencies()
is almost mandatory anyway for third party and plugins.Comment #71
dawehnerOne thing I really don't get is why do we not remove the fixed dependencies from the other calculated ones ...
Comment #72
alexpott@dawehner because at some point we need to merge the fixed and calculated dependencies together. We could do that in a myriad of methods in ConfigEntityBase and ConfigEntityDependency - or we could do this on save. I think doing it on save means that we keep things simple.
Comment #73
jhodgdon@alexpott - Read the Pros and Cons... Are you saying that individual entities that have this type of fixed dependencies need to do something special to make sure that calculatedependencies() gets called appropriately? I'm hoping that all they need to do is make sure the appropriate fixed dependencies get declared in the saved config. Right? So... if that is the case, I'm not sure what your points about making sure that gets called really mean?
Comment #74
catchI had the same question as #71 but I agree with alexpott's answer in #72. The duplication in the YAML seems OK given potential fragility doing it anywhere else.
#73 is a good question.
Comment #75
alexpottRe #73 if the config entity class extends
ConfigEntityBase
and does not implementcalculateDependencies()
then they get it for free. If they implementcalculateDependencies()
then have to call the parent. Every implementation in core does this atm. If a class does not extend fromConfigEntityBase
then they would have to reimplement the functionality.Comment #76
jhodgdonOK, that makes perfect sense. I think it is quite reasonable for a config entity that overrides calculateDependencies to need to look at what is happening in the method it is overriding and make sure it's taking care of it. Duh. :)
Comment #77
alexpottReroll - context conflict with #2246647: Rename PluginBag to LazyPluginCollection in ConfigEntityBase
Comment #78
dawehnerThank you for the explanation alex!
Comment #79
andypost+1 RTBC, The only question here that not covered with tests:
So in maintenance we still able to remove forum?
Comment #80
alexpottre #79 this is the same as
field_system_info_alter()
Comment #81
webchickI tried to review and commit this, but I got stuck here.
The docs explaining what the 'fixed' key is for say 'An array of fixed configuration dependencies' which unfortunately is not helpful. :) Questions this raised for me:
- Fixed as compared to what? (turns out, calculated, but this isn't clear because there's neither another array index to compare fixed to, nor docs above that section)
- As a module developer, how do I know whether the dependency I'm about to add to my module is "fixed" or not? I can add entities, modules, and themes to both areas. It's important I know when it's appropriate to use each.
We also discussed possibly renaming the "fixed" key but IMO if we write the docs we're missing, that'll clear up the name change (if necessary).
Comment #82
andypostAlso modules and themes are extensions, but what's about profiles?
Comment #83
jhodgdonI'm also a bit stuck on why it even makes sense for config to depend on install profiles. You can't actually uninstall them, right? So ... what's the point in having a dependency?
Comment #84
alexpottre #82 and #83 Profiles are a special case of module - a config entity can depend on a module - but a profile can't be uninstalled so this is just irrelevant. The point of including profiles at all in the documentation is so that people understand that if a profile provides a default configuration entity that has dependencies then the profile must depend on those same modules and themes and provide an configuration entities that are dependencies also.
Comment #85
alexpottThe patch attached tries to address #81. In IRC both @webchick and @xjm expressed concerns about the documentation and the label of
fixed
.Reading through the documentation in ConfigDependencyManager I realised that the order was a bit confusing and that way configuration entities and plugins are mixed together is very confusing. The patch attached fixes the order but does not completely resolve the issues around the documentation about how plugin and configuration entity dependencies work together as I think that is out of scope (opened #2360647: Documentation in ConfigDependencyManager conflates plugin dependencies and config dependencies making it confusing to address).
After discussion with @catch in IRC I've renamed
fixed
toenforced
based on the recommendations from @webchick and @xjm that a key would suggest a good method name. I thinkenforceDependency()
would do exactly what you would expect. Opened #2360659: Add methods to manage enforced dependencies as a followup to add methods to manage enforced dependencies.Comment #86
alexpottGot a +1 on the new name from @larowlan in IRC
Comment #88
vijaycs85config schema still has 'fixed'.
Comment #89
alexpottFixed up the tests and schema - thanks @vijaycs85
Comment #90
Gábor HojtsyI agree enforced sounds good. Not sure the docs explain it very well yet. These are the docs parts:
I am missing something to explain why a forum module dependency is incalculable there. There are examples in the docs as to how entities depend on each other (and how modules need to depend on each other if module dependencies exist in config), but not how module dependencies are calculated (at least with an example or so). One could think the module that shipped the config is one of the calculated modules, so why do you need to enforce it? Without a framework to think about it, it just hands in there. It is true that if you read between the lines of these two descriptions, you can gather that the dependencies are calculated based on the state of the configuration, so since the provider module is not otherwise within the "state of the configuration", it needs to be added like so in the enforced dependencies. This distinction is not really spelled out, in fact if we would not be introducing enforced dependencies, that dependencies are (supposed to be) calculated based on the state of config only is not explained.
Otherwise looks good.
Comment #91
alexpottThanks @Gábor Hojtsy - I've added more documentation on what configuration entity dependencies are and used the example of filter formats as to how a dependency on a module can come about. I've also stated why we need to add the Forum module to the list of enforced dependencies.
Note to the reviewers of the documentation please bear in mind #2360647: Documentation in ConfigDependencyManager conflates plugin dependencies and config dependencies making it confusing and that reviewing it dreditor is difficult since the whole thing has to be read as one and the patch is moving stuff about.
Comment #92
Gábor HojtsyThanks for these extensions of the docs, looks great. Back to RTBC as per 81 because all changes since then are fixing the concerns raised and they look good.
Comment #93
jhodgdonThe patch has some oddities in the docs... I will make a new one to fix this...
Comment #94
jhodgdonHere's an interdiff... sorry got very busy today and wasn't able to finish. Hopefully this is clear... there was a duplicated line, some text that needed editing, and some things referring to "previous paragraph" and "this example" that didn't make sense with the latest changes in the patch.
Comment #95
alexpottThe changes by @jhodgdon in #94 look great. Applied them to the patch attached. Since @Gábor Hojtsy, @vijaycs85 and @dawehner have set this rtbc and @webchick set this back to needs work for needs better docs and now @jhodgdon has contributed I think I'm good to set this back to rtbc!
The interdiff for the patch attached is in #94
Comment #96
vijaycs85Thanks @alexpott. +1 to RTBC.
Comment #97
catchCommitted/pushed to 8.0.x, thanks!
Comment #99
alexpottd.o weirdness
Comment #101
xjmThis should have had a CR for the changed expectations in forum. :) I also think it's worth a CR explaining the enforced dependencies because it's a significant API addition.
https://www.drupal.org/node/2235409 also doesn't cover this yet, but there's a separate issue to update that documentation.
Comment #102
larowlanDraft for the enforced dependencies is https://www.drupal.org/node/2404447
Comment #103
larowlanComment #104
mkostrzewa CreditAttribution: mkostrzewa commentedDraft for deleting forum posts before uninstalling forum module: https://www.drupal.org/node/2404451
Comment #105
larowlanComment #106
kim.pepperBoth change records reviewed and published.
Comment #107
xjmThanks PNX!
Comment #109
tjtj CreditAttribution: tjtj commentedThis arose for me in D10.1.2. I needed to uninstall forums because the table has no primary key. I deleted the forum instances as the uninstall tole me to do, but now I still cannot uninstall forum because it says the forums have content. The above patch seems to be installed.
How do I fix this?