I've looked into both #232327: Deleting node type leaves orphan nodes and #400450: Be more graceful when editing nodes with disabled node types both have relevant information but neither completely seem correct and core maintainers have different views on the subject.

To get down to the bottom of this something has to be decided, do we leave the data and mark it disabled on uninstall or do we remove it completely. Here's an example?

I'm working on the quotes update in contrib for D7. It uses the node module but the body is actually stored in {fields_data}, ok no problem unless you uninstall the module. what happens when you do is the modules schema is uninstalled, so we lose data there, the fields entity is removed, just lost the body but the {node} table retains the title, nid, vid, also {node_revision} still retains the orphaned data. However, what good is the data if it's been corrupted, and without a body what good is it at all. At this point we then have theoretically wiped out menu links, etc.., etc.., etc.. and the system should ignore those nodes.

Ok, now for some reason, I decide to re-install the module. Ooops, error messages galore and no way fix them without being able to access the database. Then even if you can access the database will the average user be able to manipulate it to clear up the mess left behind by the orphaned nodes.

So what is the answer? do we leave the data or completely delete it? My vote would be to fix the uninstall process by deleteing the orphaned nodes residing in both the {node} and {node_revision} table since it is useless without the body that has been removed already through the un-install process. Is this a function that contrib needs in thier uninstall? if so then why was it decided to uninstall the scheme by core instead of through contrib? I believe the reason was to alleviate database corruption on a system level.

However, I can see the use of keeping the data as well for archival purposes but if that is the way it is to be done then anything field related that's deleted by a uninstall needs to be kept. Either way is fine. As late as it is in the D7 cycle, however, I would suggest the easier fix of deleting and then as catch said in one of the prior linked posts to make an archival system for D8. While this may not be a beta-blocker for D7, it needs to be fixed before the official release.

Files: 
CommentFileSizeAuthor
#39 874000-remove-nodes-when-content-type-is-deleted.patch3.01 KBtizzo
#36 232327-add-nodes-to-queue-for-deletion.patch3.74 KBtizzo
#34 874000-prevent-nodes-from-being-orphaned.patch1.91 KBtizzo
FAILED: [[SimpleTest]]: [MySQL] 29,922 pass(es), 274 fail(s), and 137 exception(es). View
#33 874000-prevent-nodes-from-being-orphaned.patch1.91 KBtizzo
FAILED: [[SimpleTest]]: [MySQL] 29,912 pass(es), 274 fail(s), and 137 exception(es). View
#6 remove_nodes_upon_uninstallation_00.patch1.55 KBXano
FAILED: [[SimpleTest]]: [MySQL] 23,171 pass(es), 6 fail(s), and 4 exception(es). View

Comments

webchick’s picture

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

webchick’s picture

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.

webchick’s picture

Status: Active » Postponed (maintainer needs more info)
Xano’s picture

Title: Uninstall WTF for contrib modules with node module dependence. » Automatically remove module's nodes upon module uninstallation
Assigned: Unassigned » Xano
Status: Postponed (maintainer needs more info) » Active

Working on something using node_modules_uninstalled().

beeradb’s picture

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.

Xano’s picture

Status: Active » Needs review
FileSize
1.55 KB
FAILED: [[SimpleTest]]: [MySQL] 23,171 pass(es), 6 fail(s), and 4 exception(es). View

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.

Jeremy’s picture

Category: bug » feature
Status: Needs review » Active

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

dman’s picture

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.

Xano’s picture

Category: feature » bug
Status: Active » Needs review

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.

beeradb’s picture

Category: bug » feature
Status: Needs review » Active

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.

beeradb’s picture

Category: feature » bug

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

moshe weitzman’s picture

Priority: Critical » Normal
Status: Active » Needs review

Sure, its a normal bug. Downgrading.

dman’s picture

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?

dman’s picture

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.

tizzo’s picture

Title: Automatically remove module's nodes upon module uninstallation » Automatically remove upon node module uninstallation or content type delete
Component: install system » node.module

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

tizzo’s picture

Title: Automatically remove upon node module uninstallation or content type delete » We also want to perform this operation when a content type is deleted
tizzo’s picture

Title: We also want to perform this operation when a content type is deleted » Automatically remove upon node module uninstallation or content type delete

sorry, sloppyness in the queue thanks to me

Xano’s picture

Priority: Normal » Critical

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

webchick’s picture

Title: Automatically remove upon node module uninstallation or content type delete » Automatically remove module's nodes upon module uninstallation
Version: 7.x-dev » 8.x-dev
Component: node.module » install system
Category: bug » feature
Status: Needs review » Active

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.

webchick’s picture

Priority: Critical » Normal
webchick’s picture

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.

kaakuu’s picture

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

beeradb’s picture

Version: 8.x-dev » 7.x-dev

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.

kaakuu’s picture

+1 at #23

ctmattice1’s picture

Tried the patch in #6 as it applied locally.

Several errors on comment module. See below.

After applying the patch from todays HEAD

  • enabled module
  • created a quote
  • disabled module
  • viewed home page
  • uninstalled module

Notes:

On home page revisit, although module was disabled - content was still there - might be page cache as I forgot to clear it.

Main problem though was uninstall was trying to get non-object from comments module.

* Notice: Trying to get property of non-object in comment_node_type_delete() (line 347 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).
* Notice: Trying to get property of non-object in comment_node_type_delete() (line 358 of C:\sites\d7testsuite\modules\comment\comment.module).

beeradb’s picture

Version: 7.x-dev » 8.x-dev

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.

Xano’s picture

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

dman’s picture

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.

Damien Tournoud’s picture

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

ctmattice1’s picture

The whole reason I brought this up was in D6 uninstall was fairly easy. In D7 so much has changed that it's nearly impossible to figure out how to do what was easy in D6. Now you have to deal with both node and entities. Even when stealing the install / uninstall routines from the node_example module this is a difficult task due to different entity controllers and their part in the process.

For D8 I hope someone does come up with a solution to this WTF that's going to create hell with contrib. After D7's release I hope this is one of the first issues to get resolved. There's going to be a bunch of screwed up databases that will need this.

Xano's started a new thread #890220: Add operations (event handlers) to entities with the component ajax (actually I think it should be system). Let's at least give us poor contrib maintainers/co-maintainers some options on how to handle this.

David_Rothstein’s picture

Component: install system » node system

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

moshe weitzman’s picture

tizzo’s picture

FileSize
1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 29,912 pass(es), 274 fail(s), and 137 exception(es). View

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.

tizzo’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Xano » tizzo
Category: feature » bug
Status: Active » Needs review
FileSize
1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 29,922 pass(es), 274 fail(s), and 137 exception(es). View
tizzo’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » feature
Status: Needs review » Active

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

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

tizzo’s picture

Status: Active » Needs review
FileSize
3.74 KB

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.

moshe weitzman’s picture

Status: Needs review » Needs work

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

tizzo’s picture

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.

tizzo’s picture

Status: Needs work » Needs review
FileSize
3.01 KB

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?

MustangGB’s picture

Subscribing

halstead’s picture

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

Status: Needs review » Needs work

The last submitted patch, 874000-prevent-nodes-from-being-orphaned.patch, failed testing.

no longer here 584308’s picture

I was getting this error:

* Notice: Trying to get property of non-object in comment_node_type_delete() (line 347 of C:\sites\d7testsuite\modules\comment\comment.module).

when working on custom content types and started looking into where the error was originating and found that the comments module as written is expecting an object when comment_node_type_delete() is called but in some cases was not receiving an object:

function comment_node_type_delete($info) {
  field_attach_delete_bundle('comment', 'comment_node_' . $info->type);
  $settings = array(
    'comment',
    'comment_default_mode',
    'comment_default_per_page',
    'comment_anonymous',
    'comment_subject_field',
    'comment_preview',
    'comment_form_location',
  );
  foreach ($settings as $setting) {
    variable_del($setting . '_' . $info->type);
  }
}

I altered this function slightly to the following and the errors disappeared:

function comment_node_type_delete($info) {
  $type = _node_extract_type($info);
  field_attach_delete_bundle('comment', 'comment_node_' . $type);
  $settings = array(
    'comment',
    'comment_default_mode',
    'comment_default_per_page',
    'comment_anonymous',
    'comment_subject_field',
    'comment_preview',
    'comment_form_location',
  );
  foreach ($settings as $setting) {
    variable_del($setting . '_' . $type);
  }
}
Mile23’s picture

@andyp22: You might consider submitting a patch here: #1565892: Trying to get property of non-object in comment_node_type_delete

sidharrell’s picture

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.

dman’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.