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.
Comment | File | Size | Author |
---|---|---|---|
#45 | 1002164-2.45.patch | 10.64 KB | alexpott |
#45 | 43-45-interdiff.txt | 2.4 KB | alexpott |
#43 | 1002164.43.patch | 10.65 KB | Devin Carlson |
#43 | interdiff-1002164-40-43.txt | 707 bytes | Devin Carlson |
#40 | 1002164.40.patch | 10.63 KB | Devin Carlson |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the report. I confirmed in 8.x-dev. We need to fix this in 8.x and then backport to 7.x
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedAs 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.
Comment #4
mrf CreditAttribution: mrf commentedShouldn'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.
Comment #5
naxoc CreditAttribution: naxoc commentedHere 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.
Comment #6
naxoc CreditAttribution: naxoc commentedPatch 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()
?I have no idea if that is too soon for other stuff to happen though.
Comment #7
naxoc CreditAttribution: naxoc commentedComment #8
Carsten Müller CreditAttribution: Carsten Müller commentedthe 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.
the node is also still present.
the book table from the schema is removed.
edit:
removing the content type via admin/structure/types works.
Comment #9
Carsten Müller CreditAttribution: Carsten Müller commentedComment #10
Carsten Müller CreditAttribution: Carsten Müller commenteddeleting of the node type
The code from naxoc works
old code
....
new code
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:
Comment #11
Carsten Müller CreditAttribution: Carsten Müller commentedi 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.
Comment #12
Carsten Müller CreditAttribution: Carsten Müller commentedComment #13
broonThe 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.
Comment #14
Carsten Müller CreditAttribution: Carsten Müller commentedI 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?
Comment #15
Carsten Müller CreditAttribution: Carsten Müller commentedpatch 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.
Comment #16
Carsten Müller CreditAttribution: Carsten Müller commentedComment #18
Carsten Müller CreditAttribution: Carsten Müller commentedidea 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.
Comment #19
Carsten Müller CreditAttribution: Carsten Müller commentedComment #20
naxoc CreditAttribution: naxoc commentedLooks like it is not just book module that has this problem.
Comment #21
catchMarked #2360065: Delete book data on uninstall as duplicate. Bumping to critical for 8.x.
Comment #22
vijaycs85Using 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.
Comment #24
Gábor HojtsyBase on the fails, it seems like this may be invoked when book module is not fully installed yet or not installed anymore.
Comment #25
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #26
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #27
xjmComment #28
alexpottI think we have two possible solutions:
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.
Comment #29
alexpottUpdated 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.
Comment #30
vijaycs85$a? may be debugging left out?
Comment #31
alexpottHere's a test and removed the debug code.
Comment #33
alexpottThank you Drupal - what a nice way of telling me I'm missing an @group on my test.
Comment #34
alexpottComment #35
xjmComment #36
xjmComment #37
alexpottRerolled.
Comment #39
alexpottThere were problems with fields for book nodes being creating before the book node type. The patch attached fixes and improves the test.
Comment #40
Devin Carlson CreditAttribution: Devin Carlson commentedReroll 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.Comment #42
alexpottNeeds to use the new
module_installer
service instead.Comment #43
Devin Carlson CreditAttribution: Devin Carlson commentedIncluded the change from #42.
Comment #44
Wim LeersSuper nitpick: Part of the second line can still fit on the first line.
s/node's/nodes/
This seems an unrelated change?
Super nitpick: Unnecessary newline.
s/entity_create('node_type', /NodeType::create(/
Why delete this test coverage? It tests something else than the newly added test coverage, doesn't it? It was deleted in #29.
Comment #45
alexpottre #44
Also found an issue with a test assertion message.
Comment #46
Wim Leers#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 onlyforum.module
was using this. (Grep fornode.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 testingNodeType::isLocked()
is out of scope here.Assuming that this is a correct assessment, then this is RTBC.
Comment #47
catchI 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!
Comment #50
fagoI'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?
Comment #51
fagocreated #2392351: When an entity bundle config gets deleted, entities of that bundle break