API page: http://api.drupal.org/api/drupal/core%21includes%21module.inc/function/m...

The code docs for module_uninstall()() need improvement. They have at least the following issues:

  • They don't indicate that modules must be disabled first or what will happen if they aren't.
  • They don't specify all @param or @return types.
  • They don't specify the parameter defaults.
  • They don't @see closely-related functions.
  • Language issue: "dependent" should be "dependant" when referring to a noun. ("Dependent" is an adjective.)
  • And a couple of other minor things.

Patch to follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Assigned: TravisCarden » Unassigned
Status: Active » Needs review
Issue tags: +Novice
FileSize
2.72 KB
jhodgdon’s picture

Status: Needs review » Needs work

Good idea for improvement!

I have a couple of thoughts/questions:

1. My dictionary actually lists dependent as both adjective and noun, and dependant is not even its own entry (it's just a variant spelling for the noun). So I'm not sure about this change... The Firefox spell checker also lists "dependant" as a spelling error. :) Let's not make that change.

2. I don't think that:
"Returns FALSE if ... or TRUE otherwise"
is any clearer than (actually I think this second one is clearer:
"Returns FALSE if ...; TRUE otherwise."
(note: should be ; not , in there!) Actually, maybe it would be clearer to say essentially "Returns TRUE if everything is OK, and FALSE otherwise"? And as I read that, I wondered if that return value was only if the second arg was set to TRUE (since the docs there imply this checking only takes place if that arg is TRUE)? It would be good if the return docs made that clearer? [Looked at the code; it does appear that the function returns TRUE if you don't ask for checking, and only returns FALSE if you asked for checking and the check fails.]

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Thanks!

  1. Sounds good. We'll go with your dictionary. ;-)
  2. Boy, that seems like a complicated return condition. Maybe something like this would be clearer:

    Returns TRUE if the operation succeeds or FALSE if it aborts due to an unsafe condition (i.e. $uninstall_dependents is TRUE and a module in $module_list has dependents which are not already uninstalled and not also included in $module_list).

    That still seems burdensomely complex, but I don't know if you can eliminate the "i.e." bit without begging the question of what constitutes an unsafe condition and forcing the reader to read the whole function anyway.

  3. Also, I just noticed that the comment right before the invocation of hook_modules_uninstalled() lies about the name of the hook it's calling. I removed the explicit reference from the comment since the code is self-explanatory by itself.
TravisCarden’s picture

Oh... and it should indicate that the $uninstall_dependents param is optional. Technically the $module_list param is optional, too, but it doesn't make much sense to omit it. Do we leave it alone?

jhodgdon’s picture

I agree that it seems pointless to say you can omit $module_list. It's doubtful there is a reason to call this function unless it is there. ?!

This patch is OK. Personally I always try to avoid using i.e. in documentation, because about half of the people out there seem to think it means "for example" when it actually means "that is", and it is nearly always incorrectly punctuated (as it is in the latest patch, actually -- it requires a comma after i.e.). So if you would like to change that, I would be grateful. :) Other than that little detail, I think it's all very clear and you've done a good job clearing up many mysteries in this function. Thanks!

Actually, better yet: don't use parens in that @return. You could say for instance: "... unsafe condition. This will only occur if $uninstall_dependents is TRUE...". That might make it really really clear what the return value is?

TravisCarden’s picture

What would you think of "...unsafe condition, namely, $uninstall_dependents..."? Attached is a patch with it that way and one with it exactly as you proposed. I'll be thrilled with either one. :) Thanks so much!

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

I like your "namely" wording, so let's go with the "namely" patch. I'll get it committed shortly unless one of the other committers beats me to it. Thanks!!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x, thanks again!

It looks like we should probably port this to the Drupal 7 drupal_uninstall_modules() function, which I believe is pretty much the same and definitely has the same docs.
http://api.drupal.org/api/drupal/includes!install.inc/function/drupal_un...

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
TravisCarden’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
1.13 KB
2.63 KB

Yup; the two versions are functionally identical—just a little CMI spice in the d8 version. (A diff of the two patches is attached just to show that else is different between them.)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice

The last submitted patch, drupal-module_uninstall-1980072-10.patch, failed testing.

TravisCarden’s picture

Status: Needs work » Needs review
Issue tags: +Novice
jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! I'll get this committed soon, unless someone else beats me to it.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs work
+ * Modules that are still enabled will be silently ignored.

I don't think this is correct. When an enabled module is passed in to the function, the function still uninstalls it... but it is never disabled. So the module's database tables and everything else will be removed, but the module's code will still try to run afterwards!

Obviously that's not good, but for the purposes of this issue I think we should just document that it's the caller's responsibility not to pass enabled modules in.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
809 bytes

Here's a patch for Drupal 8.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Wow, that's correct. I think the patch(es) above might have been intending to say that module that had already been uninstalled will be ignored, but even that is only true if the 2nd arg is set to TRUE, so probably the patch here is the right thing to do. Thanks! I'll get it committed shortly and then we'll need another D7 patch.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Latest patch needs port for D7 for function drupal_uninstall_modules().

David_Rothstein’s picture

Status: Patch (to be ported) » Needs review
FileSize
760 bytes

Drupal 7 patch.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this one committed shortly, unless you want to commit your own patch. :)

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed -- thanks again for your diligence in noticing and fixing this error!

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