Original report by @andypost

Problem/Motivation

Currently forum module trying to delete taxonomy_forums field in forum_uninstall() but this field could be used in other entities. Also forum node type is not deleted when forum uninstalled so it makes sense preserve all its fields as well

Proposed resolution

Remove field deletion from forum_uninstall()
Also this removes need in calling field_purge_batch() so uninstall will be quicker.
This does not affects re-install of the forum module because forum_enable() cares to re-create all needed entities and fields

API changes

no

#2077435: Use a yml file to create the forum vocabulary

CommentFileSizeAuthor
#12 drupal8-forum-2032699-12.patch3.21 KBTemoor
#6 drupal8.forum-module.2032699-6.patch627 bytesandypost
#4 drupal8.forum-module.2032699-4.patch1.03 KBandypost
#1 forum-2032699-1.patch766 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
forum-2032699-1.patch766 bytes

let's see the tests...

Status: Needs review » Needs work

The last submitted patch, forum-2032699-1.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Is there an equivalent of field_delete_instance in d8? that used to have the 'cleanup' flag.
So we could call that, if something else used it - it would stick around, else it would be removed.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Component: plugin system » forum.module
Status: Needs work » Needs review
FileSize
drupal8.forum-module.2032699-4.patch1.03 KB

re-roll

Status: Needs review » Needs work

The last submitted patch, drupal8.forum-module.2032699-4.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
drupal8.forum-module.2032699-6.patch627 bytes

Containers should be removed, also comment about disabled taxonomy module makes no sense

Status: Needs review » Needs work

The last submitted patch, drupal8.forum-module.2032699-6.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

The forum uninstall tests can be removed, as they are to check the field is gone.
The uninstall test I'm not sure on, its making sure that taxonomy can be removed once forum has been - from memory.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Issue summary: View changes

Updated issue summary.

andyceo’s picture

andyceo CreditAttribution: andyceo commented
Status: Needs work » Needs review

6: drupal8.forum-module.2032699-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: drupal8.forum-module.2032699-6.patch, failed testing.

Temoor’s picture

Temoor CreditAttribution: Temoor commented
Issue summary: View changes
FileSize
drupal8-forum-2032699-12.patch3.21 KB

Removed field deletion and it's part of tests.

Temoor’s picture

Temoor CreditAttribution: Temoor commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: drupal8-forum-2032699-12.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs issue summary update

So, I believe this issue runs counter to the expectations we've set up for config dependencies. What should happen is:

  1. You cannot remove a field until all data in it has been purged. This should already be the case following #2198429: Make deleted fields work with config synch.
  2. 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. (See #2224581-7: Delete forum data on uninstall.)
  3. 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.
  4. If other config entities have added the taxonomy field, they too are subject to the same rules. (Field data must be purged before the field is deleted.)

So, I think this issue is actually a wonfix, and what we should do instead is clean up all our data for #2224581: Delete forum data on uninstall.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Related issues: +#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Assigned: Unassigned » larowlan

+1 to wontfix, not sure that "disable module" will be reverted for core so issue makes no sense without that

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Closed (won't fix)

Totally agree to won't fix in light of #2224581: Delete forum data on uninstall