| Project: | Drush |
| Version: | All-versions-5.x-dev |
| Component: | PM (dl, en, up ...) |
| Category: | bug report |
| Priority: | major |
| Assigned: | msonnabaum |
| Status: | closed (fixed) |
Issue Summary
I've found an interesting problem with
drush en Take this example
drush -y en cn_controlThe code for that module.install file is below, before you read that, just know that it tries to turn on two modules itself. These modules actually at some point in the drush request are enabled. Then they are switched off again! This, I think, is a bug. I realise that if I ran
drush -y en cn_control input_formats exportablesall would get enabled but I want my install function code to be honoured by drush. Skip past the module install file to see the problem....
<?php
/**
* Edit the modules array to add a new module and create a new update function
* to have it turned on automatically. Use the next number available.
* function cn_control_update_6xxx() {
* cn_control_modules();
* }
*/
function cn_control_modules() {
$modules = array('exportables', 'input_formats');
foreach ($modules as $module) {
//print "Enabling " . $module . "\n";
drupal_install_modules(array($module));
//print "Done with " . $module . "\n";
}
}
/**
* Implementation of hook_enable().
*/
function cn_control_enable() {
cn_control_modules();
}
/**
* Implementation of hook_install().
*/
function cn_control_install() {
cn_control_modules();
}
/**
* Implementation of hook_uninstall().
*/
function cn_control_uninstall() {
}
/**
* Update 6001: Turn on modules.
*/
function cn_control_update_6001() {
cn_control_modules();
}In drush/commands/pm/pm.drush.inc within the drush_pm_enable() function quite early on calls
$enabled = array_keys(array_filter(drush_get_modules(), 'pm_is_enabled'));This stores the currently enabled modules. This is where the problem begins. Drush right now believes that the only modules that will be enabled are the ones on the command line, in this case "cn_control"
Later on in drush_pm_enable() the following code runs. Note the use of the $enabled variable.
// Enable modules and pass dependency validation in form submit.
if (!empty($modules)) {
drush_module_enable($modules);
$current = drupal_map_assoc($enabled, 'pm_true');
$processed = drupal_map_assoc($modules, 'pm_true');
$active_modules = array_merge($current, $processed);
drush_system_modules_form_submit($active_modules);
}So effectively cn_control has been turned on, cn_control has turned on exportables and input_formats.
Then drush posts the drush_system_modules_form_submit form with the initially enabled list merged with cn_control. This ignores the modules that cn_control has turned on and therefore disables them when the form is processed.
I think that $current/$enabled needs to be refreshed within this if block to grab the modules that any other modules that may have been enabled down the chain.
Stew
Comments
#1
Does cn_control module works as you expect if you enable it via web?
Also, I suppose you have a very good reason to not use
dependencies[] =as usual.#2
What version of Drupal? Module enable/disable was really hackish until Drupal7 and edge cases are not easily handled.
#3
I echo jonhattan's question about dependencies[]; not using it for enable seems dubious. Also, the function
cn_control_update_6001()seems dangerous; if you want to enable modules in an update, this technique makes no guarantee that the required modules have been downloaded.Seems that the best advice would be to make sure that dependencies[] is correct; from there, we have a fighting chance of preflighting and correcting potential problems, at least in drush if not in d6.
#4
@jonhattan.
The control module is a control module that simply does stuff like enabling other modules and then performing actions against them. I included the 6001 update function as this indicates that cn_control_modules gets called on install, enable and on update so that devs can update one location to bring in new modules. The devs often use this as a way of adding brand new features automatically on update. To be honest I've never tried using the dependencies in .info as a) I hadn't thought of it, b) I wouldn't imagine that the module system would realise straight away if I added a dependency and then install that on update.
It enables perfectly when installed or enabled at admin/build/modules.
@Moshe hi!, this is latest D6 using Pressflow. Regarding your comment the module enable stuff is working perfectly so far and is the approach we took on Economist.com and I have taken it in many places since. The thing that was different about The Economist is that the control module equivalent was already enabled. The drush updates work fine, it is purely the install that pre emptively calculates the modules that should be installed and doesn't recheck that.
The fix to this could be as simple as
$current = drupal_map_assoc(array_keys(array_filter(drush_get_modules(), 'pm_is_enabled')), 'pm_true');However I don't know whether there is a reason not to recalculate the currently enabled modules?
#5
Attached is a patch that simply rechecks the enabled modules. I've tested this locally and the change works as intended.
#6
@greg.1.anderson @jonhattan I don't agree on the use of dependency for a module that is essentially trying to control what is happening on the site.
For instance take a module that is already enabled. Editing the .info file and adding a new module in that you wish to be enabled and then doing something like "drush updatedb" would actually disable the cn_control module.
The method I am using here to turn on modules automatically in an update/install is a common method of automating updates to Drupal sites.
#7
I'm sorry, but your disagreement in #6 does not make any sense to me -- that is, I do not understand the technical reasons behind your assertion.
By coincidence, there was a really good article on this subject that came by on Drupal Planet today. See: http://www.koumbit.org/en/articles/using-hook-update-non-database-relate...
This article explains pretty well why you might want to enable modules in an update hook, but I see no reason to enable a module in your install hook when the dependencies array works every bit as well. Here is a quote from the article:
I could see a feature request to drush being that pm-update should cache the module dependencies for every module prior to update, and then check them again after pm-updatecode is finished. When new module dependencies appear, drush could download [*] and enable them. This might only happen if requested, and should happen after updatedb, in case the module already "does the right thing" and enables the module itself.
I do not see a corresponding argument in favor of the feature request here. The code change is small, but there is a performance hit involved with
drush_get_modules(), so I for one would be disinclined to add it unless it could be demonstrated that this was not just an uncommon edge case.[*] Doing this would require that we solve the module-to-project-name mapping problem first.
#8
#9
I take your point and it is completely valid.
I think that from a developer experience point of view in the control module I have and in general I would like just one place to store the list of modules that should be enabled, whether on install or update. If I have to initially put them in dependencies it seems to just add code that ideally wouldn't be there.
#10
I disagree with your assertion that ideally dependencies[] would not be used. With dependencies, we can algorithmically determine relationships between projects without running code. Since this patch encourages the deprecation of a data structure I think is useful, and because it adds an additional call to
drush_get_modules(), I would prefer to not put it in. I do admit, however, that your technique works in the "pure Drupal" case, and it is therefore to a certain degree unfortunate that it does not also work with Drush. Given my way, I would prefer to deprecate your technique in Drupal rather than support it in Drush.Jonhattan is the maintainer of the pm components, though, and may decide either way on this issue.
#11
Hi Greg,
Thanks for taking the time to reply.
Please take this scenario. I have a module called t that depends as in using dependencies in the info file on x, y and z. I use this module to help me manage the other modules that are enabled. A few weeks later I want to turn on module n. If I add n to dependencies it will actually turn off t rather than also turning on n.
Regardless of this scenario I think the masses who use Drush expect "en" to work in the same way the modules page does when you submit it and currently it doesn't.
#12
Attached patch provides a little performance gain to pm-enable/disable (~ 0.5 sec) and is also compatible with @stewsnooze approach.
#13
OK, committed to master ... If we backport this, it seems like it can wait to 4.6. There is no serious bug we are fixing here AFAICT.
#14
There was a typo in the query arguments. Fixed in a5993e847fa8b14e6ae2a7405e852f80fe0269bc
#15
#16
I completely disagree with the use case described in this thread, but In the interest of drush doing the same thing as drupal, I've backported it to 4.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.