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:

  1. 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.
  2. 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.
  3. If other config entities have added the taxonomy field, they too are subject to the same rules.
  4. Forum nodes must be deleted before the module is uninstalled (same as #2).
  5. Forum taxonomy terms must be removed before the vocabulary can be deleted (same as #2 - as the 'forum_container' field will dictate this too).
  6. 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.

CommentFileSizeAuthor
#95 2224581-2.95.patch40.86 KBalexpott
#94 interdiff.txt4.67 KBjhodgdon
#91 2224581-2.91.patch40.03 KBalexpott
#91 89-91-interdiff.txt3.5 KBalexpott
#89 2224581-2.89.patch39.29 KBalexpott
#89 85-89-interdiff.txt1.84 KBalexpott
#85 2224581-2.85.patch39.27 KBalexpott
#85 77-85-interdiff.txt13.93 KBalexpott
#77 2224581-2.77.patch36.57 KBalexpott
#69 2224581-2.69.patch36.56 KBalexpott
#69 66-69-interdiff.txt2.46 KBalexpott
#66 63-66-interdiff.txt3.73 KBalexpott
#66 2224581-2.66.patch36 KBalexpott
#63 2224581-2.63.patch33.08 KBalexpott
#63 62-63-interdiff.txt12.85 KBalexpott
#62 forum-delete-3.62.patch20.87 KBlarowlan
#62 interdiff.txt549 byteslarowlan
#60 59-60-interdiff.txt2.99 KBalexpott
#60 2224581-2.60.patch20.84 KBalexpott
#59 forum-delete-2224581.59.patch19.65 KBmgifford
#49 forum-delete-2224581.49.patch19.66 KBlarowlan
#49 interdiff.txt3.13 KBlarowlan
#46 2224581.46.patch19.19 KBalexpott
#46 41-46-interdiff.txt2.91 KBalexpott
#41 2224581.41.patch17.99 KBalexpott
#41 39-41-interdiff.txt2.1 KBalexpott
#39 2224581.39.patch19.39 KBalexpott
#39 37-39-interdiff.txt1.89 KBalexpott
#37 2224581.37.patch19.58 KBalexpott
#37 34-37-interdiff.txt1.86 KBalexpott
#34 25-34-interdiff.txt11.61 KBalexpott
#34 2224581.34.patch18.86 KBalexpott
#25 forum-please-clean-your-room-2224581.25.patch21.07 KBlarowlan
#25 interdiff.txt4.39 KBlarowlan
#22 forum-please-clean-your-room-2224581.3.patch18.42 KBlarowlan
#22 interdiff.txt569 byteslarowlan
#21 interdiff.txt10.59 KBlarowlan
#20 forum-please-clean-your-room-2224581.2.patch18.49 KBlarowlan
#16 forum-please-clean-your-room-2224581.16.patch9.31 KBlarowlan
#3 forum-did-you-make-this-mess-2224581.3.patch1.63 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

larowlan’s picture

Hai
Where is the patch

larowlan’s picture

Status: Active » Needs review
FileSize
1.63 KB

expecting this to fail cause sure there's some tests around this like moduleenabledisabletest or somesuch

Status: Needs review » Needs work

The last submitted patch, 3: forum-did-you-make-this-mess-2224581.3.patch, failed testing.

xtfer’s picture

Issue summary: View changes
andypost’s picture

andypost’s picture

discussed with @alexpott

<andypost> alexpott, currently forum just partially deletes data
<alexpott> andypost: yeah what it does at the moment is wrong
<alexpott> andypost: partial is really wrong
<andypost> alexpott, so are you agree that uninstall should remove data?
<alexpott> andypost: I discussed this yched and others recently and he made a very good point that if forum was designed today it would not use node it would have its own entity
<alexpott> andypost: if forum was an entity type unisntalling the module would remove everything
xjm’s picture

Priority: Normal » Major

This is at least major.

xjm’s picture

Priority: Major » Critical

Per discussion with @alexpott and @larowlan, this blocks #2140511: Configuration file name collisions silently ignored for default configuration, so bumping to critical.

xjm’s picture

Title: don't remove forum config entities on uninstall » Delete forum data on uninstall
Issue tags: +Needs issue summary update

And 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.)

xjm’s picture

alexpott’s picture

+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.

xjm’s picture

From #2032699: Preserve taxonomy_forums field when uninstalling forum module:

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.)
larowlan’s picture

Issue summary: View changes
larowlan’s picture

Assigned: Unassigned » larowlan

ding ding

larowlan’s picture

Status: Needs work » Needs review
FileSize
9.31 KB

No interdiff, clean start.

andypost’s picture

+++ b/core/modules/forum/forum.module
@@ -737,3 +739,33 @@ function template_preprocess_forum_submitted(&$variables) {
+ * Prevents forum module from being uninstalled whilst any forum nodes exist
+ * or there are any terms in the forum vocabulary.
...
+    $terms = entity_load_multiple_by_properties('taxonomy_term', ['vid' => $vid]);
+    if (!empty($terms)) {
...
+        $info['explanation'] = t('To uninstall Forum first delete all %vocabulary terms.', ['%vocabulary' => $vocabulary->label()]);

Vocabulary 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

Status: Needs review » Needs work

The last submitted patch, 16: forum-please-clean-your-room-2224581.16.patch, failed testing.

larowlan’s picture

#17 great idea

larowlan’s picture

Status: Needs work » Needs review
FileSize
18.49 KB

Address #17 and tries to sort some failing tests.

larowlan’s picture

FileSize
10.59 KB

Interdiff for #20

larowlan’s picture

Minor cleanup, self-reviews++

The last submitted patch, 20: forum-please-clean-your-room-2224581.2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: forum-please-clean-your-room-2224581.3.patch, failed testing.

larowlan’s picture

So 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.

pwolanin’s picture

typo/grammar:

+  // Do not allow to delete the book's node type machine name.

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.

xjm’s picture

Status: Needs work » Needs review

Can has testbot?

larowlan’s picture

doh

Status: Needs review » Needs work

The last submitted patch, 25: forum-please-clean-your-room-2224581.25.patch, failed testing.

larowlan’s picture

So 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?

catch’s picture

Issue tags: +D8 upgrade path
xjm’s picture

Issue tags: +Pre-AMS beta sprint

Tagging as a priority for a pre-AMS beta sprint.

larowlan’s picture

Alexpott pointed out that forum should add thirdpartysettings for forum node type to enforce the dependency

alexpott’s picture

Status: Needs work » Needs review
FileSize
18.86 KB
11.61 KB

I 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?

andypost’s picture

+++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
@@ -76,8 +76,16 @@ public function testInstallUninstall() {
+    // Delete any forum terms so it can be uninstalled.
+    $vid = \Drupal::config('forum.settings')->get('vocabulary');
+    $terms = entity_load_multiple_by_properties('taxonomy_term', ['vid' => $vid]);

+++ b/core/modules/system/src/Tests/Module/DependencyTest.php
@@ -154,6 +154,15 @@ function testUninstallDependents() {
+    // Delete any forum terms.
+    $vid = \Drupal::config('forum.settings')->get('vocabulary');
+    // Ensure taxonomy has been loaded into the test-runner after forum was
+    // enabled.

+++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
@@ -151,6 +151,15 @@ public function testInstallUninstall() {
   protected function assertSuccessfullUninstall($module, $package = 'Core') {
     $edit = array();
+    if ($module == 'forum') {
+      // Forum cannot be uninstalled until all of the content entities related
+      // to it have been deleted.
+      $vid = \Drupal::config('forum.settings')->get('vocabulary');

there's no way to delete them automatically?

+++ b/core/modules/forum/forum.module
@@ -725,3 +727,52 @@ function template_preprocess_forum_submitted(&$variables) {
+    $nodes = entity_load_multiple_by_properties('node', ['type' => 'forum']);
...
+    $terms = entity_load_multiple_by_properties('taxonomy_term', ['vid' => $vid]);

please use count for entity query

Status: Needs review » Needs work

The last submitted patch, 34: 2224581.34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
19.58 KB

@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.

andypost’s picture

Looks good except routing and access, once we in context of vocabulary better use entity access

  1. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,63 @@ function template_preprocess_forum_submitted(&$variables) {
    +      ->range(0, 1)
    ...
    +      ->range(0, 1)
    

    is not needed for count

  2. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,63 @@ function template_preprocess_forum_submitted(&$variables) {
    +      $access = \Drupal::currentUser()->hasPermission('administer taxonomy');
    

    $vocabulary->access('view')

  3. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,63 @@ function template_preprocess_forum_submitted(&$variables) {
    +            '!url' => \Drupal::url('entity.taxonomy_vocabulary.overview_form', ['taxonomy_vocabulary' => $vid]),
    ...
    +            '!url' => \Drupal::url('entity.taxonomy_vocabulary.overview_form', ['taxonomy_vocabulary' => $vid]),
    

    $vocabulary->url('overview-form')

alexpott’s picture

FileSize
1.89 KB
19.39 KB

Fixed stuff from #38

larowlan’s picture

+++ b/core/modules/forum/config/install/node.type.forum.yml
@@ -1,9 +1,15 @@
+  forum: {  }

+++ b/core/modules/forum/config/schema/forum.schema.yml
@@ -64,3 +64,8 @@ block.settings.forum_new_block:
+  type: ignore

ooh nice

+1 RTBC @alexpott++

alexpott’s picture

FileSize
2.1 KB
17.99 KB

Fixed docs and could not see why this change requires the new testCommentFieldDelete test.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

rtbc if green

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
@@ -63,29 +63,6 @@ function testCommentDefaultFields() {
-   * Tests that you can remove a comment field.
-   */
-  public function testCommentFieldDelete() {
-    $this->drupalCreateContentType(array('type' => 'test_node_type'));
-    $this->container->get('comment.manager')->addDefaultField('node', 'test_node_type');
-    // We want to test the handling of removing the primary comment field, so we
-    // ensure there is at least one other comment field attached to a node type
-    // so that comment_entity_load() runs for nodes.
-    $this->container->get('comment.manager')->addDefaultField('node', 'test_node_type', 'comment2');
-
-    // Create a sample node.
-    $node = $this->drupalCreateNode(array(
-      'title' => 'Baloney',
-      'type' => 'test_node_type',
-    ));
-
-    // Delete the first comment field.
-    FieldStorageConfig::loadByName('node', 'comment')->delete();
-    $this->drupalGet('node/' . $node->nid->value);
-    $this->assertResponse(200);
-  }
-
-  /**

It was moved here from forum module tests, see hunk below

+++ b/core/modules/forum/src/Tests/ForumUninstallTest.php
@@ -65,27 +71,69 @@ function testForumUninstallWithField() {
-    // Uninstall the forum module which should trigger field deletion.
-    $this->container->get('module_handler')->uninstall(array('forum'));
-
-    // We want to test the handling of removing the forum comment field, so we
-    // ensure there is at least one other comment field attached to a node type
-    // so that comment_entity_load() runs for nodes.
-    \Drupal::service('comment.manager')->addDefaultField('node', 'forum', 'another_comment_field', CommentItemInterface::OPEN, 'another_comment_field');
-
-    $this->drupalGet('node/' . $node->nid->value);
-    $this->assertResponse(200);

this bit

larowlan’s picture

ie we need that test method back

catch’s picture

  1. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,59 @@ function template_preprocess_forum_submitted(&$variables) {
    +      ->count()
    

    If we only want to check existence, range(0, 1) is plenty.

  2. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,59 @@ function template_preprocess_forum_submitted(&$variables) {
    +      ->count()
    

    Same here for range(0, 1)

  3. +++ b/core/modules/forum/forum.module
    @@ -725,3 +727,59 @@ function template_preprocess_forum_submitted(&$variables) {
    +        if ($vocabulary->access('view')) {
    

    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?

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
19.19 KB

Fixed everything from #43 and #45 - just added the link - if it 403's so be it.

larowlan’s picture

Thanks alexpott, beat me to it

+++ b/core/modules/forum/forum.module
@@ -742,6 +742,7 @@ function forum_system_info_alter(&$info, Extension $file, $type) {
       ->count()

@@ -752,6 +753,7 @@ function forum_system_info_alter(&$info, Extension $file, $type) {
       ->count()

these can go now we use range right?

Status: Needs review » Needs work

The last submitted patch, 46: 2224581.46.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
19.66 KB

Fixes fails and #47

xjm’s picture

Issue tags: +beta target
larowlan’s picture

Based on 42 to 45, I think this is back to rtbc

andypost’s picture

@catch #45-3

+++ b/core/modules/forum/forum.module
@@ -725,3 +728,63 @@ function template_preprocess_forum_submitted(&$variables) {
+          if ($vocabulary->access('view')) {

I'd suggest to use that to prevent edge cases like custom access for vocabularies

+++ b/core/modules/forum/forum.module
@@ -725,3 +728,63 @@ function template_preprocess_forum_submitted(&$variables) {
+            $info['explanation'] = t('To uninstall Forum first delete all <em>Forum</em> content and <a href="!url">%vocabulary</a> terms.', [
...
+            $info['explanation'] = t('To uninstall Forum first delete all <em>Forum</em> content and %vocabulary terms.', [

we have /admin/content - once we point to taxonomy pages suppose it makes sense to provide a link here too

xjm’s picture

I did a quick skim of the patch and this looks like exactly the right solution all around. Nice work!

+++ b/core/modules/forum/forum.module
@@ -725,3 +728,63 @@ function template_preprocess_forum_submitted(&$variables) {
+          if ($vocabulary->access('view')) {
+            $info['explanation'] = t('To uninstall Forum first delete all <em>Forum</em> content and <a href="!url">%vocabulary</a> terms.', [
+              '%vocabulary' => $vocabulary->label(),
+              '!url' => $vocabulary->url('overview-form'),
...
+          $info['explanation'] = t('To uninstall Forum first delete all <a href="!url">%vocabulary</a> terms.', [
+            '%vocabulary' => $vocabulary->label(),
+            '!url' => $vocabulary->url('overview-form'),

I don't particularly care if we do the access check or not, but let's do it in both places, or neither?

xjm’s picture

+++ b/core/modules/forum/config/install/node.type.forum.yml
@@ -1,9 +1,15 @@
+  forum: {  }

+++ b/core/modules/forum/config/schema/forum.schema.yml
@@ -64,3 +64,8 @@ block.settings.forum_new_block:
+
+# Forum's third party settings only exist to ensure that the node type is
+# dependent on the forum module.
+node_type.third_party.forum:
+  type: ignore

Oh 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.

Berdir’s picture

Dependency 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())

Berdir’s picture

That 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.

alexpott’s picture

Then 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.

marcus7777’s picture

Issue tags: +Amsterdam2014

the patch #49 will not apply.

git apply --index forum-delete-2224581.49.patch 
error: patch failed: core/modules/forum/forum.module:10
error: core/modules/forum/forum.module: patch does not apply
mgifford’s picture

Thanks @marcus7777 - here's a straight re-roll.

alexpott’s picture

FileSize
20.84 KB
2.99 KB

So we can add fixed dependencies... like so.

Status: Needs review » Needs work

The last submitted patch, 60: 2224581-2.60.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
549 bytes
20.87 KB

looks good, fixes fail
think this is ready

alexpott’s picture

FileSize
12.85 KB
33.08 KB

Improved 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:

uuid: 4d079fb1-d00c-410b-8c2c-44741b7ddf0b
langcode: en
status: true
dependencies:
  module:
    - forum
  fixed:
    module:
      - forum
name: 'Forum topic'
type: forum
description: 'A <em>forum topic</em> starts a new discussion thread within a forum.'
help: ''
new_revision: false
preview_mode: 1
display_submitted: true
third_party_settings: {  }

Basically the fixed dependencies are duplicated in the calculated dependencies - I can't think of a way around this.

vijaycs85’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -208,9 +208,8 @@ route:
-config_dependencies:
+config_dependencies_calculated:

@@ -228,6 +227,14 @@ config_dependencies:
+  type: config_dependencies_calculated
...
+      type: config_dependencies_calculated

Nice way of reusing elements. would be good fit, if we rename config_dependencies_calculated as config_dependencies_base.

vijaycs85’s picture

Issue summary: View changes
alexpott’s picture

Now with added documentation and @vijaycs85's suggestion from #64

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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"? :)

dawehner’s picture

I like that we secure the data of the users until the administrator decided to drop it!

Some small bits.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -19,6 +19,27 @@
    + * The configuration dependency value is structured like so:
    + * <code>
    + * array(
    + *   'config => array(
    + *     // An array of configuration entity object names.
    + *   ),
    + *   'module' => array(
    + *     // An array of module names.
    + *   ),
    + *   'theme' => array(
    + *     // An array of theme names.
    + *   ),
    + *   'fixed' => array(
    + *     // An array of fixed configuration dependencies.
    + *     'config' => array(),
    + *     'module' => array(),
    + *     'theme' => array(),
    + *   ),
    + * );
    + * 

    Should we also add entity into that documentation here?

  2. +++ b/core/modules/comment/src/Tests/CommentFieldsTest.php
    @@ -63,6 +63,29 @@ function testCommentDefaultFields() {
    +    FieldStorageConfig::loadByName('node', 'comment')->delete();
    +    $this->drupalGet('node/' . $node->nid->value);
    +    $this->assertResponse(200);
    +  }
    

    Maybe you should check that the comment field doesn't appear there? (just to be sure and add a bit more semantic here)

  3. +++ b/core/modules/config/tests/config_test/src/Entity/ConfigTest.php
    @@ -156,4 +137,9 @@ public function onDependencyRemoval(array $dependencies) {
     
    +  public function setFixedDependencies(array $dependencies) {
    

    I don't nitpick here, explicit

  4. +++ b/core/modules/forum/forum.module
    @@ -691,3 +694,63 @@ function template_preprocess_forum_submitted(&$variables) {
    + *
    + * Prevents forum module from being uninstalled whilst any forum nodes exist
    + * or there are any terms in the forum vocabulary.
    + */
    +function forum_system_info_alter(&$info, Extension $file, $type) {
    

    Did we considered on the longrun to be able to validate uninstall using hook_requirements()?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.46 KB
36.56 KB

Thanks for the review @dawehner.

  1. I'm ahead of myself - stuff creeping in from another patch - good spot
  2. Added more explicit testing
  3. Added docs
  4. Nope
alexpott’s picture

@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

  • The duplication of the keys in the fixed and the "calculated" part of the config
  • The necessity to call the ConfigEntityBase::calculateDependencies() to get this functionality

Pros

  • All config entities get this ability. This is good because, for example, Features in Drupal 8 can easily create modules which provides configuration that has a fixed dependency on the created module
  • Adding a module key wouldn't work for something like #2351991: Add a config entity for a configurable, topic-based help system - themes can provide help to
  • Fixed dependencies can be interrogated when the configuration is in staging and before the module that provides the configuration entity type is installed

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.

dawehner’s picture

One thing I really don't get is why do we not remove the fixed dependencies from the other calculated ones ...

alexpott’s picture

@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.

jhodgdon’s picture

@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?

catch’s picture

I 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.

alexpott’s picture

Re #73 if the config entity class extends ConfigEntityBase and does not implement calculateDependencies() then they get it for free. If they implement calculateDependencies() then have to call the parent. Every implementation in core does this atm. If a class does not extend from ConfigEntityBase then they would have to reimplement the functionality.

jhodgdon’s picture

OK, 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. :)

alexpott’s picture

FileSize
36.57 KB

Reroll - context conflict with #2246647: Rename PluginBag to LazyPluginCollection in ConfigEntityBase

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the explanation alex!

andypost’s picture

+1 RTBC, The only question here that not covered with tests:

+++ b/core/modules/forum/forum.module
@@ -691,3 +694,63 @@ function template_preprocess_forum_submitted(&$variables) {
+function forum_system_info_alter(&$info, Extension $file, $type) {
+  // It is not safe use the entity query service during maintenance mode.
+  if ($type == 'module' && !defined('MAINTENANCE_MODE') && $file->getName() == 'forum') {

So in maintenance we still able to remove forum?

alexpott’s picture

re #79 this is the same as field_system_info_alter()

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
@@ -19,6 +19,27 @@
+ * The configuration dependency value is structured like so:
+ * <code>
+ * array(
+ *   'entity => array(
+ *     // An array of configuration entity object names.
+ *   ),
+ *   'module' => array(
+ *     // An array of module names.
+ *   ),
+ *   'theme' => array(
+ *     // An array of theme names.
+ *   ),
+ *   'fixed' => array(
+ *     // An array of fixed configuration dependencies.
+ *     'entity' => array(),
+ *     'module' => array(),
+ *     'theme' => array(),
+ *   ),
+ * );
+ *

I 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).

andypost’s picture

Also modules and themes are extensions, but what's about profiles?

jhodgdon’s picture

I'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?

alexpott’s picture

re #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.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +Configurables
FileSize
13.93 KB
39.27 KB

The 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 to enforced based on the recommendations from @webchick and @xjm that a key would suggest a good method name. I think enforceDependency() 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.

alexpott’s picture

Got a +1 on the new name from @larowlan in IRC

11:56 larowlan: alexpott: enforced sounds good

Status: Needs review » Needs work

The last submitted patch, 85: 2224581-2.85.patch, failed testing.

vijaycs85’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -228,6 +227,14 @@ config_dependencies:
+    fixed:

config schema still has 'fixed'.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.84 KB
39.29 KB

Fixed up the tests and schema - thanks @vijaycs85

Gábor Hojtsy’s picture

I agree enforced sounds good. Not sure the docs explain it very well yet. These are the docs parts:

 *   'enforced' => array(
 *     // An array of configuration dependencies that the config entity is
 *     // ensured to have regardless of the state of the configuration. These
 *     // dependencies are not recalculated on save.
 *     'entity' => array(),
 *     'module' => array(),
 *     'theme' => array(),
 *   ),

[...]

 * If an extension author wants a configuration entity to depend on something
 * that is not calculable then they can add these dependencies to the enforced
 * dependencies key. For example, the forum module provides the forum node type
 * and in order for it to be deleted when the forum module is uninstalled it has
 * an enforced dependency on the module.

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.

alexpott’s picture

FileSize
3.5 KB
40.03 KB

Thanks @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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

The patch has some oddities in the docs... I will make a new one to fix this...

jhodgdon’s picture

FileSize
4.67 KB

Here'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.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
40.86 KB

The 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

vijaycs85’s picture

Thanks @alexpott. +1 to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 0c11136 on 8.0.x
    Issue #2224581 by alexpott, larowlan, jhodgdon, mgifford: Delete forum...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

d.o weirdness

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Assigned: larowlan » Unassigned
Status: Closed (fixed) » Active
Issue tags: +Needs change record

This 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.

larowlan’s picture

Draft for the enforced dependencies is https://www.drupal.org/node/2404447

larowlan’s picture

Issue tags: +Critical Office Hours
mkostrzewa’s picture

Draft for deleting forum posts before uninstalling forum module: https://www.drupal.org/node/2404451

larowlan’s picture

Status: Active » Needs review
kim.pepper’s picture

Status: Needs review » Fixed
Issue tags: -Needs change record

Both change records reviewed and published.

xjm’s picture

Thanks PNX!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tjtj’s picture

This 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?