Right,

The steps required to reproduce the bug are:

  1. Create node of content type X
  2. Delete the content type X
  3. Go to node/n/edit where n is the id of the node you created in step one.

What I expected to see was a drop-down list of current content types I could change my node to.

Instead I got this ugly error message:
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'blog_node_form' was given in /home/.saul/lindenlion/michaelkillian.com/includes/form.inc on line 358.

I had to go to the mysql database and manually change the content type entry of my node to something that was available.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LindenLion’s picture

Title: Unexisting node content type un-editable » Unexisting node content type un-editable and un-deletable!
Priority: Critical » Minor

A possible quick fix for this would be to add a delete tab to nodes when you have permission to delete them. The biggest problem with this error is that it is impossible to delete them as the delete button is on the edit page and can't be accessed when the content type doesn't exist anymore... There is then only way to delete these pages: by going to /admin/content/node and selecting all the nodes of that type and deleting.

So my feature request has two sides:

One is adding a delete tab on nodes for users which have delete permissions.

The other one is handling unexisting node types better (which could be part of re-thinking the whole content type system...) so there is no hurry for that

catch’s picture

Category: feature » bug
Priority: Minor » Normal

I think the main issue here is the initial orphaning of the content type rather than the fact you can't delete individual nodes afterwards. My first reaction is that we should absolutely prevent deleting content types which have existing nodes in the database to stop this from happening. That'd still require a contrib module or direct database hacking to change the types before you delete it, but it'd be a bit clearer and avoid that nasty error.

Of course it'd be great to be able to switch content types en masse upon deleting one (and general merging operations) - but this is very tricky, since you may have custom fields or module settings which would be lost moving between differently configured content types. So I reckon stopping the delete operation with a message as to why would be a good start.

Also this isn't minor, and it's definitely a bug if you can do stuff in the UI which generates nasty error messages.

LindenLion’s picture

Status: Active » Postponed (maintainer needs more info)

Of course it'd be great to be able to switch content types en masse upon deleting one (and general merging operations) - but this is very tricky, since you may have custom fields or module settings which would be lost moving between differently configured content types.

Can you tell me more about the trickiness of this? I'm mostly interested in changing for example a Book Page to a Story or something like that (they are all pretty similar these content types anyway), so where do the custom fields come in? Which one of the following have custom fields in the database?:
[1]Page, [2]Story, [3]Book Page, [4]Forum topic, [5]Blog entry, [6]Poll.

My guess is that changing from any of [1-5] to any of [1-5] wouldn't be a problem, but I might be mistaken with the Forum and Blog types. Is my guess wrong with the first three though? Is the content type with these three (or even five!) not just purely a semantical thing with default node configuration when creating the node? The Poll clearly has fields in the database which are not compatible with the other node types.

Please clarify this for me, because if my guess is right, it would mean that it wouldn't be too hard to make a little node type switcher when trying to edit a node with an unexisting type. I guess this would only make it into D7 though?!

catch’s picture

Status: Postponed (maintainer needs more info) » Active

You're right about Poll, except there's no way for core to know what's going to happen to node types via contrib modules so it can potentially happen to any of them.

The issue is if you use CCK to add, for example, an imagefield and additional textfield to "Article", but not to "Page" - then you change the type of a lot of articles over to pages, your fields are orphaned because they don't exist in the new type. It's the same issue with a taxonomy vocabulary that's assigned to one node type and not another.

There's a contrib module that allows you to change node type from the node/edit form: http://drupal.org/project/nodetype - and if you know what fields are where, it's fairly harmless in itself - but unlikely to get into core for the reasons given above.

Also a side note: "Active needs more info" should really be used when you're not sure if a bug really exists or not. i.e. an issue that just says "my site broke" goes to "active (needs more info)" because there's no way to know why it's broken without more information. It also has the side effect of taking issues out of the 'active' queue in contributor links - so you don't want to do that to issues which are definitely valid.

LindenLion’s picture

Hi catch, thanks for the instruction with the status, I had no idea :-D

What I was actually thinking of in the case of a deleted content type was something more like this:

Imagine a node with node type article, with an image field which has been added by CCK, and now the content type article is deleted. Everyone can still read this node (actually I don't know what would happen with the image field on the view tab in this case) and when you try to edit it, it will come up with a screen asking you to select one of the existing content types because article is not available anymore, with a warning that some data might get lost in the conversion. Click cancel if you want to leave the node as it is or if you plan to recreate the node type which is missing. Something along those lines.

How does for example the taxonomy thing work? Are terms tied to specific node types? Because that would simply mean that the taxonomy terms have to be entered from scratch after an emergency conversion, which sounds fine to me.

I can see that this might be much more complicated than I imagine and why it wouldn't make it into core. Thanks for the link to nodetype. Hopefully they get release happening for D6 sometime.

By the way, I started a forum topic about re-thinking node types here: http://drupal.org/node/233222

catch’s picture

It's the potential data loss problem that we need to fix I think - I'm pretty sure changing content types for nodes on the fly has been won't fixed for core before - and I still think we should extend that to deletion of node types with data in.

For taxonomy terms it wouldn't make much difference on display, but you'd have to enable the vocabulary for the new content type to be able to change the terms.

LindenLion’s picture

Yeah, sounds good. Just prevent node types of existing nodes from being deleted. That would solve the bug and make people think before they act.

In fact, I think I forgot to mention, because you get actually a warning message that something bad might happen if you delete a node type. Just didn't tell me what exactly would happen, so I had to try it out :-)

KarenS’s picture

Title: Unexisting node content type un-editable and un-deletable! » Deleting node type leaves orphan nodes

Renaming this to identify the key issue.

I think the only thing that makes sense for core is to absolutely prevent the deletion of content types that have nodes, so the solutions are:

1) Refuse to delete content types that have data.

To make this much friendlier, we could add a couple options if there are nodes rather than just stopping at that point:

2) Add an option to delete the nodes, then delete the type, then use a batch process to delete all the nodes one at a time, triggering hook_nodeapi('delete') so modules can do whatever they need to with the data before the node is deleted.

3) Add a second option to merge the nodes into a different content type. This can only be made to work correctly by adding a hook in core to notify other modules that content type 'page' is being merged into content type 'story'. With a hook, modules like CCK and taxonomy could do whatever they need to do to manage the changes that would be needed. With taxonomy, that would be to make sure that all 'page' vocabularies also get marked as being available for 'story'. With CCK we would have to add the 'page' fields to the 'story' content type, if they aren't already there. Any module invoking the hook would be able to prevent the merge if there is some reason those content types can't be merged. There's really no way a contrib module can do this right unless core provides a hook and a system to handle this in an organized way.

I think 1) and maybe 2) should be done now for 6.x, but 3) would be an API change that needs to be thought through and could only be added to 7.x as a new feature.

catch’s picture

Version: 6.1 » 7.x-dev
Priority: Normal » Critical

1 and 2 would be a very good start. They'd also help when deleting forums which has much the same issues (we falsely claimed that deleting forums would delete all posts in them for at least three releases, only managed to get a text change into D6).

1 and 2 apply equally to D7 so I'm moving this there, although it might be worth a backport. Also leaving orphaned data in the database via any admin operation probably ought to count as critical.

KarenS’s picture

I'd like to propose that we do #1 in D7 and backport that to both D6 and D5 as a critical bugfix.

I'd like to also do #2 in D7 and backport it to D6. It can't be backported to D5 because we don't have the batch engine in D5.

After that is done, we could consider adding #3 as a new D7-only feature.

Does that seem like a reasonable approach? Is there enough consensus on this to roll a patch?

catch’s picture

All three ought to work, and IMO this is important enough to backport (edit. for clarity, backport at least 1. maybe 2).

KarenS’s picture

Another note so I don't forget about it, the patch should include an update to clean up previously orphaned nodes. Since they were never properly deleted, there may be related info that also needs to be cleaned up by other modules, so instead of doing a brute force delete, the update might temporarily re-create the type (so it exists again and nodeapi will work), then do a batch process to delete each node with node_delete(), then again delete the type. Just thinking out loud -- not sure of the mechanics of getting it working.

yched’s picture

About cleaning up already orphaned nodes : Problem is that we lost all info about the actual deleted type(s), thus all we can do is recreate *one* temporary type to receive orphaned nodes, that might have been of different content types originally - not sure what might come out of this when we then delete these nodes.

KarenS’s picture

I was figuring we know the type name (from the node info), I guess I was figuring we find all the unique type names in the node file that don't have a type identified in the node_type table and just create a simple type with that name. Then let modules figure out if that type name means anything to them about what they need to clean up. But maybe you're right that it isn't safe to do that. Are there any core modules that might have other orphaned data that needs to be cleaned up (like Upload module -- will it have orphaned files)? If so, how else do we get that done when we can't use nodeapi? If there are contrib modules that need to do cleanup behind those nodes, how can they do it without using nodeapi?

[Edit] We already know the Forum module has garbage left behind that needs to be cleaned up, see http://drupal.org/node/206820.

moshe weitzman’s picture

FYI, when you try to delete an organic group (and have sitewide perms), the confirm form offers you the ability to move posts to a different group or delete the affiliated posts or leave the posts alone. This is analogous to whats suggested here.

metaface’s picture

It seems worth noting that you can also get this error in cases where the content type has not been deleted but has simply been disabled. I got this error in the following scenario:

I imported Drupal blog content from Drupal site A into Drupal site B. After the import, I could view the content in Drupal site B but got this "warning: call_user_func_array()..." error message when attempting to edit the content. It turned out that I simply had not enabled the Blog module in Drupal site B. Doing so fixed the problem.

danielb’s picture

Please allow the situation where I can delete a content type and cascade the delete to the nodes, with ease.

brianV’s picture

+1 subscribing

buckley’s picture

+1 subscribing

I just encountered this problem in D6

jwuk’s picture

+1 subscribing. Encountered several times. Strongly agree with #2 ("Also this isn't minor, and it's definitely a bug if you can do stuff in the UI which generates nasty error messages").

At the very least, meanwhile we need php snippets that will scan a site for orphan pages, so when we see this problem on one page we can find out if we need to hack other pages also.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » feature
Priority: Critical » Normal

The behaviour of leaving orphaned nodes behind, desired or not, has been present since at least Drupal 4.x. This sounds more like a feature request to me than a bug report. Bumping to 8.x.

jwuk’s picture

PHP error messages (puzzling to many people)... eradicating them is just a feature request? Webchick, I thought you were keen on the user experience, I'm disappointed. Surely ANY situation where a raw PHP error is displayed is a bug. For some it might not be a very urgent bug, but am I alone in thinking it's not a feature request?

I'm hesitant to change the settings back, but I really hope you can consider doing so.

catch’s picture

Version: 8.x-dev » 7.x-dev
Category: feature » bug

We should do one of two things:

a) prevent content types from being deleted via the UI (i.e. hide the edit links) if there are any nodes attached to them.

b) in the deletion confirmation screen, add a warning that the content type still contains some nodes.

I'd go for the former approach personally, should be an easy enough patch.

I'm also surprised by the status change. Core spitting PHP errors when you click around is the definition of a critical bug,

If we had a test for this (create a node, delete the node type, view the node), this would not have been bumped to 8.x, and a regression would have been considered critical. Just because bugs are old and hard to fix properly doesn't make them less critical than new ones.

webchick’s picture

Oh, ok. I got turned around by the replies later on that seemed to imply we wanted to add additional workflow options on the delete confirm form, not about PHP errors, which is of course very out of scope at this point.

a) sounds fine.

Sivaji_Ganesh_Jojodae’s picture

Subscribing.

This is some what out of scope but related to this issue. Similar problem exists with contrib modules (say quiz, faq etc) which implements additional node types. When the module is uninstalled the content type gets deleted and leaves its nodes, views, menu, etc. orphaned. I have already posted this in the developers list http://lists.drupal.org/pipermail/development/2009-April/032755.html haven't found any concrete solution for this yet.

jhedstrom’s picture

Status: Active » Needs review
FileSize
605 bytes

This patch implements catch's suggestion in #23 of removing the delete link if nodes of a content type exist.

catch’s picture

Status: Needs review » Needs work

Looks good, but shouldn't use COUNT for this kind of check, see http://drupal.org/update/modules/6/7#select_count

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
611 bytes

That makes sense. This patch uses db_query_range instead of count.

David_Rothstein’s picture

b) in the deletion confirmation screen, add a warning that the content type still contains some nodes.

Drupal already does this. See node_type_delete_confirm(), which displays this message:

%type is used by @count pieces of content on your site. If you remove %type, you will not be able to edit the %type content and it may not display correctly.

Given that already exists, what does the patch here really accomplish? It seems like it's kind of playing babysitter a bit. IMO there are actually legitimate reasons to delete a content type and leave some orphan nodes behind, as long as you know what you're doing. And if all we want to do is get rid of the PHP errors, then there is already an RTBC patch at #400450: Be more graceful when editing nodes with disabled node types which does that.

Also, preventing deletion in the content types UI doesn't solve the full problem. As others mentioned above, node types can be removed by modules too (when you disable the module that provided them, e.g. Blog), and all the same problems occur there as well. Maybe that could be a separate issue, but it seems to me like the correct solution should address both cases. And that second case is pretty important too; e.g. we have several "one-off content type" modules in core (like Blog), based on the theory that it's a basic implementation of a feature that's good for a simple site, but that you eventually might scale out of that module as your site gets more complex. However, it's tough to scale out of it if turning off the Blog module makes all your existing blog posts completely uneditable, right? :)

Couldn't we potentially fix this by adding a 'deleted' column to the {node_type} table in the database, and then never actually deleting content types, only marking them as deleted (and hiding them in the UI)? Then when we try to edit a node with the deleted type, we'd still have all the necessary data in place to be able to correctly construct the node form. Any reason that wouldn't work? It's maybe a bit heavy for D7, but on the other hand, it should be possible to do without breaking any APIs, and this is a relatively serious bug.

Gábor Hojtsy’s picture

My thinking was that it is not about the content type being deleted only but also the permissions going away defined for the content type if those were defined by the module. However looking at blog module that does not seem to define these permissions anymore. I guess it is falling back on node module providing the permissions in D7. So that might not be an issue anymore if we keep permissions manageable to deleted node types.

JacobSingh’s picture

eek... sticky topic.

I think that if a module creates a custom content type, it should be responsible for handling its removal / deletion / de-activation gracefully.

The best practice IMO would be to add a "disabled" field to the type's definition if the module is uninstalled. Of course if the module is just removed from the server... ?

Still, that leaves all those nodes kicking around, I imagine most people would want all Quizes on the site to vanish if they disabled the quiz module, right? How can we do that? Do we need status = -1 in the node table for sorta-deleted nodes? Status = 0 is used too frequently by modules, and admins can see them.

This thread has enumerated well all the permutations around what's desirable and there are quite a few, and no easy answer. Custom node types are defined mainly in contrib (and a few in core); we should provide tools perhaps for migrations, batch delete, etc at some point 8.x) but maybe the onus of helping the user decide what to do should be on the module coder?

-J

catch’s picture

In Drupal 8, I want to have {node} and {node_revision} as self-sufficient tables. So it would be possible to 'archive' a node by deleting it entirely from {node}, then at a later date restore it to that table if necessary (there's more pieces to this but they aren't relevant to this issue). Given #400450: Be more graceful when editing nodes with disabled node types we could consider just punting this to D8.

mikeytown2’s picture

quick question in regards to this patch #28.

http://api.drupal.org/api/function/node_overview_types/7
why do we still have the edit, manage fields, & manage display links? Should we get rid of these as well sense we got rid of the delete one?

kscheirer’s picture

How about simply offering an option on the confirm form to delete the content type and all its nodes?

Also, node_type_delete_confirm() has one of those bad SELECT COUNT(*) queries - that should be cleaned up similar to the patch in #28.

kscheirer’s picture

Status: Needs review » Needs work
tizzo’s picture

Assigned: Unassigned » tizzo

We're working on this at the sprint now!

sumitk’s picture

Assigned: tizzo » Unassigned
FileSize
7.19 KB

Adding support for revisions .. now when you visit node/%/revisions page you will see an error message-

"This content cannot be reverted to a previous revision because the content type drupal_issue has been disabled or removed. Edit content types."

Working more on it today .. at drupalcon core sprint!

tizzo’s picture

Currently once you have deleted a content type there is no way to filter the orphaned content to clean up the problem. This patch adds the deleted content types to the filter list (and marks them as such) so that they can be filtered for delete/unpublish/etc.

@sumitk is planning on adding a patch that would add links to delete the node, administer nodes of this type or administer content types (pending proper permissions)

tizzo’s picture

Improved patch, before you could end up with a PHP notice on the content page if you deleted all items for one of the content types you were filtering on (if that content type had previously been deleted).

sumitk’s picture

Cleaner version of #39

tizzo’s picture

Status: Needs work » Needs review

I think this stuff looks farily good, review anyone?

NicolasH’s picture

#40 brought the error addressed in #39 back for me.

tizzo’s picture

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.

David_Rothstein’s picture

Scenario: I create a node type called "blog" and start writing blog posts on my site. For two months, I write blog posts.

Then, I decide I don't want to write new ones anymore. I now consider the "blog" type to be cruft and don't want it showing up in on my list every time I go to create content, etc.

So now with this patch, the only way I can get rid of it is to also destroy all the old blog posts on my site?

(Or install some contrib module to work around it, I suppose.)

To me, this really sounds like a feature change, and it's not clear what bug it solves. We already warn people in the UI what will happen if they delete the content type, and there's a patch at #400450: Be more graceful when editing nodes with disabled node types to fix the PHP errors. If there other errors, they can/should be fixed too. I tend to think that content without a defined node type is something that Drupal should (in theory) be able to handle... maybe there are limits on what you can do with it, but it's not clear to me that it's a bug for it to exist? Content types are just ways of classifying nodes, after all.

David_Rothstein’s picture

I now consider the "blog" type to be cruft and don't want it showing up in on my list every time I go to create content, etc.

Actually, in the particular case of creating content, I guess I could just disable the corresponding menu item :) But I think the larger point still remains.

tizzo’s picture

The same items break if you delete a content type via the interface, this patch prevents users from doing that completely.

You are right, it does not solve the issue of uninstalling a module. The problem is that we need to remove an indeterminate number of nodes and node deletion can be an expensive operation.

What we really need to do is:

1. Perform a logical delete of the content type (setting a flag, not removing it)
2. Add all of the nodes to a new job queue to be deleted
3. When the queue has run out of nodes to delete, remove the content type itself

This seems like a big change to get into core right now and it should probably be in the entity api itself rather than an operation for nodes specifically...

yched’s picture

I tend to think that content without a defined node type is something that Drupal should (in theory) be able to handle

in D7, deleting a node type calls http://api.drupal.org/api/function/field_attach_delete_bundle/7, which deletes field instances --> your nodes will really start to look empty (body, taxo are fields too now)

tizzo’s picture

At #874000: Automatically remove module's nodes upon module uninstallation I created a patch that actually adds the appropriate nodes to a job queue for deletion via an updated node_delete_multiple which would fix both use cases (once the queue had time to process).

halstead’s picture

When progress is made here I (or anyone else) will most likely be able to solve or close #775878: Comment form body field disappears when content type deleted..

kr0k’s picture

Jody Lynn’s picture

FWIW, I end up having to delete these orphan nodes on older sites form time to time, with a little

drush php-eval '$result = db_query("select n.nid from node n left join node_type t on n.type = t.type where t.type is NULL"); foreach ($result as $record) {node_delete($record->nid);}'

(D7)

Jody Lynn’s picture

I support the patch in #43 and disagree with the legitimacy of the use case in #44- as yched points out if you delete a content type the orphaned nodes that result are completely broken.

It's a regular thing in my work to have to programmatically delete these orphaned nodes because they appear on sites as broken pages that can not be deleted through the UI. People seem to very commonly ignore the warning about deleting content types that have nodes and get their sites into this broken state. We shouldn't let them do an action through the UI that creates a state that cannot be fixed through the UI - the warning is just not enough.

Jody Lynn’s picture

I found some related suggestions in #348932: Options when deleting Content Type

chx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

I think we must not allow deleting a node type if there is content in it. What #46 forgets is that nodes pending deletion would need to be hidden from the admin and we simply has no capacity to hide a node from a node admin. #43 is the right thing to do.

yannickoo’s picture

Priority: Normal » Major

Why here isn't any activity? I think this isn't a small problem.

webchick’s picture

Priority: Major » Normal
kristiaanvandeneynde’s picture

IMO, the way we're going at it is good, but there is more work to be done.
So far we've only covered node type deletion through the UI.

Disabling the blog module also disables the blog node type by setting the 'disabled' flag in {node_type} to 1. This causes the same problem as described in the OP, because it also orphans the nodes. However, when you re-enable the blog module, the disabled flag is removed again and all your blog nodes automagically work again.

The same goes for every module that defines node types through hook_node_info().
(Not through node_type_save()).

How about a module-level workflow that unifies the clean-up for node types created through hook_node_info():

  • Install: Register node type
  • Enable: Enable node type (flag in {node_type})
  • Disable: Disable node type (flag in {node_type}), optionally hide nodes?
  • Uninstall: Remove node type, remove nodes

Note that, currently, uninstalling the blog module does not delete the blog node type.
It remains to haunt your database for all eternity.

If you want, I'll do the adjustments to node.module to achieve the above suggestion.
(Perhaps in a completely separate issue?)

tim.plunkett’s picture

Issue tags: +drupaldelphia2012

Tagging for the sprint

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
disasm’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: -Needs backport to D7, -drupaldelphia2012
FileSize
1.73 KB

Attached patch is a re-roll. Still working on patch.

aspilicious’s picture

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

Leave it at D8. Bugs should be fixed first in d8.

disasm’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: +Needs backport to D7, +drupaldelphia2012
disasm’s picture

Version: 7.x-dev » 8.x-dev
djdevin’s picture

Assigned: Unassigned » djdevin
djdevin’s picture

Assigned: djdevin » Unassigned
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
2 KB

Added a test to #60 while working with disasm, dkinzer, and Kinano.

tim.plunkett’s picture

Issue tags: -Needs tests

Removing tag. Patch still applies.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

xjm’s picture

Minor cleanups:

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -163,4 +163,30 @@ class NodeTypeTest extends NodeTestBase {
+   * Test deleting a content type before and after all content has been removed.

Should say "Tests...". The comment may need to be reworded slightly to fit 80 characters.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -163,4 +163,30 @@ class NodeTypeTest extends NodeTestBase {
+    // Create a content type programmaticaly.

"Programmatically" is misspelled.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -163,4 +163,30 @@ class NodeTypeTest extends NodeTestBase {
+    // Login a test user.

"Log in" should be two words when it's a verb.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.phpundefined
@@ -163,4 +163,30 @@ class NodeTypeTest extends NodeTestBase {
+  }
 }

There should be a blank line between the final method and the end of the class.

no_commit_credit’s picture

Sorry for the noise; something goofy going on with uploads.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yep, makes sense. You can still get in this state with disabled modules, but at least it's one less way for people to shoot themselves in the foot.

Committe/dpushed to 8.x, not sure if this is backportable to 7.x with the string change but that's up to David.

kid_icarus’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.7 KB

So this is my first shot at backporting this.

I tested the functionality and it's there. I'm uncertain about the test though, it failed for me locally. Let's see how the bot feels about it.

kid_icarus’s picture

Here's a tests only patch as requested by @xjm and @Cottser :)

Status: Needs review » Needs work

The last submitted patch, node-232327-76-tests-only.patch, failed testing.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
519 bytes
3.71 KB
2 KB

Ok, so I replaced a call to Entity::delete() with node_delete() and the failing test passed for me locally, hopefully that does the trick.

Sorry for messing up the thread, hopefully I got the patch naming/ordering conventions right :)

YesCT’s picture

#79: node-232327-79.patch queued for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
9.01 KB
17.72 KB

I tested #79. The patch works to correct the error as described in the OP.

Before the patch, I encountered the error shown in the image below.

before.png

After the patch, I received the warning message shown below when attempting to delete the content type.

after.png

This looks RTBC to me.

yannickoo’s picture

So cool that we can get this feature into 7.x!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
+    $caption = '<p>' . format_plural($num_nodes, '%type is used by 1 piece of content on your site. You can not remove this content type until you have removed all of the %type content.', '%type is used by @count pieces of content on your site. You may not remove %type until you have removed all of the %type content.', array('%type' => $type->name)) . '</p>';

Why are the single/plural cases using completely different wording? (Also, "can not" should be "cannot", assuming that one is kept.)

This issue affects Drupal 8 too - could be spun off to a followup issue but we have to get it fixed first since we don't want to break strings twice in Drupal 7.

***

On the overall issue, I guess @yched makes a good point in #47... perhaps the ship has sailed and we should do this in Drupal 7 after all.

However, we're talking about a string change and behavior change in the middle of a stable release. Meanwhile, the PHP warning shown in the above screenshot (which is what started this issue in the first place) should be solved by the simpler patch at #400450: Be more graceful when editing nodes with disabled node types as well (and furthermore, that solves other cases too, such as existing nodes where the content type was already previously deleted).

Shouldn't we really try to get that one in first and see how things go after doing that?

fengtan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.73 KB

Re-rolled #79 to make single/plural messages consistent.
Agree #400450: Be more graceful when editing nodes with disabled node types might be a simpler solution though.

royal121’s picture

Status: Needs review » Needs work
FileSize
11.88 KB

Patch in comment #84 seems to work fine for me. Here is a screenshot: afterpatch

One thing which I think that should be changed more is the heading. It still says:

Are you sure you want to delete the content type x?

But we aren't giving them an option to delete the file.

pwolanin’s picture

related/dup possibly this should be marked duplicate #104572: collect "garbage" if a node type is deleted even though it's older since this has the 8.x commit.

kristiaanvandeneynde’s picture

@pwolanin I don't think they're duplicates:

  • This issue is about ghost nodes left behind when deleting a node type
  • The other issue is about database garbage left behind when deleting a node type

  • catch committed 292a4c5 on 8.3.x
    Issue #232327 by tizzo, jhedstrom, sumitk, NicolasH, disasm, tim....

  • catch committed 292a4c5 on 8.3.x
    Issue #232327 by tizzo, jhedstrom, sumitk, NicolasH, disasm, tim....

  • catch committed 292a4c5 on 8.4.x
    Issue #232327 by tizzo, jhedstrom, sumitk, NicolasH, disasm, tim....

  • catch committed 292a4c5 on 8.4.x
    Issue #232327 by tizzo, jhedstrom, sumitk, NicolasH, disasm, tim....
hgoto’s picture

Status: Needs work » Reviewed & tested by the community

This issue seems to be old but I'd like to move ahead with this. (Using Entity reference fields, I met this issue.)

The patch #84 can be applied to the latest D7 dev cleanly. It works well and I cannot find any problem with the code. As for the point of #85, the same title is already used in D8 and it's better to discuss it in a follow-up issue, I believe. So I'd like to mark this RTBC :)

Parashram’s picture

FileSize
45.47 KB

Patch #84 works well but as per comment #85, Its heading should be different as we are not going to delete CT. and also i think, there shouldn't be shortcut icon. Because it has no mean to be here.

Parashram’s picture

Pol’s picture

Issue tags: -Needs backport to D7 +Drupal 7.60 target

This looks indeed perfectly fine.

The most amazing thing is that the patch is from late 2013 and it can still be applied without any single issue... incredible.

Pol’s picture

Issue tags: +Pending Drupal 7 commit
joseph.olstad’s picture

Version: 7.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Drupal 7.60 target, -Pending Drupal 7 commit

marking this as fixed, it was fixed in 8.3.x a while back.

as per backport policy, a new issue for D7
still RTBC for the new issue, no change
Removed tags and placing them on the new D7 backport issue.

#3008170: [D7] Deleting node type leaves orphan nodes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.