Automatically remove module's nodes upon module uninstallation

The answer is, we do whatever Drupal 6 does. Which as far as I know, is leaving the nodes there.

And unless someone can make the case for D7's behaviour being somehow more broken than D6's behaviour (fatal errors or whatnot), this isn't critical, and is instead a feature request against D8.

Working on something using node_modules_uninstalled().

This has crossover with some of what we're seeing at #890092: Comment form appering only with a title field if content type of that node is deleted. Having a general solution to what should happen when a node type is deleting would be a good thing, as it does break stuff (the comment form in the case of the issue linked above) when node types no longer exist, but content still does.

I merged the new code with the existing node_modules_uninstalled() and moved the function up in the file, following the convention that hook implementations come first in a file, followed by regular functions.

How to test it using core:
1) Install Forum.
2) Create a forum topic.
3) Uninstall Forum.
The node is still there in {node}. After applying this patch, it will be gone. Forum itself doesn't cause any error, so ctmattice1 needs to confirm if this patch solves the problems he experiences with his contrib module. The patch does clean up unused data, which we should do at all times when uninstalling modules.

Per Webchick's comment above, this is not a new bug in D7, and is really a new feature request.

Drupal6 behavior of leaving invalid nodes behind, sometimes displaying n/a, sometimes displaying without a title, sometimes throwing errors. Was broken. We can fix it.
Old behavior was a bug. This therefore would be a bugfix.

Now, in D7, contrib modules are much better at cleaning up after themselves. All the cck-type fields now detach themselves and discard all their data.
Leaving behind only an empty shell.
So all of the interesting data is already gone. You already have data loss.

I've now seen a bunch of issues, UI WTFs all of which would be solved if when you deleted a set of content - that content was actually deleted in full, not leaving these inaccessible, unrecoverable zombie nodes behind.

The behavior in D6 was that old contrib modules often did not delete their extra data. It was, often, possible to recover from a type deletion by just making it again. This was not deliberate behavior, and is not a good thing to support.

Delete node content when deleting the node content type
- this WILL leave our DB in a much more stable, clean state. It's an integrity check.

By rights, there should be a key constraint between {node}.type and {node_type}.type
We may not be able to enforce this in the DB, but we should behave as if that constraint was honored.

This *is* a bug. Modules have to clean up their data when they are uninstalled, which Forum.module doesn't do, for instance. Instead of writing code for every module that defines node types (there are at least three of those in core alone), it's easier to add a couple of extra lines of code to node.module instead of duplicating the same code to different modules.

I agree with dman here. After talking with the people posting to #890092: Comment form appering only with a title field if content type of that node is deleted we've all pretty much agreed that just deleting the content is the correct behavior. It would fix that issue, this issue, and the two issues linked in the original issue by ctmattice1. For this reason I don't know that the current patch in #6 does the job as the code shouldn't be in hook_uninstall, but probably in hook_node_type_delete.

Agree it's a bug. sorry for the cross-post.

Sure, its a normal bug. Downgrading.

As this is a blocker, and a fix for #890092: Comment form appering only with a title field if content type of that node is deleted - which is currently treated as 'critical', this should therefore have the same priority.
So maybe downgrade that one? Or merge the issues?

If we fix this base issue, it would make #232327: Deleting node type leaves orphan nodes redundant - as that just tries to make editing invalid nodes less error-prone.
The fix is to stop invalid nodes from existing.

We also want to perform this operation when a content type is deleted.

sorry, sloppyness in the queue thanks to me

Moving to critical again, as other (critical) issues depend on this.

Yes. And we are not "fixing" this in Drupal 7, because it's debateable whether this is the right thing to do, and a huge behaviour change from D6.

Oops. I cross-posted from like 11 replies ago.

I still maintain that this is a huge behaviour change, and needs to be handled in D8. It's too late for this kind of thing in D7.

Xano, we actually need this. Is the patch working? Any changes will be made to the patch?
Any backport for D6?

Respectfully, I have to disagree with moving this to D8. Simply having a history of functionality which is fundamentally broken is not a basis for allowing that behavior to exist. The root cause of this (not deleting the content) is the basis for no less than four bugs.

See #874000: Automatically remove module's nodes upon module uninstallation, #232327: Deleting node type leaves orphan nodes, #400450: Be more graceful when editing nodes with disabled node types and #890092: Comment form appering only with a title field if content type of that node is deleted

This is all fixable, although admittedly non-trivial, by deleting node content when the node type is removed. I would also argue that this is what the user intuitively expects to happen.

If I'm outvoted and/or just considered wrong, then so be it, but I think it would be a shame to allow this bug to persist on the grounds that it's always been that way. There very well may be compelling reasons (that I'm not aware of) to keep this content around, but I haven't seen them presented here, nor in the issues I linked above. If I'm just missing why this is such a bad idea, providing an explanation would be awesome.

+1 at #23

After discussing this with Angie I've (surprise) come to agree with her. The issue here is not whether or not this should be fixed, but whether or not it should be fixed right now. This works exactly as it did in Drupal 6, and as such having such a major functionality change creep in at this late stage in D7 is not desirable. There's also a larger issue at play here. It's not just node types which can leave data behind when being deleted, but any entities. Things like profiles, fields, users (which has a one off fix in D7) all can leave cruft behind when being deleted. It sounds like we need a generic entity deletion method which ensures data is properly cleaned up. Otherwise we're left with basically copying/pasting code for all of the areas that are effected by this. As such, this needs to be a D8 issue.

A possible solution is being discussed in #890220: Add operations (event handlers) to entities.

So this is to be left broken. I've seen it been 'not fixed' a dozen times now.

In all remaining cases where any contrib ( including comment, views and CCK extensions ) expect a sane response from node_type_get_types(), node_type_get_type($node), node_type_load($type_name) etc, will have to add special-case handling to re-assert that the node they touch is actually a sort-of valid node, and add extra handling to deal with semi-nodes. :-(

Given that status is to be maintained for folk who have already broken their site by deleting a content type ... perhaps we are still able to make that less likely to happen?

Can we catch any attempt to delete a content type that is in use and just refuse?

There are nodes still using this content type. You must delete all nodes using this content type definition before the content type can be removed.
- this addresses the (current) issue/challenge of mass/batch deleting many many nodes.
- It really puts the side-effects of the serious damage you are about to do to your site in your face.

I don't know (this is just a top-of-the-head idea) if we can capture the removal of a module (where this undesirable error usually comes from) and prevent an admin from disabling an in-use module. That would stop the error from even arising. Could be that the dependency system could be leveraged? ... random idea, probably weird scope creep and behavior changes.

This can *easily* be dealt with in contrib. Core cannot (and should not) tackle every possible use cases.

Um, yeah, this patch is a tricky one....

Basically, to do this successfully would make #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do look like an easy cake-walk. It has all the same challenges as that issue (needing to update an arbitrarily large number of nodes at once, could be tens or hundreds of thousands for all we know), but none of the possible workarounds (there is no shortcut to deleting a node - you actually have to delete it). If the field module's system for handling bulk deletion were generalized in D8, maybe this could happen.

It's also not clear to me which bugs this would fully solve, so I kind of agree with the 'feature request' designation: As far as I understand, some (most?) of the "orphaned content" bugs that occur when you uninstall a node-type module also occur when you disable that module. But we obviously don't want to delete the nodes when the module is merely disabled, which means we need to find independent solutions for those bugs anyway.

Moving to the node system queue because I don't think it has much to do with the install system...

IMO this depends on #89181: Use queue API for node and comment, user, node multiple deletes

I agree with dman in #28. What if we stop content type deletions from happening if you have nodes in the db?

I was working on a patch that would perform the node deletions using the job queue but Barry Jaspan pointed out that what we really need to do then is keep the content type around so that we know how to properly delete these nodes. Performing a logical delete on the content type is probably more than we're going to get done right now.

The Interim solution for the moment provided by this patch is to prevent users from deleting a content type if there is any content belonging to it. It's simple but it should keep people from breaking their sites.

I totally posted this to the wrong issue queue. Maybe too much Awesomesauce...

Patch cross posted on #232327: Deleting node type leaves orphan nodes

Ok, this patch actually belongs here.

This patch allows node_delete_multiple() to work with arbitrarily large numbers of nodes by adding them to the queue in chunks of 10 if more than that number have been added. It also updates the comment on the node_delete_confirm form, updates node_type_delete and node_modules_uninstalled() to call node_delete_multiples on all affected nodes.

See the link I posted in #32. You want a reliableQueue for this. ALso, I propose deleting 100 at a time.

I updated as per Moshe's comment and posted it to #89181: Use queue API for node and comment, user, node multiple deletes, which does seem like a parent (if not duplicate) ticket.

This patch really relies on my patch in #89181: Use queue API for node and comment, user, node multiple deletes. It does the same as above but only handles the calls to node_delete_multiple and the text changes.

Perhaps on node_type_delete_confirm_submit() we should perform a batch operation and perform it full time?

Subscribing

This would solve #775878: Comment form body field disappears when content type deleted. for Drupal 8 if it was adopted.

@andyp22: You might consider submitting a patch here: #1565892: node_type_delete() doesn't check the value returned from node_type_get_type()

may or may not be the same issue...
if I install my module, it has the path defined /hello, so the menu system allows me to create an entry with that path.
If I then uninstall my module, I get an unrecoverable error, since the menu admin crashes, it will not then allow me to remove the bad link.
I had to go into the DB with phpmyadmin and delete the offending menu entry. then I could get back into the menu configuration.

Let me know if you need screenshots or casts.
Thanks.

@sidharrell that's probably unrelated.
What you report (the menu being defined by the module itself) should be getting resolved by the menu router rebuild when you uninstall the module.
Look for or open a new issue if that's not working.

But if you are describing a module that provides a new content type, and accessing one of those nodes starts breaking after removing the module ... then yes, it may be related.
IF this is happening today in 8.0.x-dev - Please provide more detail as "steps to replicate"

This seems to be a duplicate of the newer and RTBC #2688945: Allow removing a module's content entities prior to module uninstallation