Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | module-uninstall-1980072-19.patch | 760 bytes | David_Rothstein |
#16 | module-uninstall-1980072-16.patch | 809 bytes | David_Rothstein |
#10 | drupal-module_uninstall-1980072-10.patch | 2.63 KB | TravisCarden |
#10 | interdiff.txt | 1.13 KB | TravisCarden |
#6 | drupal-module_uninstall-1980072-6-namely.patch | 2.71 KB | TravisCarden |
Comments
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedComment #2
jhodgdonGood 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.]
Comment #3
TravisCarden CreditAttribution: TravisCarden commentedThanks!
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.
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.Comment #4
TravisCarden CreditAttribution: TravisCarden commentedOh... 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?Comment #5
jhodgdonI 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?
Comment #6
TravisCarden CreditAttribution: TravisCarden commentedWhat 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!
Comment #7
jhodgdonI 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!!
Comment #8
jhodgdonCommitted 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...
Comment #9
jhodgdonComment #10
TravisCarden CreditAttribution: TravisCarden commentedYup; 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.)
Comment #12
TravisCarden CreditAttribution: TravisCarden commented#10: drupal-module_uninstall-1980072-10.patch queued for re-testing.
Comment #13
jhodgdonLooks good, thanks! I'll get this committed soon, unless someone else beats me to it.
Comment #14
jhodgdonThanks again! Committed to 7.x.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a patch for Drupal 8.
Comment #17
jhodgdonWow, 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.
Comment #18
jhodgdonCommitted to 8.x. Latest patch needs port for D7 for function drupal_uninstall_modules().
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7 patch.
Comment #20
jhodgdonThanks! I'll get this one committed shortly, unless you want to commit your own patch. :)
Comment #21
jhodgdonCommitted -- thanks again for your diligence in noticing and fixing this error!