Be more graceful when editing nodes with disabled node types

nodes aren't deleted when a module is disabled. The data is only deleted when you use the uninstall tab at the top of administer -> modules.

This is done on purpose and is not a bug as far as I can tell, as there are times when disabling a module is necessary, ie. upgrades and you wouldn't want sites to lose their data. If you don't want the data there any longer you must uninstall the module not simply disable it.

Needs to be fixed in 7 (CVS HEAD) first, then backport if necessary.

tagging for the novice queue. shouldn't be too hard to add something to forum.install to clean up nodes of type 'forum'

Thing is, we do not delete existing content. If you disable the blog module, your blog content still shows up. Forum should work the same. Maybe our expectations should change? The first thing that needs to happen is get rid of the nasty error at node/x/edit of a node of a disabled content type.

sorry, misread the thread. Hmmm...I guess the question is, what's the expected behavior? Should you be able to edit content that's of a type that's no longer enabled? Should it catch the fact that forum no longer is enabled and present a friendlier message like:

"This node was created using xyz module, which is no longer available. Please visit the modules section to enable xyz."

Would that work? Is that even feasible?

I've been playing around with this a little bit. We basically have two options:

A. The text at admin/build/modules/uninstall says: "The uninstall process removes all data related to a module." That already includes the content types, but we should also make sure all content associated with that type is also removed. This might lead to unwanted data loss scenarios: user didn't realize that the blog entry content type was related to the blog.module. Poof! All blog content gone!
B. Add a condition to node_page_edit() that checks if the form ID type_node_form doesn't exist. Intercept, throw up a drupal_set_message, and stop the form from being rendered.

Option B seems the most logical, so I created a quick patch to see how it would look compared to the current error message.

Heh...my Amazon search for "Night Watch" from chx's tweet has been saved for all. :)

Why can't we fix it in drupal 6.x rather than going for drupal 7.x

That's because this is a new feature and new features are always implemented in HEAD (i.e. 7.x) and then back ported to earlier versions.

Changing the category to feature request to reflect that.

- Arie

Quick review:

1. You are using return drupal_get_page($page); in node_page_edit in a place where $page might be undefined (if the if access-branch is not used).
2. I guess since this is a new scenario, we should add a test for this?

I don't think we should delete nodes when we uninstall the forum module - nodes belong to the node module, only their type and some metadata belongs to the forum module.

Screenshot looks good, no patch review brain left tonight so just subscribing for now.

For a more readable logical structure, I would suggest reversing the condition so the normal case comes first, and the exception case comes second. First check if the type exists, then if it does return the form - and otherwise return the error page.

---

This fixes the undefined function error on the edit page - but nodes can still be displayed. Should we leave it at this, or hide these orphaned nodes from view completely?

Here's a patch with Arancaytar's suggestion of reversing the condition.

I think that nodes that are created should remain. Nodes belong to the Node module, as catch says.

bot?

I think we should reverse the sentence structure to say that the content can't be edited first, then explain why and how to fix it. Having said that, I tried to rewrite it and it's a bit tricky - got this far:

This content cannot be edited because the content type @type has been disabled or removed.

Yeah, I like catch's wording better. Here's a patch.

Really Mr.Bot?

Stupid bot, it is giving me green! I do not get this.

Some bots was broken

patch applied cleanly. reuploading for test bot - this is a nice looking fix, though i can't really critique the code itself.

This patch applies okay to the latest Drupal 7, but it produces a WSOD when going to the edit screen. I'm going to work on this today.

It would be really nice if it were possible to, if the content type is deleted, allow the user to migrate the node to another content type that still exists. Not sure what's involved with that, though.

A lot, a lot is involved with that.

Okay, this one should work.

I'll ignore the content type migration for the time being.

Bad bot?

What if we set all forum nodes after uninstallation to "unpublished" ?

The nodes (in general) do function fine as is, even without a defined content type. It's just when you try to edit them that the confusing error message appears. This patch is just supposed to make it a little more friendly.

What's up, bot? Let's try it again.

Okay, here's a re-rolled patch, but it's still not perfect. An error message appears when the node is displayed in teaser mode on the home page.

Here's what it says:

Notice: Undefined index: teaser in field_default_view() (line 65 of /path/to/drupal/modules/field/field.default.inc).

I'm not seeing the error message that Shawn mentioned in #34 ... perhaps that error has been fixed somewhere else in the last few months. Anywho, this patch is very similar to #34 but it also includes tests...

Logic looks good. Test passed successfully.

ooops ... forgot an assertion message for the test. No functional differences between this patch and #35 though

I tested both this code change, and the testing script.

I was able to duplicate this issue before applying patch.
After applying test-script patch, testing also failed.
After applying fix patch, the error message now displays, and the test-script runs with a pass.

Related but not duplicate issue regarding comments: #775878: Comment form body field disappears when content type deleted.

I think i set a new drupal time record for a cross post! 46minutes! Drupalcon inet was so bad i just posted when i got back. Sorry about that.

Hm, I guess a Drupal error message is better than a PHP error message, but it's still not amazing :)

Seems like this patch should go in, but also see #232327: Deleting node type leaves orphan nodes for discussion of the underlying problem.

This solution is ugly and not really satisfying, but better than nothing. Patch still applies.

Patch at #38 makes sense. +1 from me.

The "ideal way" would probably require a default node handler so the old data can be displayed & a new permission for viewing "deleted node types". Most likely a new issue.

A few patches related to this have just been added on the related issue #232327: Deleting node type leaves orphan nodes

http://drupal.org/node/232327#comment-3357070 is fixing most of it by showing an error message on node/%/edit and node/%/revisions pages.

What about node view page? I am having issues with #890092: Comment form appering only with a title field if content type of that node is deleted comment form see this

+++ modules/node/node.pages.inc	19 Apr 2010 00:00:47 -0000
@@ -13,7 +13,15 @@
+    $page = array();
...
+    return $page;

1) We usually call this variable $build, not $page.

2) It doesn't look like we need a variable (name) at all here. Just return an empty string (or array).

+++ modules/node/node.pages.inc	19 Apr 2010 00:00:47 -0000
@@ -13,7 +13,15 @@
+    $types_link = user_access('administer content types') ? l(t(' Edit content types.'), 'admin/structure/types') : '';
+    drupal_set_message(t('This content cannot be edited because the content type %type has been disabled or removed.', array('%type' => $node->type)) . $types_link, 'error');

1) Stray leading space.

2) The link text should actually be part of the message to provide sufficient context for translators.

+++ modules/node/node.test	19 Apr 2010 00:00:47 -0000
@@ -298,6 +298,14 @@ class PageEditTestCase extends DrupalWeb
+    // Ensure that the node edit screen throws a message when type no longer exists.

Exceeds 80 chars.

+++ modules/node/node.test	19 Apr 2010 00:00:47 -0000
@@ -298,6 +298,14 @@ class PageEditTestCase extends DrupalWeb
+    module_enable(array('poll'));
+    $this->drupalLogin($this->admin_user);
+    $node = $this->drupalCreateNode(array('type' => 'poll'));
+    module_disable(array('poll'));

If I'm not mistaken, you don't need to login $this->admin_user to create the content type.

+++ modules/node/node.test	19 Apr 2010 00:00:47 -0000
@@ -298,6 +298,14 @@ class PageEditTestCase extends DrupalWeb
+    $this->drupalGet("node/$node->nid/edit");

Could use {$node->nid} for safety.

Powered by Dreditor.

Rewrote patch based on the suggestions made by sun. Let me know if this works. My first contribution to Drupal Core! Yay!

First off patch #48 works :-) it solved the problem of providing a graceful error when editing a node.

But, during a community and contribute meetup between LA Drupal and OC Drupal user groups last evening we had a couple questions/suggestions for this patch.

1. Should the content type associated with the newly disabled module have all of its nodes unpublished as a default action? maybe a notification to this effect when you are disabling the module, it will check for the nodes that are affected and notify user that they are about to be unpublished.
2. or, make this an option on the hook_disable() event? i.e. "You are disabling a module x, would you like all associated nodes disabled? [] yes, []no"
3. and lastly, should there be a graceful error on the viewing of the effected node type as well? using the same method as the above patch this could alert the user that the nodes are visible but missing dependencies.

We would love to hear the communities input on these items before we break ground on any patch work.

Thanks for your work on the patch! This seems like it would be a good change.

In addition to the feedback in #50, this patch would need to go into 8.x first. (Whether or not it could be backported to 7.x is a topic for discussion; see http://drupal.org/node/767608 and http://groups.drupal.org/node/211298). Edit: That also means changing the test to use something other than poll. :)

Also, two minor points about the code style:

+++ b/modules/node/node.testundefined
@@ -346,6 +346,15 @@ class PageEditTestCase extends DrupalWebTestCase {
     $first_node_version = node_load($node->nid, $node->vid);
     $second_node_version = node_load($node->nid, $revised_node->vid);
     $this->assertNotIdentical($first_node_version->revision_uid, $second_node_version->revision_uid, 'Each revision has a distinct user.');
+    ¶

Trailing whitespace here.

+++ b/modules/node/node.testundefined
@@ -346,6 +346,15 @@ class PageEditTestCase extends DrupalWebTestCase {
+    // Ensure that the node edit screen throws
+    // a message when type no longer exists.

This comment wraps a bit too early; we should go as close to 80 chars as possible without going over.

Regarding #50 -- great feedback! #1 and #2 would depend on #89181: Use queue API for node and comment, user, node multiple deletes, so we'd probably want to open a separate followup issue for that. #3 might be a good idea as well; in both the case of that and the original patch here, it'd be helpful to have before/after screenshots for a usability review.

+++ b/modules/node/node.pages.incundefined
@@ -12,7 +12,17 @@
+    $build = array();

unused variable, suppose a message could be displayd in $build to be customizable with contrib

rerolled, I think I got the fixes called out in #51 and #52, also I nearly removed the

global $user;

from node_form since it doesn't seem to be used in that function.

set to review so testbot can check it.

#53: 400450-53-node.patch queued for re-testing.

Seems good to go

+++ b/core/modules/node/node.testundefined
@@ -346,6 +346,14 @@ class PageEditTestCase extends DrupalWebTestCase {
+    // Ensure that the node edit screen throws a message when type no longer exists.

Now this comment is wrapping too late... :)

I'm also a little hesitant about relying on poll for the test, in light of #61285: Remove poll module from core and #1266336: Modernize Poll module, but considering those haven't seen activity in months, I think there's no reason to worry about it for this patch.

Edit: Before/after screenshots would be helpful here, as would a test-only patch exposing the test failure.

cosmicdreams is going to look back at this one later tonight.

+++ b/core/modules/node/node.pages.incundefined
@@ -20,7 +20,16 @@
+    $types_link = user_access('administer content types') ? l(t('Edit content types.'), 'admin/structure/types') : '';
+    drupal_set_message(t('This content cannot be edited because the content type %type has been disabled or removed. ' . $types_link, array('%type' => $node->type)), 'error');

Oh, I just noticed this. This is not the correct way to translate a link in another string. See: http://drupal.org/node/322774

I still think that better to use content $build to display the message to make it easy to customize this page from contrib

Here is a straight-up reroll of the patch because node.test moved since this patch was submitted. I'll get to xjm's comments now and make those fixes.

Here is the patch based on comments from xjm.

#63: 400450-63-node.patch queued for re-testing.

This line is missing a ; at the end.

message = t('This content cannot be edited because the content type %type has been disabled or removed.', array('%type' => $node->type))

Hi updated what did ryanissamson work on.this is my 2nd contribution in drupal if this will not fail..hopefully

sorry about the last update i forgot again the format, this also happen on my first contribution in drupal

@mjonesdinero you shouldn't set an issue to RTBC if you are the one submitting the patch.

sorry about that bleen18, im new in contributing patch, sorry again about that

I've taken the patch from #71 and separated out the test to expose the test coverage. Also added screenshots as suggested by @xjm at #58.

Before:

After:

This looks good to me!

I'm pretty sure we don't allow people to re-enable content types from the content type administration page, they only get disabled when modules providing them are and re-enabled if the module is re-enabled, so the link might not help people at all?

Good point. Though pointing them nowhere might be just as frustrating.

We could point them to #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed :p

I agree. From what I can tell, it seems like this issue should be closed and marked as a duplicate of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed.

Closed (duplicate) as suggested by mtift in #80.