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_%%'");
}
CommentFileSizeAuthor
#4 url_replace_filter.install.patch1.1 KBfgm

Comments

David Lesieur’s picture

David Lesieur commented

We need this. +1!

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
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
David Lesieur’s picture

David Lesieur commented
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 (//).
fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Needs work » Needs review
StatusFileSize
new url_replace_filter.install.patch1.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.

David Lesieur’s picture

David Lesieur commented

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.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

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 ?

David Lesieur’s picture

David Lesieur commented
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?

David Lesieur’s picture

David Lesieur commented
Status: Reviewed & tested by the community » Needs review

Oops, changed status by mistake.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

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

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.

David Lesieur’s picture

David Lesieur commented
Status: Needs review » Reviewed & tested by the community

Looks like there is enough solid evidence. :)
Let's follow the masses.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Reviewed & tested by the community » Fixed

OK, patch applied with ref to this thread. On to the other patch, now ...

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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