Be more graceful when editing nodes with disabled node types
sai571 - March 13, 2009 - 03:45
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs tests, Needs text review, Novice, Usability |
Description
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.
#1
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.
#2
Hello again,
I've checked forum.install file for hook_unistall implementation for the forum module.
<?php
/**
* 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).
#3
#4
Needs to be fixed in 7 (CVS HEAD) first, then backport if necessary.
#5
tagging for the novice queue. shouldn't be too hard to add something to forum.install to clean up nodes of type 'forum'
#6
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.
#7
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?
#8
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. :)
#9
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??
#10
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
#11
Quick review:
return drupal_get_page($page);innode_page_editin a place where$pagemight be undefined (if the if access-branch is not used).#12
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.
#13
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?
#14
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.
#15
bot?
#16
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.
#17
Yeah, I like catch's wording better. Here's a patch.
#18
The last submitted patch failed testing.
#19
Really Mr.Bot?
#20
The last submitted patch failed testing.
#21
Stupid bot, it is giving me green! I do not get this.
#22
Some bots was broken
#23
patch applied cleanly. reuploading for test bot - this is a nice looking fix, though i can't really critique the code itself.
#24
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.
#25
A lot, a lot is involved with that.
#26
Okay, this one should work.
I'll ignore the content type migration for the time being.
#27
The last submitted patch failed testing.
#28
Bad bot?
#29
What if we set all forum nodes after uninstallation to "unpublished" ?
#30
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.
#31
The last submitted patch failed testing.
#32
What's up, bot? Let's try it again.
#33
The last submitted patch failed testing.
#34
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: