Currently if a node type is deleted, extraneous entries are left behind in the variable table by modules like comment and upload. This is probably not a real problem except if sites add and delete a lot of node types.

Two possible approaches to cleaning up the variable table when a node type is deleted:

1) Each module that adds elements to the node type form should implement hook_node_type and clean up when they get the 'delete' $op. This is the simplest, in a sense, but more burden on contrib module authors to get it right.

2) The node module should do its best to clean up automatically.

Patch implementing approach #1 attached for review.

Note, this is a relatively atomic bit of the issues originally discussed here: http://drupal.org/node/88633

CommentFileSizeAuthor
#21 collect_garbage_if_a-104572-21.patch3.84 KBparashram
#15 extra_vars-104572-15.patch1.55 KB13rac1
#11 104572.patch1.72 KBdouggreen
#6 type_deletion_cleanup6x_3.patch2 KBpwolanin
#4 type_deletion_cleanup6x_2.patch1.78 KBpwolanin
#2 type_deletion_cleanup_2.diff1.8 KBpwolanin
type_deletion_cleanup_1.diff2.21 KBpwolanin

Comments

Jaza’s picture

Jaza commented

pwolanin, although I'm all for approach #1, it's not going to get committed. Dries indicated very clearly in http://drupal.org/node/88633 that he hates hook_node_type(), so there's no way that he's going to let more implementations of it get committed.

Therefore, in the interests of getting this bug fixed (albeit not in the cleanest way), could you please modify your patch to implement approach #2 instead. You can probably once again use the node_type_excluded_keys mechanism that you introduced earlier in http://drupal.org/node/88633.

pwolanin’s picture

pwolanin commented
StatusFileSize
new type_deletion_cleanup_2.diff1.8 KB

Ok, here is a simplified version of approach #2 (automatic cleanup)- the keys used to generate the variable names are themselves saved as a variable, and then used to delete variables if the type is deleted.

pwolanin’s picture

pwolanin commented

I think #2 is the way to go based on the current API- the array_merge() to keep a list of all past variables may not totally be necessary, but I think it insures a maximum of garbage collection.

pwolanin’s picture

pwolanin commented
Version: 5.x-dev » 6.x-dev
StatusFileSize
new type_deletion_cleanup6x_2.patch1.78 KB

Patch re-rolled for HEAD (previous one applies fine, but with offset).

Any reason this is not RTBC?

pwolanin’s picture

pwolanin commented

bump - needs to be re-rolled

pwolanin’s picture

pwolanin commented
StatusFileSize
new type_deletion_cleanup6x_3.patch2 KB

Patch re-rolled for HEAD (previous one applies fine, but with offset).

catch’s picture

catch
he/him
Primary language English
commented

Still applies with offset. My variables table contains all kinds of cruft (> 1000 rows) so I'm all for cleanup, but not qualified to review the code.

pwolanin’s picture

pwolanin commented

I guess also the question of whether this is worth while to do depends on whether node types are ever deleted on a typical site.

dman’s picture

dman commented

I vote for it being worthwhile, because although not typical, the UI does allow it to happen. But actually doing so puts the site into an unstable state that can't be repaired without messing with the DB. Thus, repairing the DB when appropriate is a good thing.

catch’s picture

catch
he/him
Primary language English
commented

Especially when starting off a site for the first time, you can be making and deleting content types quite a lot. Also, if you had two content types and wanted to merge them into one, it'd be handy to have a two-click solution rather than update node set type where ....

douggreen’s picture

douggreen commented
Version: 6.x-dev » 7.x-dev
StatusFileSize
new 104572.patch1.72 KB

I also think this is needed after discovering that my site, after a year of development, has 500 of these variables cluttering up the variable table. Patch still applied, albeit not cleanly, here's the one re-rolled for 7.x.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

#11: 104572.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 104572.patch, failed testing.

13rac1’s picture

13rac1 commented
Status: Needs work » Needs review
StatusFileSize
new extra_vars-104572-15.patch1.55 KB

Re-rolled for current dev.

alan d.’s picture

alan d. commented

This may have to be rolled against 8.x first. Has anyone checked?

thedavidmeister’s picture

thedavidmeister commented
Issue summary: View changes
Status: Needs review » Needs work

yes, can we check if this is an issue in D8? but also, can we write some tests for this regardless of what version we roll against?

dcam’s picture

dcam commented
Related issues: +#1933374: Deleted content types leave a bunch of variables behind

Added #1933374: Deleted content types leave a bunch of variables behind as a related issue. There's almost certainly some duplication of work between the two issues, but I didn't check thoroughly.

parashram’s picture

parashram commented

Yes, https://www.drupal.org/node/1933374 is same like this issue, So following variables should be

additional_settings__active_tab_NODETYPE
language_content_type_NODETYPE
menu_options_NODETYPE
menu_parent_NODETYPE
save_continue_NODETYPE

Thanks:
Parashram

parashram’s picture

parashram commented
Assigned: Unassigned » parashram
parashram’s picture

parashram commented
StatusFileSize
new collect_garbage_if_a-104572-21.patch3.84 KB
parashram’s picture

parashram commented
Assigned: parashram » Unassigned
parashram’s picture

parashram commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: collect_garbage_if_a-104572-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

parashram’s picture

parashram commented

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.