Currently, in multi-site environments, when you want to download some projects for a specific site you must:
- create a config file for each site with the respective command specific option, or
- add to each alias the respective 'command-specific' item, or
- use the --destination option every time.

This patch tries to avoid this.

CommentFileSizeAuthor
#31 drush-951026.patch2.57 KBjonhattan
#29 drush-951026.patch2 KBjonhattan
#25 drush_dl_destination_3.patch2.06 KBluchochs
#15 drush-951026.patch939 bytesjonhattan
#11 drush_dl_destination_2.patch2.21 KBluchochs
#1 drush_dl_destination__.patch1.65 KBluchochs
drush_dl_destination_.patch2.1 KBluchochs

Comments

luchochs’s picture

luchochs commented
StatusFileSize
new drush_dl_destination__.patch1.65 KB

I will create a new issue for the code that does not correspond to this one.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Postponed (maintainer needs more info)

We discussed the current behavior in #433838-8: drush dl attempts download to default when nothing specified and its replies. Pls review that and see if this patch is still desired.

luchochs’s picture

luchochs commented
Status: Postponed (maintainer needs more info) » Needs review

Moshe, the proposal here is different.
Which is avoided by the suggested patch is, for example, this:

shell> drush @site-ar status --pipe
...
drupal_root=/home/lucho/proyectos/www/drupal
site_path=sites/site.com.ar
modules_path=sites/site.com.ar/modules
themes_path=sites/site.com.ar/themes
...
shell> drush @site-ar dl robotstxt
Project robotstxt (6.x-1.2) downloaded to /home/lucho/proyectos/www/drupal/sites/all/modules/robotstxt.

After applying the patch:

shell> drush @site-ar dl robotstxt
Project robotstxt (6.x-1.2) downloaded to sites/site.com.ar/modules/robotstxt.

Sorry if I was not descriptive previously.

Update: Most comprehensive example.
Update2 [comprehensive != comprehensible (Spanish)]: Most understandable example.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs review » Needs work

When running drush dl from sites/example.com you get a different result than when running drush @example.com from other path (if sites/example.com/modules exists).

Forcing the usage of sites/example.com/{modules,themes}, even creating the directory if it doesn't exist could be implemented as an option to set in drushrc.

@luchochs: your patch is conceptually wrong. pm_dl_destination_lookup() is called several times by pm_dl_destination().

luchochs’s picture

luchochs commented

Jonnhattan:

When running drush dl from sites/example.com you get a different result than when running drush @example.com from other path (if sites/example.com/modules exists).

I can't reproduce that.

@luchochs: your patch is conceptually wrong. pm_dl_destination_lookup() is called several times by pm_dl_destination().

According to my tests, 'several' does not make sense in that sentence.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

To confirm «When running drush dl from sites/example.com you get a different result than when running drush @example.com from other path (if sites/example.com/modules exists).»

jonhattan@larry:/var/www/drupal-6.x-cvs$ drush @d6 dl examples
Project examples (6.x-1.x-dev) downloaded to /var/www/drupal-6.x-cvs/sites/all/modules/examples.                   [success]

jonhattan@larry:/var/www/drupal-6.x-cvs$ cd sites/d6
jonhattan@larry:/var/www/drupal-6.x-cvs/sites/d6$ drush dl examples
Project examples (6.x-1.x-dev) downloaded to /var/www/drupal-6.x-cvs/sites/d6/modules/examples.                    [success]
According to my tests, 'several' does not make sense in that sentence.

It does.

ilo’s picture

ilo commented

subscribing.

luchochs’s picture

luchochs commented

You must add the 'uri' item to your alias, i.e., 'uri' => 'd6'.

Regarding the call to the pm_dl_destination_lookup() function, could you explain in what condition/context is called several times? Thanks.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

Indeed I have 'uri' => 'http://d6', but this is not the cause.

Responses are here: http://drupalcode.org/viewvc/drupal/contributions/modules/drush/commands...

cwd is used to find a destination. If you are running drush dl zen from within the site directory, it will be placed in sites/example.com/modules only if modules/ exists. If running from outside the site directory, it will use sites/all/modules, even if using @example.

moshe weitzman’s picture

moshe weitzman commented

I think the issue is that folks are naturally lazy and changing your cwd is too much work and adding --destination is too much work. they just want to drop modules in site specific directory if it exists and cwd does not suggest otherwise (e.g. you are in drupal root). seems reasonable at first thought.

luchochs’s picture

luchochs commented
Status: Needs work » Needs review
StatusFileSize
new drush_dl_destination_2.patch2.21 KB

That's the idea. A simple and practical way to tell Drush that there's a site that requires specific projects; just creating the appropriate (and conventional) directory.

In my tests (results below) the pm_dl_destination_lookup() function was called only one time whenever the pm-download command was in use and cwd did not behave in an unexpected way.

# Same site and sitealias that I used in #3.

shell> pwd
/tmp
shell> dr @site-ar dl robotstxt
Project robotstxt (6.x-1.2) downloaded to sites/site.com.ar/modules/robotstxt.                                   [success]
shell> cd `dr @site-ar dd site`
shell> dr @site-ar dl robotstxt
Install location sites/site.com.ar/modules/robotstxt already exists. Do you want to overwrite it? (y/n): n
Abort installation of robotstxt to sites/site.com.ar/modules/robotstxt.                                                [warning]
shell> cd ../..
shell> dr @site-ar dl robotstxt
Install location sites/site.com.ar/modules/robotstxt already exists. Do you want to overwrite it? (y/n): n
Abort installation of robotstxt to sites/site.com.ar/modules/robotstxt.                                                [warning]
shell> pwd
/home/lucho/proyectos/www/drupal

Probably someone else can test this too.

New version of the patch with a bit of documentation in accordance with the code, and without special handling for the Theme engines (disuse nowadays).

luchochs’s picture

luchochs commented

For those who are interested in this: #11's patch still applies correctly against HEAD.

greg.1.anderson’s picture

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

Conceptually I am okay with this patch. It follows the same pattern as the existing test for 'contrib'. The only think that is odd is that the path printed is an absolute path for all existing cases, but is a relative path for the new locations.

$ drush @gkhome dl eightball
Project eightball (6.x-1.2) downloaded to
/srv/www/home.greenknowe.org/sites/all/modules/eightball.
$ rm -rf /srv/www/home.greenknowe.org/sites/all/modules/eightball
$ mkdir /srv/www/home.greenknowe.org/sites/greenknowe.org/modules
$ drush @gkhome dl eightballProject eightball (6.x-1.2) downloaded to
sites/greenknowe.org/modules/eightball.

It would be better to prepend $drupal_root to conf_path() for consistency. Also, I have a slight preference for following the style of the $contrib check, and assign the variable outside of the 'if' statement.

With those changes, I think this could be committed.

luchochs’s picture

luchochs commented

Good news for those who work daily in multisite environments and prefer to take advantage of site-alias feature. New patch shortly.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs work » Needs review
StatusFileSize
new drush-951026.patch939 bytes

As I mentioned in previous comments luchochs approach is not the way to solve this.

Attached is a clean way to fix this. I'm not very familiar with sitealias and haven't found a function to test if drush was invoked with @site.

and here's my test:

jonhattan@larry:/var/www/drupal-6.x-cvs$ drush dl devel
Project devel (6.x-1.22) downloaded to /var/www/drupal-6.x-cvs/sites/all/modules/devel.                                                           [success]
Project devel contains 4 modules: performance, devel_node_access, devel_generate, devel.

jonhattan@larry:/var/www/drupal-6.x-cvs$ cd sites/d6
jonhattan@larry:/var/www/drupal-6.x-cvs/sites/d6$ drush dl devel
Project devel (6.x-1.22) downloaded to /var/www/drupal-6.x-cvs/sites/d6/modules/devel.                                                            [success]
Project devel contains 4 modules: performance, devel_node_access, devel_generate, devel.

jonhattan@larry:/var/www/drupal-6.x-cvs/sites/d6$ cd ../..
jonhattan@larry:/var/www/drupal-6.x-cvs$ drush @d6 dl devel
Install location /var/www/drupal-6.x-cvs/sites/d6/modules/devel already exists. Do you want to overwrite it? (y/n): n
Abort installation of devel to /var/www/drupal-6.x-cvs/sites/d6/modules/devel.                                                                    [warning]

jonhattan@larry:/var/www/drupal-6.x-cvs$ mkdir sites/d6/modules/contrib
jonhattan@larry:/var/www/drupal-6.x-cvs$ drush @d6 dl devel
Project devel (6.x-1.22) downloaded to /var/www/drupal-6.x-cvs/sites/d6/modules/contrib/devel.                                                    [success]
Project devel contains 4 modules: performance, devel_node_access, devel_generate, devel.

In addition, as I mentioned in #4 when using a sitealias would be oportune to create the directory if it doesn't exist. In fact you're telling drush to download a project to the site dir.

greg.1.anderson’s picture

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

This patch is mildly problematic, regardless of approach.

Moshe said:

they just want to drop modules in site specific directory if it exists and cwd does not suggest otherwise (e.g. you are in drupal root)

It seems odd that if the user's cwd is the drupal root that the target of dl would change. If the user explicitly made a 'sites/mysite/modules' directory, then drush should use that. The user might be at the drupal root 'by accident', as this is a common location to be. On the other hand, if the user wants some modules to be at sites/all and some to be at sites/mysite/modules, well, I'm not sure that there's an easy way to support that besides using --destination. Prompt?

Anyway, I never use site-specific module directories, though, so at the end of the day I'm indifferent which way this goes.

Regarding #15, (count(drush_get_context('alias'))) is not a good way to check to see if an alias is being used. By the current implementation, which uses lazy-evaluation to load alias definitions as needed, it happens at the moment that the alias context will be empty when there is no alias reference. However, this was not true in the past and might not be true in the future; the alias code might pre-load something and stuff it in the 'alias' context prior to alias evaluation.

The alias system is designed to be equivalent to using options, so I think that perhaps a better test would be to see if --root has been defined. If it has, then cwd will not affect bootstrapping, so you might as well treat this case the same as the @sitealias case.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

I just wanted to outline a clean way to implement luchochs proposal but I'm fine with cd'ing or --destination.

Other different approach is --use-site-dir. This is not conflictive and perhaps the only option to not mark the issue as won't fix.

One more approach (up to users): implement a custom hook_drush_pm_download_destination_alter() to set the install location (see @pm_drush_pm_download_destination_alter() for reference).

I agree it is problematic to use @alias. I suppose there're also problems with built-in, remote and/or group aliases.

Some tests:

jonhattan@jengibre:/tmp$ drush dl relativity
Project relativity (6.x-1.4) downloaded to /tmp/relativity.                                                        [success]
jonhattan@jengibre:/tmp$ drush @imc dl relativity
Project relativity (6.x-1.4) downloaded to /var/www/drupal/sites/all/modules/relativity.                              [success]
jonhattan@jengibre:/tmp$ /usr/src/drush-951026/drush @imc dl relativity
Project relativity (6.x-1.4) downloaded to /var/www/drupal/sites/default/modules/relativity.                          [success]

^^ problematic: current implementation will not use site-specific-folder and it's not clear what the user wanted by providing @alias.

jonhattan@jengibre:/tmp$ cd /var/www/drupal

jonhattan@jengibre:/var/www/drupal$ drush dl relativity
Install location /var/www/drupal/sites/all/modules/relativity already exists. Do you want to overwrite it? (y/n): n
Abort installation of relativity to /var/www/drupal/sites/all/modules/relativity.                                     [warning]

jonhattan@jengibre:/var/www/drupal$ /usr/src/drush-951026/drush dl relativity
Install location /var/www/drupal/sites/all/modules/relativity already exists. Do you want to overwrite it? (y/n): n
Abort installation of relativity to /var/www/drupal/sites/all/modules/relativity.                                     [warning]

jonhattan@jengibre:/var/www/drupal$ /usr/src/drush-951026/drush @imc dl relativity
Install location /var/www/drupal/sites/default/modules/relativity already exists. Do you want to overwrite it? (y/n): n
Abort installation of relativity to /var/www/drupal/sites/default/modules/relativity.                                 [warning]

^^ here there is no incompatibility with current behaviour. It can be considered as desirable: you ask drush to download to a given site by providing the alias.

greg.1.anderson’s picture

greg.1.anderson commented

I like --use-site-dir. Users who want drush to work as in #11 can set $options['use-site-dir'] = TRUE; in their drushrc.php.

luchochs’s picture

luchochs commented

Jonhattan, you're working on a patch for this issue? Let me know please.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

no, i'm not working on this.

luchochs’s picture

luchochs commented

Thanks for the prompt response, I'll see if I can do something in the matter.
P.S.: Greg, I assumed that you aren't working on a patch here, if I'm mistaken let me know please.

greg.1.anderson’s picture

greg.1.anderson commented

Patches welcome; I'll review what is submitted.

luchochs’s picture

luchochs commented

It seems appropriate to add the following in case anyone read my previous comment and it stops his desire to take this into HEAD:
At the moment I'm not working on this, if this changes I will notify.

luchochs’s picture

luchochs commented
Assigned: Unassigned » luchochs
luchochs’s picture

luchochs commented
Assigned: luchochs » Unassigned
Status: Needs work » Needs review
StatusFileSize
new drush_dl_destination_3.patch2.06 KB

New approach.

Pre-patch:

- Attempt 2: If the destination directory already exists for sites/all, then we use that.
- Attempt 3: If a specific (non default) site directory exists and sites/all does not exist, then we create destination in the site specific directory.

Post-patch:

- Attempt 2: If a specific (non default) site directory exists, then:
-- if sites/specific_site/modules|themes directory also exists we use that, or
-- if sites/all/modules|themes directory does not exist or exist but contains only one file (potentially README.txt) we create & use destination in the site specific directory.
- Attempt 3: If the destination directory already exists for sites/all, then we use that.

Attempts 1,4,5 & 6 don't vary.

greg.1.anderson’s picture

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

I think we should require a flag (e.g. --use-site-dir) before allowing sites/sitename/modules to be used. If --use-site-dir is specified, it should take priority and create sites/sitename/modules if it does not exist. The 'contrib' check should still be done.

luchochs’s picture

luchochs commented

The flag once the directory has been created is no longer needed, but yes, is useful (maybe --make-...... is more representative).
Regarding 'contrib', the check is still done (the comment need a fix).

greg.1.anderson’s picture

greg.1.anderson commented

Per #16, we must consider the situation where someone wants some of their modules in the site-specific directory, and some of their modules in sites/all/modules. Today you can use --destination to select. Any patch that changes current behavior should be more convenient without removing options, ergo --use-site-dir. If the site-specific directory exists, but --use-site-dir is not specified, then the project should be downloaded to sites/all/modules.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs work » Needs review
StatusFileSize
new drush-951026.patch2 KB

Sumarizing:

1- we want to preserve current behaviour and
2- add a new flag --use-site-dir that will force to use the site specific module/theme directory if bootstrap level >= SITE and
3- will create that directory if it does not exist.

luchochs’s picture

luchochs commented

--use-site-dir is a good idea.

Other thoughts:
1.- We need to update the current behavior to ensure conformity with the sitealias age.
2.- pm_dl_destination_lookup() has a parameter to create the directory, we might take advantage of it.
3.- pm_dl_destination()'s $site_root is a good reference.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
StatusFileSize
new drush-951026.patch2.57 KB

New patch takes advantage of $create argument to pm_dl_destination_lookup()

luchochs’s picture

luchochs commented

#31 looks & works good to me.

luchochs’s picture

luchochs commented
Status: Needs review » Reviewed & tested by the community

Productive exchange of ideas, really.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Reviewed & tested by the community » Fixed

commited.

Status: Fixed » Closed (fixed)

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