Need a UI for hook_uninstall()

I like this approach.

If uninstall is its own page, we can probably use checkboxes to be consistent with the normal module administration page.

i propose that we use merlin's suggestion, and build this now as a separate module. worst case scenario is that it will be available in contrib for those who want it.

we need to get this done soon, b/c it sure would be nice to have it available for testing w/ the betas...

I put together a very first version of an uninstall ui patch. It lacks a confirm page atm, but all the rest works nicely.

i've got one better--here's a complete tested and working uninstall module, with confirm screens.

I agree with webchick. Uninstall is core functionality and must not be an optional module IMO.
The patch is the completed version from patch for system.module above (incl. messages/confirm page/...). I'm not a native speaker, thus help text and messages need work.

alrighty then, in core it is!

both our versions had some good ideas, so this patch combines them and puts the functionality in system.module as requested. Patch has been tested on a clean HEAD and appears to work perfectly. Code notation and doxygen included... :)

Really nice, hunmonk :) There are just some minor issues:
- wrong path for canceled uninstall => should be 'admin/build/modules/uninstall'
- confirm page title is much too long ('WARNING: The following modules ...'')
- existence of .info file is not essential for being able to uninstall a module (although without it the module wouldnt get listed under modules)
- 'No modules are available to uninstall.' should be a 'empty table' message. Thats more consistent with e.g. taxonomy term tables.

"// If hook exist, the module can be uninstalled." should be "// If the hook exists, the module can be uninstalled."

also, "t('confirm modules uninstall')," is a bit clunky. i think just "confirm uninstall" would be better english. also, shouldn't the first word be capitalized? not sure on that...

nice catches on the other stuff!

Comments/messages corrected. Thanks.

so i got to thinking that this stuff belongs in install.inc--no need to load it for every page! system module now only contains the menu calls, help, and a small wrapper function that includes install.inc and invokes the uninstall stuff.

also fixed those two small grammer errors i noted above.

once again patch tested and working on latest HEAD.

On code organization- install.inc looks to just have a bunch of API functions. The added code isn't API, but code for user interfaces. I think user interface code (which is specific to the system module) belongs somewhere in the system module's directory, either in system module or a conditionally-included file.

The uninstall UI stuff is not that much code, so I think it should be in system.module directly OR we should move all modules-related stuff (admin modules (enable/disable) + uninstall modules) into an include file, e.g. system_modules.inc. I guess we should prefer the include file, because the modules-related code is a large piece in system.module now and it is used only on the two pages.

@hunmonk: Please inform me in case you agree but you cant do the .inc stuff now.

I concur on the conditional include, and think that's probably true for the bulk of the administrative pages as well, and starting on such a path here would be a good thing.

You probably want to do just the uninstall stuff in a conditional include; if that's accepted, a followup patch could move other system.module administrative pages into that same include.

the code from my last patch is portable to any other include file--just move it there and change the include statement in the wrapper--then we'll need just a couple more include statements in the uninstall functions to grab the install.inc file when it's needed. i like the idea of another include file in the system module--if somebody will throw a name my way for the name of that include, then i'll take two minutes and change it over for the uninstall stuff as our starter. admin.inc?

lemme know...

after some discussion in IRC, i gathered that system.inc in the system folder was the way to go, so here is the adjusted patch-- tested and working on current HEAD.

i only moved the uninstall stuff over there for now. after this lands, then we can start moving over other admin functions.

Took a quick spin of this for aggregator. Works nicely.

One bug I found. If you don't select any modules to uninstall, you get the "No modules selected." error and then if you click a module from THAT page and submit, the form does nothing. Clicking the module again from the form without the error msg, works.

Also, I think the warning messages need be more blatant and in your face as this is a serious operation. I guess only giving 'uninstall' perms to the super admin will alleviate that, but really driving home the fact that THIS WILL DELETE YOUR DATA.

Good work hunmonk! Way to keep the DB clean.

attached patch fixes the bug RobRoy reported. apparently the button count advances on this validation error, although i'm not sure why, and the form won't properly submit in that state. i don't see any reasonable way to manipulate the button count, so i went with a drupal_set_message, drupal_goto combo in the validation function. since we're only validating that the form is not empty, this should work fine.

as far as the error messages--i'll let someone with more theming knowledge take care of that. :)

Works great hunmonk. Yeah, the warning message is good enough as is. A good ole +1 from me.

"the code from my last patch is portable to any other include file" No, I guess its not that simple ;)

- drupal_uninstall_... should be system_... were are back in 'system' namespace
- also system_... should be an internal function (_system_...), it is not to be called from outside system.module.
- added #theme parameter to avoid auto-generated theme__system_... (double underscore)
- use drupal_get_path() to find the path to system.inc (in system.module)

i'm not sure i agree with some of these changes.

while i do understand the system_ name in the system space argument, i'm not so sure these should be private system functions. i'm pretty sure that other modules could invoke these functions for their own custom package uninstallations. looking at the code, if we were to simply add an optional second argument to what used to be drupal_modules_uninstall: drupal_modules_uninstall($form_values = NULL, $disabled_modules = NULL)

then:

if (!isset($disabled_modules)) {
  $disabled_modules = db_query("SELECT name, filename FROM {system} WHERE type = 'module' AND status = 0 AND schema_version > %d ORDER BY name", SCHEMA_UNINSTALLED);
}

this would allow other modules to invoke the uninstall stuff as generic functions, building their own custom list of modules to disable--possibly good for bigger contribs that have a lot of sub-modules, and want a custom uninstall interface.

i suppose this approach would rightly move the code back out of system.inc, and maybe into uninstall.inc in /includes

thoughts?

I dont think uninstall should be a generic function! Some big module packages (ecommerce?) may provide a page to enable/disable dependent modules, but they shouldnt provide uninstall feature IMO. This functionality should be available only in one central place, the modules administration page. Reasons: 1. uninstall is not needed for configuring a module (being able to disable it is really enough) and 2. it is a much too destructive operation to be integrated into the module settings.

We could think to add your proposed feature for enabling/disabling a set of modules. But thats another issue.

your theme call is wrong--theme_ is automatically prepended... :)

also, if we're sticking w/ internal functions, then we can remove the = NULL form the function declaration for _system_modules_uninstall -- as it will always be passed from the wrapper.

attached patch corrects these, and has been tested as working.

"your theme call is wrong" - oh yeah, thats what happens with copy/paste ;) Thanks.

All in all I think we can live with the latest patch. Hope others will agree.

updated patch to reflect the new capitalization scheme in core. no functionality change whatsoever.

code / forms are OK.

per Steven's suggestions:

• got rid of the all caps WARNING -- excessive
• add emphasis tags around the 'all data will be lost' text
• some minor spacing corrections
• changed the tab names to 'List' and 'Uninstall'
• moved the functions back to system.module -- we're not ready to move the admin stuff to a .inc file yet. this also allowed some slight simplification in the fapi calls.

once again, tested as working on a clean HEAD.

"moved the functions back to system.module"

you forgot to remove #validate and #submit, these are no longer needed

Committed to CVS HEAD. Thanks.

Is there any reason, why the help text didnt get committed?
(Also #33 redundant #validate / #submit was included)

ah, the #validate/#submit was needed when the code was living in system.inc--good catch! i removed them and tested, and things still function as advertised, so they should in fact be removed.

dunno why the help text wasn't put in. it was in my original patch.

this needs to go in. RTBC

attached patch implements the help text in a much cleaner way, and removes the extraneous #validate/#submit handlers. tested as working.

'<p>'. t('The uninstall process removes all
is better than
t('<p>The uninstall process removes all

Sorry, doesnt work. I will post a fixed version shortly.

- #validate/#submit handlers removed (#33/#35)
- new help text implementation without $POST (#37) also for 'modules/list'
- fixed broken #action path (wrapped in url())

@Tobias: Not sure about the <p>, all the messages in system_help() are implemented this way.

'<p>'. stuff fixed

Committed to CVS HEAD. Thanks.