The patch deletes all page title variables in once after uninstalling the module. It also uses the D7 database abstraction layer API. What do you think ?

CommentFileSizeAuthor
page_title_uninstall.patch1.25 KBhles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicholasThompson’s picture

nicholasThompson
Primary language English
CreditAttribution: nicholasThompson commented
Status: Needs review » Needs work

I have toyed with this idea before.

We assume that any variable prefixed with page_title is related to the Page Title module. Is this safe? What if another module overlaps our namespace? Yeah, that's unlikely (and stupid of the other module), but does that give us to right to wipe their settings?

An alternative is to migrate all settings into a single variable - much the same way GlobalRedirect has recently done. See here (GlobalRedirect.module 1.1.2.4.2.5.2.32 line 322).

This has several advantages:

  1. We can easily define defaults in a single place (no more hunting around for every use of a variable).
  2. Install, update and uninstall become easy - everything is in one place.
  3. It gives us scope to expose Page Title settings via an API. Wouldn't it be cool if any module could expose and provide its own default settings to Page Title? It also means we could move settings into specific includes (node.page_title.inc, etc)

Arguably this is a separate conversation to the "DELETE FROM {variables} WHERE name LIKE "page_title%""... but I think it's something we should be working towards.

hles’s picture

hles CreditAttribution: hles commented

page_title namespace might be more subject to overlaps than other modules because page and title are very common words, so i think it was not such a good idea after all.
I didn't know about that GlobalRedirect implementation, sounds good to me. As this is a minor issue because uninstall just works fine, i guess there's no hurry to work on that then.

nicholasThompson’s picture

nicholasThompson
Primary language English
CreditAttribution: nicholasThompson commented
Title: Maybe we can reduce code for uninstall » Change Page Title Preferences to a single variable.
Category: task » feature
Priority: Normal » Major
Status: Needs work » Active

For reference: http://drupalcode.org/viewvc/drupal/contributions/modules/globalredirect...

Advantages:

  • Upgrading becomes easier as settings become centralized - no more hunting for every use of variable_get
  • Uninstalling becomes easier as you only need to drop one variable.
  • We can expose settings as an API hook.
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I'm not a huge fan of this for uninstall. It can potentially clobber modules that extend page_title, and doesn't follow the pattern of core by taking care of your own variables individually (no core module uninstalls their variables this way).

Another side note is that if you merge your variables into one giant array variable then it becomes significantly harder if not impossible to override variables using the $conf global in your site's settings.php file.

nicholasThompson’s picture

nicholasThompson
Primary language English
CreditAttribution: nicholasThompson commented

Ah. Yes. That is a fair point. I'd not considered the impact on $conf. Would it be sensible to look for 'page_title_conf_override' in variable_get as a key which should only be set from settings.php?