Problem/Motivation

When the book module gets uninstalled the content type book remains and also it's content. So all book pages are still present and you can also create new ones, although the modue is disabled

Proposed resolution

Only allow uninstalling of the Book module if no nodes exist that are in books and no nodes of type book exist. Then through an enforced dependency on the Book module remove the node type during uninstall.

Remaining tasks

  • reviews

User interface changes

None

API changes

Add method on book outline storage to detect if there are books.

Original report by doka

On a fresh RC2, enable the book module, and create few book pages. Then delete these nodes, and disable and uninstall the book module. After that new book pages can be still created, since content type book page remains available after uninstall of the bookmodule. Expected behaviour is that the content type, which was initilized by the book module, get's also deleted upon uninstall of the module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Version: 7.0-rc2 » 8.x-dev
Issue tags: +Needs backport to D7

Thanks for the report. I confirmed in 8.x-dev. We need to fix this in 8.x and then backport to 7.x

Anonymous’s picture

Status: Active » Needs review
FileSize
427 bytes

This patch deletes the node type. However, because of #1238704: deleted node types still show up in menu until cache clear, it will still show up in the menu until you do a manual cache clear. I'm not sure why the call to menu_cache_clear_all() in book_uninstall doesn't work.

Anonymous’s picture

Title: book page content type is not deleted during book module uninstall » book page content type is not disabled when module disabled
FileSize
797 bytes

As davereid pointed out in IRC, this node type shouldn't be deleted if there is still content that has been defined with it.

Instead, I think it should be disabled when the module is disabled, just as any node type declared with hook_node_info would be.

mrf’s picture

Shouldn't the book content type still be deleted when the module is uninstalled? I assumed yes, but that still leads to the possibility of abandoned content mentioned above.

naxoc’s picture

FileSize
832 bytes

Here is a reroll. When I tested this it seems like #1238704: deleted node types still show up in menu until cache clear is not a problem anymore in D8. It might be for D7 though. I'll take a look.

naxoc’s picture

Issue summary: View changes

Patch does not apply anymore. I tried uninstalling the book module, and now the node type is never deleted no matter how many cache flushes are done.

I'm not really sure how to solve it, but maybe something like this in node_modules_uninstalled()?

foreach ($modules as $module) {
  $config = \Drupal::config('node.type.' . $module);
  $config->delete();
}

I have no idea if that is too soon for other stuff to happen though.

naxoc’s picture

Status: Needs review » Needs work
Carsten Müller’s picture

the problem still exists, the node type is after the uninstall present and the content i created too.

In the config table the entry name: node.type.book is still present.

collection: 
      name: node.type.book
      data: a:13:{s:4:"uuid";s:36:"0216bdab-0853-42b3-b304-4f3979ff9d14";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:0:{}s:4:"name";s:9:"Book page";s:4:"type";s:4:"book";s:11:"description";s:87:"<em>Books</em> have a built-in hierarchical navigation. Use for handbooks or tutorials.";s:4:"help";s:0:"";s:12:"new_revision";b:0;s:12:"preview_mode";i:1;s:17:"display_submitted";b:1;s:20:"third_party_settings";a:0:{}s:24:"book_node_type_installed";i:1;}

the node is also still present.
the book table from the schema is removed.

edit:
removing the content type via admin/structure/types works.

Carsten Müller’s picture

Issue summary: View changes
Carsten Müller’s picture

deleting of the node type

The code from naxoc works

old code

function node_modules_uninstalled($modules) {
  // Remove module-specific settings from all node types.
  $config_names = \Drupal::configFactory()->listAll('node.type.');
  foreach ($config_names as $config_name) {
    $config = \Drupal::config($config_name);
    $changed = FALSE;
    foreach ($modules as $module) {
      if ($config->get('settings.' . $module)) {
        $config->clear('settings.' . $module);
        $changed = TRUE;
      }
    }
    if ($changed) {
      $config->save();
    }
  }<code>

....

new code

function node_modules_uninstalled($modules) {

  // delete node type specific config settings
  foreach ($modules as $module) {
    // delete node type specific config settings
    if ($config = \Drupal::config('node.type.' . $module)) {
      $config->delete();
    }
  }
....

Then the config of node.type.book is removed and the content type disappears. So this could work. I'm unsure if this is the right way how it should be done.

The other topic is about the content of this content type. What should happen with the nodes?
Two possibilities:

  1. The module cannot be uninstalled as long there is content of this type - requires a hook_requirements() checking if there is still content. This is the way how it works in admin/structure/types. The content type can only be deleted when all content was deleted manually.
  2. Delete the content as well when the module is uninstalled. This is the way the uninstall text of the module announces.
Carsten Müller’s picture

i added the last code in the function node_modules_uninstalled() as a patch. But i'm not sure if this is the right way.
I let the tests run and see if that passes.

The question what should happen with the content is still open.
And there is no test for this right now.

Carsten Müller’s picture

Status: Needs work » Needs review
Issue tags: +Amsterdam2014
broon’s picture

Status: Needs review » Needs work

The patch in #11 applies without issues.

However, Drupal now removes the "Book Page" when Book module is uninstalled but the content is still present. Content overview still shows the content without any value in the Type column. When calling the content's url, a fatal error appears:
Fatal error: Call to a member function displaySubmitted() on a non-object in /core/modules/node/node.module on line 620.

The issue of how to proceed with the content should be resolved before this is going to be committed.

Carsten Müller’s picture

Assigned: Unassigned » Carsten Müller

I write an test for that.

But i do not know when and where the content should be deleted.
The question is, should the content be deleted when the module is uninstalled?

Carsten Müller’s picture

patch contains now a test for uninstalling the book module. Fails at the end, because the created node of type book is still available after the uninstall of the book module.

Carsten Müller’s picture

The last submitted patch, 11: book-type-not-removed-1002164-11.patch, failed testing.

Carsten Müller’s picture

idea for removing the content:

usage of hook_uninstall(), but that does not work for me. The type book is not available any more then, even i move the deleting of the config node.type.book also in hook_uninstall() after removing of the content. Seems like the type is removed first.

Carsten Müller’s picture

Assigned: Carsten Müller » Unassigned
naxoc’s picture

catch’s picture

Priority: Normal » Critical

Marked #2360065: Delete book data on uninstall as duplicate. Bumping to critical for 8.x.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

Using the implementation provided by #2224581: Delete forum data on uninstall. We might need to add #16 to make sure we delete the type, but not sure, if it is safe to delete by module name or part of this issue.

Status: Needs review » Needs work

The last submitted patch, 22: 1002164-book-uninstall-warning-22.patch, failed testing.

Gábor Hojtsy’s picture

+++ b/core/modules/book/book.module
@@ -572,3 +575,32 @@ function book_node_type_update(NodeTypeInterface $type) {
+function book_system_info_alter(&$info, Extension $file, $type) {

Base on the fails, it seems like this may be invoked when book module is not fully installed yet or not installed anymore.

pwolanin’s picture

I strongly disagree - why would I want the book content or content type to be deleted?

Also, any type of node content can be added to the book structure.

I propose this should be closed as won't fix, or should be limited to removing the content type only in the case no content of type "book" exists.

pwolanin’s picture

Ok, so, the patch doesn't correspond to the issue summary. The patch simply seems to be preventing you from uninstalling if there is "book" type content.

I'm not sure that's right either, but not as bad as what's suggested in the summary.

xjm’s picture

Issue tags: +Configuration system
alexpott’s picture

Title: book page content type is not disabled when module disabled » The Book module can be uninstalled with nodes with a book node type still existing
Issue tags: -Needs backport to D7

I think we have two possible solutions:

  1. So the we should allow uninstall of book if there are no nodes existing of the types that appear in books. Also we should add an ensured dependency on the book module to node.type.book so that it is removed when book is uninstalled.
  2. Or we could allow book to be uninstalled but not allow book to be reinstalled if the book node type still exists.

We seem to be tending towards solution 1 - and this correlates with #2224581: Delete forum data on uninstall and #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference. Also I don't think that this needs a backport to drupal 7 since node type preservation is a feature in Drupal 7.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +API change
FileSize
6.71 KB
6.74 KB

Updated summary and fixed patch. I exported the book node type from active config since we really should not be editing it directly.

We need to add some tests.

vijaycs85’s picture

+++ b/core/modules/book/book.module
@@ -585,22 +583,26 @@
+      $a = 1;

$a? may be debugging left out?

alexpott’s picture

FileSize
4.38 KB
10.3 KB

Here's a test and removed the debug code.

Status: Needs review » Needs work

The last submitted patch, 31: 1002164.31.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
428 bytes
10.32 KB

Thank you Drupal - what a nice way of telling me I'm missing an @group on my test.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
xjm’s picture

Issue tags: +blocker
xjm’s picture

Issue tags: +D8 upgrade path
alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
10.3 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 37: 1002164.37.patch, failed testing.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.26 KB
10.63 KB

There were problems with fields for book nodes being creating before the book node type. The patch attached fixes and improves the test.

Devin Carlson’s picture

FileSize
10.63 KB

Reroll of #39.

Manually tested to verify that the patch makes it impossible to uninstall the Book module without first deleting all of the book content and outlines. I also didn't find any issues when reviewing the code; it is very similar to #2224581: Delete forum data on uninstall and uses the same approach.

My only suggestion would be to possibly link the first error message to the books overview page (admin/structure/book) to make it easier for users to find the remaining book outlines on their site.

Status: Needs review » Needs work

The last submitted patch, 40: 1002164.40.patch, failed testing.

alexpott’s picture

+++ b/core/modules/book/src/Tests/BookUninstallTest.php
@@ -0,0 +1,103 @@
+    \Drupal::moduleHandler()->uninstall(['book']);

Needs to use the new module_installer service instead.

Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
707 bytes
10.65 KB

Included the change from #42.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/book/book.module
    @@ -572,3 +573,33 @@ function book_node_type_update(NodeTypeInterface $type) {
    + * Prevents book module from being uninstalled whilst any book nodes exist
    + * or there are any book outline stored.
    

    Super nitpick: Part of the second line can still fit on the first line.

  2. +++ b/core/modules/book/book.module
    @@ -572,3 +573,33 @@ function book_node_type_update(NodeTypeInterface $type) {
    +      // there are any node's of that type.
    

    s/node's/nodes/

  3. +++ b/core/modules/book/config/install/node.type.book.yml
    @@ -1,11 +1,15 @@
    +display_submitted: true
    

    This seems an unrelated change?

  4. +++ b/core/modules/book/src/Tests/BookUninstallTest.php
    @@ -0,0 +1,103 @@
    +  public function testBookUninstall() {
    +
    +    // No nodes
    

    Super nitpick: Unnecessary newline.

  5. +++ b/core/modules/book/src/Tests/BookUninstallTest.php
    @@ -0,0 +1,103 @@
    +    $content_type = entity_create('node_type', array(
    

    s/entity_create('node_type', /NodeType::create(/

  6. +++ b/core/modules/node/src/Tests/NodeTypeTest.php
    @@ -123,35 +123,6 @@ function testNodeTypeEditing() {
       /**
    -   * Tests that node types correctly handles their locking.
    -   */
    -  function testNodeTypeStatus() {
    

    Why delete this test coverage? It tests something else than the newly added test coverage, doesn't it? It was deleted in #29.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
10.64 KB

re #44

  1. Fixed
  2. Fixed
  3. This is a re-copy of the node.type.book configuration object from active storage - I'm loathed to just hack the changes in.
  4. Fixed
  5. Fixed
  6. Because now the book node type is removed on uninstall so this test is quite pointless

Also found an issue with a test assertion message.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#45.6: But we still have NodeType::isLocked(), which was being tested in that removed test. But it seems like you're seeing and like the deleted indicates that the test itself was wrong; it looks like only forum.module was using this. (Grep for node.type.locked.) Looks like the test method name plus docs were suggesting one thing, but that the test itself was actually doing something completely different. Hence properly testing NodeType::isLocked() is out of scope here.

Assuming that this is a correct assessment, then this is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I think that's fine with the test coverage. It might be worth opening an issue to see if we could remove node.type.locked altogether - doesn't really make sense with dependency management.

Committed/pushed to 8.0.x, thanks!

  • catch committed 8656312 on 8.0.x
    Issue #1002164 by alexpott, Carsten Müller, Devin Carlson, linclark,...

Status: Fixed » Closed (fixed)

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

fago’s picture

I'm not sure this issue has been properly addressed. You can still delete node.type.book config on dev site and push the changes on a live site. Then, there is no validation in place which prevents preventing your book data. Sounds like a general issue that we need to validate deletions of entity bundle configs?

fago’s picture