We need to implement hook_modules_uninstalled() and/or hook_node_type_delete() to make sure we remove unused configuration variables (from the variable table). See #1050424: D7 install updates.
Right now we only remove node type specific variables in scheduler_uninstall() for node types that are returned by node_type_get_types().

CommentFileSizeAuthor
#9 1052628_9.scheduler.delete_variables_when_node_type_deleted.d6.patch994 bytesjonathan1055
#1 hook_node_type_delete-1052628-1.patch893 bytesjaydub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jaydub’s picture

jaydub CreditAttribution: jaydub commented
Status: Postponed » Needs review
FileSize
hook_node_type_delete-1052628-1.patch893 bytes

Patch attached. I basically just copied the hook_uninstall code and added the aforementioned node type delete hook.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented
Status: Needs review » Reviewed & tested by the community

Looks good and I've tested it. Does exactly as required. The seven variables were being left before, but now they are deleted. Also checked the patch against coder review - all clean.

Just one question - when trying to apply your patch I had to delete the a/ and b/ from infront of
--- a/scheduler.module
+++ b/scheduler.module

I apply patches using 

patch -b -p0 -Vt < hook_node_type_delete-1052628-1.patch

Is there a different way you can produce patches from git which is consistent with non git users?

jaydub’s picture

jaydub CreditAttribution: jaydub commented

I just created the patch the way git patches are documented in every module's version control information page which for scheduler is:

http://drupal.org/project/scheduler/git-instructions

A git patch can be applied by: git apply -v PATCH

I think you could apply with patch by using -p1 instead of -p0 in this case and would still work.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented

Ah, thanks for the info. I wasn't meaning to criticise, re-reading my post it did sound like that. Sorry.

Just tried with -p1 and it works correctly without editting the path. I should have investigated that myself. Cheers.

rickmanelius’s picture

rickmanelius CreditAttribution: rickmanelius commented

Hi Everyone. This was committed: http://drupalcode.org/project/scheduler.git/commit/099fe14. I ran my own series of tests and it works as expected.

rickmanelius’s picture

rickmanelius CreditAttribution: rickmanelius commented
Status: Reviewed & tested by the community » Closed (fixed)
rickmanelius’s picture

rickmanelius CreditAttribution: rickmanelius commented
Status: Closed (fixed) » Fixed

I prematurely set this to "closed (fixed)". Setting to fixed.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented
Version: 7.x-1.x-dev » 6.x-1.9
Status: Fixed » Patch (to be ported)

I think this fix should be ported to 6.x.
hook_node_type_delete() is the D7 version of the old way in D6 which uses hook_node_type() with $op='delete'.
I'll make a patch for it.

Jonathan

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented
Status: Patch (to be ported) » Needs review
FileSize
1052628_9.scheduler.delete_variables_when_node_type_deleted.d6.patch994 bytes

Here is a patch for D6.

rickmanelius’s picture

rickmanelius CreditAttribution: rickmanelius commented
Status: Needs review » Reviewed & tested by the community

Looks good. Will get into 6.x-1.10

rickmanelius’s picture

rickmanelius CreditAttribution: rickmanelius commented
Status: Reviewed & tested by the community » Fixed

Committed
http://drupalcode.org/project/scheduler.git/commitdiff/b215975?hp=49f45a...

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented

Thank you.

Status: Fixed » Closed (fixed)

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