The enclosed patch will download drush modules to $HOME/.drush. Any project that is a module and has a drush command file (*.drush.inc) but DOES NOT have a *.module file will be downloaded to $HOME/.drush. The .drush folder will be created if it does not yet exists. Drupal modules that contain drush commands (e.g. Features) will still be downloaded to sites/all/modules.

This change in behaviour will make it easier to download drush extensions in a way that will make them available to all Drupal installations on the system.

This feature was implemented by adding a new hook get-install-location, so any drush command can add a hook to control where the module should be downloaded. Currently, it is possible to use a pre-dl hook to set the destination based on project type and name, but with this patch there is a new option to hook in at a point where the contents of the project can also be considered. Might be useful in instances such as #754796: If downloadig ubercart contrib moduel move it to sites/all/modules/ubercart/contrib instead to sites/all/modules .

CommentFileSizeAuthor
#13 control-dl-location-3.patch6.17 KBgreg.1.anderson
#3 control-dl-location-2.patch5.87 KBgreg.1.anderson
control-dl-location.patch4.82 KBgreg.1.anderson

Comments

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

nice work:

1. this is a download destination in our language, not an 'install location'. we're not installing here. i would think that --destination overrides this?
2. i guess we just leave alone the --drupal-project-rename option since that renames the project folder itself, not its containing folder.

related issue: #727436: drush dl drupal reports wrong directory path

greg.1.anderson’s picture

greg.1.anderson commented

Okay; I will rename "install location" to "download destination". I will skip the hook completely if the user sets the --destination option. I'll see if I can fix up #727436 while I'm at it.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Needs review
StatusFileSize
new control-dl-location-2.patch5.87 KB

The hook is now called "adjust download destination"; it is not called if the --destination option is set.

Patch for #727436: drush dl drupal reports wrong directory path included with that issue.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

typo: paramter

i'd like to study drush_hook() a bit more. the naming suggests that it is like module_hook in drupal's module.inc but thats not right. i'd like to mirror drupal conventions if possible. the reference suggests that drupal_alter() is more akin to drush_hook but thats not right either I think.

greg.1.anderson’s picture

greg.1.anderson commented

I think drush_hook is fairly similar in concept to Drupal hooks, but I admit there are differences, and I'm not at all attached to the naming if this function. drush_invoke_with_reference_parameter would be very descriptive, but perhaps too long. drush_invoke_ref?

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Needs review

This is waiting for review by Owen and Adrian, yes?

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

Good idea. Lets go with drush_invoke_ref instead of drush_hook. Also, I'm not sure what this comment means: "// The reference parameter should be an array if it is used.". Why is that? Seems like that comment belongs in the @param section.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Reviewed & tested by the community

Also, I think we need to edit drush.api.php along with this patch. feel free to commit after these changes.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Reviewed & tested by the community » Needs work

The truth of the matter is that the variable does not have to be an array, but if it is not an array then you can run into the problem that NULL is a special value that means 'no parameter'. If you always use an array here, there are two benefits: you never run into NULL (pass an empty array for 'nothing'), and it is extensible, as you can set multiple values inside an array. The comment that an array 'should' be used is simplified advice. I can move it to the @param section & make it longer if desired.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs work » Reviewed & tested by the community

I was editing #9 when #8 was saved, hence the status change. I'll fix and commit this right now.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Reviewed & tested by the community » Needs work

I realize I made a mistake. I should have split drush_command_invoke_all_ref off of drush_command_invoke_all.

I'll do that shortly, but I think I'm out of time now.

greg.1.anderson’s picture

greg.1.anderson commented

Actually, rather than make drush_command_invoke_all_ref, I think I'll just make drush_command_invoke_all have one required parameter. Currently, all usage of drush_command_invoke_all pass at least one parameter.

I will also make the one required parameter a reference parameter. Existing hooks called by drush_command_invoke_all do not have a reference parameter, which is fine; if the calling function does not declare a reference parameter, then the first parameter cannot be changed, so making the first parameter a reference in drush_command_invoke_all will not introduce any change of behavior / side effects to existing code.

That will be cleaner.

greg.1.anderson’s picture

greg.1.anderson commented
StatusFileSize
new control-dl-location-3.patch6.17 KB

Almost there. I think I want to use 'rsync' instead of 'rename'.

moshe weitzman’s picture

moshe weitzman commented

drush_hook() is still there? I don't see it removed in the diff. Guess thats why this 'needs work'

greg.1.anderson’s picture

greg.1.anderson commented

No, drush_hook is gone. It's not in the patch in #13 because it was introduced in the patches in #0 and #3, which were never committed. In #13, I switched from drush_invoke to drush_command_invoke_all.

#13 is really RTBC (or at least ready for review) except for the fact that I want to use rsync instead of rename to move the project. As was discovered in another issue, php rename will fail if the source and destination locations are on different volumes. I'll finish this up once I take care of #764564: move drush_core_call_rsync to its own file in 'includes'.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Reviewed & tested by the community

Using rsync will make this a bit less accessible for windows folks, no? Right now, they can use 95% of drush without having rsync.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Reviewed & tested by the community » Fixed

Well, that is a good point. I didn't think of it as much of an issue, because I think that Windows without cygwin is pretty useless as a scripting environment.

For now, I've committed it as-is; I'm going to spend some more time considering what the best action to take is when drush wants to use 'rename' in a way that might potentially cross from one volume to another.

Status: Fixed » Closed (fixed)

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