When it gets enabled, Forum does a number of things that do not get undone during uninstall:
- creates the vocabulary "forum_nav_vocabulary"
- creates the field "taxonomy_forums"
- set the variable "forum_nav_vocabulary"
I could understand an argument against deleting the vocabulary: taxonomy is used in a lot of different ways by contrib modules, and deleting it out from under contrib modules could cause problems.
But, the variable should be deleted, and if the field is not in use by any other bundle, it should go as well.
In forum_uninstall(), this patch:
- deletes the variable "forum_nav_vocabulary"
- checks to see if the field "taxonomy_forums" is in use by any bundle besides node type forum. If it is not in use by another bundle, it is deleted.
Comment | File | Size | Author |
---|---|---|---|
#19 | Screenshot_2021-06-27 Confirm uninstall dev1-web.png | 41.25 KB | quietone |
#19 | Screenshot_2021-06-27 Uninstall dev1-web.png | 10.58 KB | quietone |
#5 | 837806-5-forum_uninstall.patch | 1.02 KB | AaronBauman |
forum_uninstall.patch | 1.23 KB | AaronBauman | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedI would actually delete the vocab too since it is clearly owned by forum module (see its machine name). But I can see both sides so lets just go with what you've proposed.
Comment #2
andypost+1, also related #599016: Only allow nodes to be posted to forums
Comment #3
Dries CreditAttribution: Dries commentedI'm not sure I fully understand this patch. We know that there is exactly one bundle, so why would $bundles be empty()?
Comment #4
andypostI think probably bundle could be deleted
Comment #5
AaronBaumanDries:
I think the logic in the attached revised patch is cleaner. Please let me know if it's still unclear.
andypost:
the bundle gets deleted since it's owned by a module that's getting disabled. I think node module handles this.
Comment #6
andypostRelated #599016: Only allow nodes to be posted to forums
Why the condition is
count($field_info['bundles']) == 1
Comment #7
AaronBaumanandypost:
In English, the logic should read:
"Given the field "taxonomy_forums", if the count of bundles is 1, and the count of entities for the 'node' bundle is 1, and the single node entity is 'forum', then delete the field 'taxonomy_forums'"
After re-examining, I see that the if-statement in the previous patch is insufficient.
(It doesn't account for other node types using the field, only other bundles.)
Before I roll another patch, should we further discuss whether this is desirable behavior?
Comment #8
andypostAaron, I scare we should remove that field from all bundles while #599016 is not fixed
Anyway, this field could be optionaly attached by some contrib module so if forum is uninstalling then it should remove all its traces.
I'm not sure that deleting a field will remove it from all bundles
Comment #9
larowlanComment #10
andypostI've marked #599016 as fixed because it iterated now right (only node bundles are used)
The only questions:
1) should we delete forum field?
2) should we check for entities/bundles that could use this field?
3) if we delete variable
forum_nav_vocabulary
then we shoudl delete vocabulary or make it's seaching in forum_enable() more robust because now we had exception trying to make vocabulary with a same name again.I think all forum variables should be removed in _uninstall hook with vocabulary and fields
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedI tested this on Drupal 9.3.x, standard install, using devel to generate content. I also added a second term reference field to the forum content. After generating content, I edited a couple of pages to make sure terms for used in both the reference fields. Then I went to admin/modules/uninstall forum.
There is a nice message saying that I have to some work by hand first,
I did than then returned and uninstalled forum. Then get a nice list of what will be deleted:
Note that it is also deleting the term reference field I added, TestTerms. That wasn't being used anywhere else so it was ok to delete.
That seems to working fine. Is there any need to improve that in some way? If not, this can be moved to Drupal 7.
Comment #20
larowlanAgree, this is D7 domain now