There is no hook_uninstall to delete all the variables created by this module. I'd suggest implementing hook_uninstall for the cleanup. find a batch attached for that

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mark Trapp’s picture

Title: Clean Up » Clean up variables when no longer in use
Version: 7.x-1.1 » 7.x-1.x-dev
Category: Bug report » Task
Status: Active » Needs work
Issue tags: -install uninstall missing, -hook_uninstall, -No uninstall code, -uninstall problems

This is a good idea, and thanks for providing a patch!

Just a few housekeeping notes:

  • Whenever you have a patch ready in an issue, mark an issue as "Needs review".
  • This is a task, not a bug with functionality.
  • Issue tags are used for initiatives and things that span multiple issues: your issue name and description are more than adequate to describe the issue.

Now for your patch:

  • This will delete variables for existing node content types, but what about content types that are deleted before Date Popup Authored is uninstalled? Probably makes sense for them to be deleted when a content type is deleted.
  • Try to match the existing code style and Drupal's coding standards. There's a missing space between <?php and the top docblock, and there's a missing blank line at the end of the file.
  • Doc standards: top docblock should read "Install, update, and uninstall functions for the Date Popup Authored module."
amagdy’s picture

Thank you for clearing this up.

I've been putting too many hours into fixing my formatting but I still can't get it 100% correct.
Right now I've PhpStorm with phpSniffer configured with Drupal Standards from Coder Module.

It helps me this way https://www.dropbox.com/s/87blsug320d1sfv/Screenshot%202014-11-24%2010.4...

I wonder what tool do you use to help you with formatting? or do you just memorize every single rule :D

  • Mark Trapp committed 1d9310f on 7.x-1.x
    Issue #2329773 by amagdy, Mark Trapp: Clean up variables when no longer...
Mark Trapp’s picture

Hey, sorry for the late reply!

My process for following the Drupal coding standards has been to eventually memorize them through repetition and practice, and give my code a once-over comparing it to the docs before committing. Your patch was almost there: the stuff I mentioned in my previous comment are the only parts where your patch deviated.

I did another review, and besides the variable cleanup during content type removal, we need tests for this change. I went ahead and added both, then committed the final result. I attached two patch files: one with just the tests and one with everything—here so you can take a look.

The changes will be part of the next beta that I plan on rolling in a day or two.

Thanks again for the good idea and patch!

Mark Trapp’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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