Hello,

I'm a new bie, I've observed that when i enable forum/poll module and create a forum( and then a topic)/poll and then disable the forum/poll module. The nodes which are related to those modules are not being disabled automatically. This is creating problems when those nodes of the types are displayed and we try to edit them. One such problem occurs when we edit the node type the appropriate form is not being displayed, rather a error message is being displayed saying "there is no forum_node_form() function".

Please check this bug and if possible do delete the node types corressponding to the modules that are being disabled.

CommentFileSizeAuthor
#75 400450-75-screenshot-before.png90.65 KBcossovich
#75 400450-75-screenshot-after.png43.12 KBcossovich
#75 400450-75-tests.patch1.13 KBcossovich
FAILED: [[SimpleTest]]: [MySQL] 37,306 pass(es), 1 fail(s), and 2 exception(s). View
#75 400450-75-node.patch2.24 KBcossovich
PASSED: [[SimpleTest]]: [MySQL] 37,310 pass(es). View
#71 400450-69-node-71.patch2.24 KBmjonesdinero
PASSED: [[SimpleTest]]: [MySQL] 36,667 pass(es). View
#69 400450-69-node.patch2.28 KBmjonesdinero
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 400450-69-node.patch. View
#63 interdiff.txt1.91 KBryanissamson
#63 400450-63-node.patch2.24 KBryanissamson
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.pages.inc. View
#62 400450-62-node.patch2.15 KBryanissamson
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es). View
#53 400450-53-node.patch2.03 KBcosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 36,053 pass(es). View
#48 400450-disabled-content-types-be-nice-D7-v7.patch2.05 KBAham
PASSED: [[SimpleTest]]: [MySQL] 36,886 pass(es). View
#38 400450-disabled-content-types-be-nice-D7-v6.patch2.49 KBbleen
PASSED: [[SimpleTest]]: [MySQL] 19,214 pass(es). View
#35 400450-disabled-content-types-be-nice-D7-v5.patch2.44 KBbleen
PASSED: [[SimpleTest]]: [MySQL] 19,213 pass(es). View
#34 400450-disabled-content-types-be-nice-D7-v4.patch1.19 KBShawn DeArmond
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 400450-disabled-content-types-be-nice-D7-v4.patch. View
#28 400450-disabled-content-types-be-nice-D7-v3_1.patch1.09 KBShawn DeArmond
Failed: Failed to apply patch. View
#26 400450-disabled-content-types-be-nice-D7-v3.patch1.09 KBShawn DeArmond
Failed: 12165 passes, 2 fails, 0 exceptions View
#23 400450-disabled-content-types-be-nice-D7-v2.patch1.08 KBarianek
Failed: Failed to apply patch. View
#17 400450-disabled-content-types-be-nice-D7-v2.patch1.08 KBShawn DeArmond
Failed: Failed to apply patch. View
#14 400450-disabled-content-types-be-nice-D7.patch1.14 KBShawn DeArmond
Failed: Failed to install HEAD. View
#8 400450-disabled-content-types-be-nice-D7.patch1.54 KBDave Reid
Failed: Failed to run tests. View
#8 400450-before.png121.48 KBDave Reid
#8 400450-after.png103.44 KBDave Reid
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

VM’s picture

Assigned: sai571 » Unassigned
Category: bug » support
Priority: Critical » Normal
Status: Active » Closed (won't fix)

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.

sai571’s picture

Hello again,

I've checked forum.install file for hook_unistall implementation for the forum module.

/**
* Implementation of hook_uninstall().
*/
function forum_uninstall() {
  // Load the dependent Taxonomy module, in case it has been disabled.
  drupal_load('module', 'taxonomy');

  // Delete the vocabulary.
  $vid = variable_get('forum_nav_vocabulary', '');
  taxonomy_del_vocabulary($vid);

  db_query('DROP TABLE {forum}');
  variable_del('forum_containers');
  variable_del('forum_nav_vocabulary');
  variable_del('forum_hot_topic');
  variable_del('forum_per_page');
  variable_del('forum_order');
  variable_del('forum_block_num_0');
  variable_del('forum_block_num_1');
}

You can see that forum module drops only the "FORUM" table, but it doesn't delete the rows it had created in "NODE" table, which may be displayed even after the forum is disabled and unintsalled (say through search).

sai571’s picture

Status: Closed (won't fix) » Active
Dave Reid’s picture

Title: This bug is there in forum, poll modules (could be present in other modules also). » Be more graceful when editing nodes with disabled node types
Version: 6.9 » 7.x-dev
Issue tags: +Usability

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

karschsp’s picture

Issue tags: +Novice

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

Dave Reid’s picture

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.

karschsp’s picture

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?

Dave Reid’s picture

Status: Active » Needs review
FileSize
103.44 KB
121.48 KB
1.54 KB
Failed: Failed to run tests. View

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

sai571’s picture

Thank you for your support.

Yes Option B, as stated by Dave Reid would be more graceful and less lossy when the module is only disabled. But when the module is "Uninstalled" all the data that's related to the module should be deleted ( i mean the nodes corressponding to the module in the 'node' table).

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

ainigma32’s picture

Category: support » feature

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

mr.baileys’s picture

Issue tags: +Needs tests

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?
catch’s picture

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.

cburschka’s picture

Status: Needs review » Needs work

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?

Shawn DeArmond’s picture

FileSize
1.14 KB
Failed: Failed to install HEAD. View

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.

Shawn DeArmond’s picture

Status: Needs work » Needs review

bot?

catch’s picture

Issue tags: +Needs text review

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.

Shawn DeArmond’s picture

FileSize
1.08 KB
Failed: Failed to apply patch. View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

Status: Needs work » Needs review

Really Mr.Bot?

Status: Needs review » Needs work

The last submitted patch failed testing.

Bojhan’s picture

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

andypost’s picture

Status: Needs work » Needs review

Some bots was broken

arianek’s picture

FileSize
1.08 KB
Failed: Failed to apply patch. View

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

Shawn DeArmond’s picture

Status: Needs review » Needs work

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.

Bojhan’s picture

Status: Needs review » Needs work

A lot, a lot is involved with that.

Shawn DeArmond’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
Failed: 12165 passes, 2 fails, 0 exceptions View

Okay, this one should work.

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

The last submitted patch failed testing.

Shawn DeArmond’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
Failed: Failed to apply patch. View

Bad bot?

joshmiller’s picture

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

Shawn DeArmond’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Shawn DeArmond’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch failed testing.

Shawn DeArmond’s picture

FileSize
1.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 400450-disabled-content-types-be-nice-D7-v4.patch. View

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

bleen’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 19,213 pass(es). View

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

markdorison’s picture

Logic looks good. Test passed successfully.

markdorison’s picture

Status: Needs review » Reviewed & tested by the community
bleen’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 19,214 pass(es). View

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

rurri’s picture

Status: Needs review » Reviewed & tested by the community

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.

michaelfavia’s picture

Status: Reviewed & tested by the community » Needs review

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

michaelfavia’s picture

Status: Needs review » Reviewed & tested by the community

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.

David_Rothstein’s picture

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.

klausi’s picture

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

mikeytown2’s picture

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.

tizzo’s picture

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

sumitk’s picture

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

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Aham’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.05 KB
PASSED: [[SimpleTest]]: [MySQL] 36,886 pass(es). View

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

sun’s picture

Status: Reviewed & tested by the community » Needs review
philosurfer’s picture

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.

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs 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.

andypost’s picture

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

cosmicdreams’s picture

FileSize
2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 36,053 pass(es). View

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.

cosmicdreams’s picture

Status: Needs work » Needs review

set to review so testbot can check it.

Status: Needs review » Needs work
Issue tags: -Usability, -Needs tests, -Needs text review, -Novice

The last submitted patch, 400450-53-node.patch, failed testing.

zserno’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Needs tests, +Needs text review, +Novice

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Seems good to go

xjm’s picture

Issue tags: -Needs tests
+++ 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.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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

xjm’s picture

Status: Needs review » Needs work
+++ 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

andypost’s picture

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

ryanissamson’s picture

Assigned: Unassigned » ryanissamson
FileSize
2.15 KB
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es). View

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.

ryanissamson’s picture

FileSize
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.pages.inc. View
1.91 KB

Here is the patch based on comments from xjm.

ryanissamson’s picture

Assigned: ryanissamson » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Usability, -Needs text review, -Novice

The last submitted patch, 400450-63-node.patch, failed testing.

mjonesdinero’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Usability, +Needs text review, +Novice

The last submitted patch, 400450-63-node.patch, failed testing.

marcingy’s picture

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))
mjonesdinero’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 400450-69-node.patch. View

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

Status: Needs review » Needs work

The last submitted patch, 400450-69-node.patch, failed testing.

mjonesdinero’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 36,667 pass(es). View

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

mjonesdinero’s picture

Status: Needs review » Reviewed & tested by the community
bleen’s picture

Status: Reviewed & tested by the community » Needs review

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

mjonesdinero’s picture

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

cossovich’s picture

FileSize
2.24 KB
PASSED: [[SimpleTest]]: [MySQL] 37,310 pass(es). View
1.13 KB
FAILED: [[SimpleTest]]: [MySQL] 37,306 pass(es), 1 fail(s), and 2 exception(s). View
43.12 KB
90.65 KB

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:
Before screenshot

After:
After screenshot

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me!

catch’s picture

Status: Reviewed & tested by the community » Needs review

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?

tim.plunkett’s picture

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

catch’s picture

mtift’s picture

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.

klonos’s picture

olav’s picture

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

olav’s picture

Issue tags: +Amsterdam2014