URL replace filter should include an uninstall feature [#255332]

The current version leaves data in the DB when uninstalled. The code below is a possible implementation of url_replace_filter.install, and the attached patch is for the README.txt file.

The code assumes my previous patch from http://drupal.org/node/255314 has been applied, to include the "absolute" setting.

/**
 * url_replace_filter module : installer
 *
 * @version $Id$
 */
function url_replace_filter_uninstall() {
  /**
   * Delete module settings
   */
  variable_del('url_replace_filter_absolute');

  /**
   * Delete filters from content types
   */
  db_query("DELETE FROM {filters} WHERE module = 'url_replace_filter'");

  /**
   * Delete replacement rules
   */
  db_query("DELETE from {variable} WHERE name LIKE 'url_replace_filter_%%'");
}

Comment	File	Size	Author
#4	url_replace_filter.install.patch	1.1 KB	fgm
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

David Lesieur CreditAttribution: David Lesieur commented 8 July 2008 at 16:33

We need this. +1!

Comment #2

fgm

French

Paris, France

CreditAttribution: fgm commented 9 July 2008 at 08:02

Assigned:	Unassigned	» fgm
Status:	Needs review	» Fixed

Removed the variable_del, which is no longer necessary after removal of the setting.
Updated format to pass coder review
Committed to DRUPAL-5 branch

Comment #3

David Lesieur CreditAttribution: David Lesieur commented 9 July 2008 at 18:45

Status:

Fixed

» Needs work

Thanks! Some comments:

The description for url_replace_filter_uninstall() does not seem appropriate — it says "installer" but this is the uninstall hook... (i.e. Doxygen-style comments apply to the element that follows, so that particular comment actually applies to the function, not to the whole file as the description seem to imply). A more common comment for this function would be: "Implementation of hook_uninstall()".
Comments within a function's body should not use the Doxygen syntax (/** */) but just be simple PHP comments (//).

Comment #4

fgm

French

Paris, France

CreditAttribution: fgm commented 10 July 2008 at 10:35

Status:

Needs work

» Needs review

File	Size
url_replace_filter.install.patch	1.1 KB

Actually, the comment you mentioned was supposed to be the file-level phpdoc, but it was ill-placed for this. I now split the comment in three:

one for the CVS Id, which coder.module wants to have on a // line on its own
the file-level description for api.module
the function description, completed

I checked the result in api.module and it seems to look good. Also, even with "minor" warnings enabled in coder.module, there is no warning.

Comment #5

David Lesieur CreditAttribution: David Lesieur commented 11 July 2008 at 18:28

Nice!

However, I don't think the "@return void" is really useful. It amounts more to noise than anything else. ;) We don't usually see this in Drupal core's documentation.

Comment #6

fgm

French

Paris, France

CreditAttribution: fgm commented 11 July 2008 at 19:34

We probably have an ambiguity in d.o. practice on this. Look at devel.module, for instance, which is maintained by some of the top drupal coders, IMHO: 5 functions have "@return void".

In addition, when you are using an IDE like Zend Studio or Eclipse PDT, it *is* actually useful, because it shows you in the outline that the function is actually a procedure instead of showing it as returning unknown. Not even adding the fact that it seems more logical not to break regularity in documentation (least astonishment principle).

And the doc at http://drupal.org/node/1354 is actually incorrect when it says After all the parameters, a @return directive should be used to document the return value because it adds if there is one., since there is always a return value in PHP, even if it is NULL.

But I'll agree core doesn't do this. So, as the project maintainer, it's up to you: core compliance, or best practices ?

Comment #7

David Lesieur CreditAttribution: David Lesieur commented 11 July 2008 at 21:11

Status:

Needs review

» Reviewed & tested by the community

Hehehe... You have some good points and I don't have strong feelings about either way. Let's keep that documentation.

However, although "void" is defined in some languages, it is not a value in PHP. If we want to document that return value, shouldn't we just use NULL then?

Comment #8

David Lesieur CreditAttribution: David Lesieur commented 11 July 2008 at 21:12

Status:

Reviewed & tested by the community

» Needs review

Oops, changed status by mistake.

Comment #9

fgm

French

Paris, France

CreditAttribution: fgm commented 12 July 2008 at 21:10

Actually, this is how functions returning NULL are really documented elsewhere in PHP-land.

devel.module
PHPdoc
PEAR (and Drupal coding standards are supposed to follow PEAR standards)
Zend Framework
Doctrine ORM (which chx wanted Drupal to use some time ago for the new DB API)

FWIW, grepping contrib branch DRUPAL-5 shows @return void to be present at 161 occurrences, spread over 53 files in 32 modules. In comparison, @return null happens only 8 times and @return NULL just once.

For DRUPAL-6-1, the respective numbers are 274 occurrences / 77 files / 40 modules vs 13 occurrences / 11 files / 11 modules only, so it seems there is a trend towards @return void.