drush dl attempts download to default when nothing specified

mojzis - April 14, 2009 - 22:34
Project:Drush
Version:All-Versions-HEAD
Component:Code
Category:bug report
Priority:critical
Assigned:Owen Barton
Status:closed
Description

the original behavior with 'all' as a default directory was much nicer :-)

added is a suggested patch to make sure some site was specified.

AttachmentSize
drush-dl-defall.patch971 bytes

#1

moshe weitzman - April 15, 2009 - 00:50
Assigned to:Anonymous» Owen Barton

when i last touched this code it would only use a site specific dir if *modules* had been created for that site dir. to me that suggests that you like to keep your modules there.

this is all overrideable in .drushrc so you might want to just do that. i will add more docs to that file soon.

i will take feedback on this patch but not so sure.

#2

duellj - April 15, 2009 - 01:30

I've run into this problem too. Expected behavior is that if I don't specify the URI parameter or the --destination parameter, then downloaded modules/themes should be saved in sites/all. Alternatively, drush could try and save in sites/default if the appropriate directories exist, then fall back to sites/all if not. Currently, drush saves in the document root if, for example, sites/default/modules does exist when downloading a module.

#3

mojzis - April 15, 2009 - 07:43

what it does now is that it tries default/modules, finds out it is not a directory and thus falls into current folder. I added a check that a uri was specified, and if not it takes the all directory instead. Probably the change from before is that the "DRUSH_DRUPAL_SITE_ROOT" wasnt filled ?

#4

Owen Barton - April 16, 2009 - 00:37

So, the ideal behavior (if no valid URL is provided) is that it will check for a default/modules directory and if that doesn't exist will check for an all/modules directory and use that if it exists, before falling back to the current directory? I have not problem with adding a check for that, especially if someone can pull together a patch.

#5

mojzis - April 16, 2009 - 00:45

I think the original behavior which went first to all/modules in case no url provided is ideal (sorry if I was unclear - in my last post I was referring to the current behavior). And I think that's what my suggested patch does - checks for the -l option. Probably I should also add a check for -url ? But otherwise I think it should be OK ?

#6

mojzis - April 21, 2009 - 08:38
Category:task» bug report

i tried my patch on a different machine and it didnt work, so sending a new attempt, complete with the uri switch (equivalent to l). I am quite surprised noone is unhappy about this ... it is annoying to have downloads fall into the current dir :-)

AttachmentSize
drush-dl-defall2.patch 1013 bytes

#7

NikLP - April 21, 2009 - 09:27

From the point of view of what is generally considered best practise, and also given that statistically it's more likely that a site will be operating as a standalone unit, I'd favour the idea of selecting sites/all/modules before sites/[default]/modules.

Normally, in a single site setup, I'd use sites/default/modules for custom modules and sites/all/modules for all my contribs.

Given that (let's be honest (aegir aside for a minute)) multi-site setups are pretty rare, I think it's totally fair to provide docs to help configure this for the more niche cases, but have it working "more simply, more of the time" - i.e. with de facto standard expected behaviour, out of the box.

I'd love to hear a good argument against that, I'm sure *someone* probably has one! ;)

#8

Owen Barton - April 21, 2009 - 21:15

After chatting with Moshe I agree that sites/all/modules is a better destination that sites/default/module for non-multisite setups, especially since that is the suggested module location for Drupal 7.

However, the patch in #6 breaks intelligent behavior for downloading modules to a multisite directory when you are calling drush within that directory (and using Drush automatic URI detection, without needing a --uri option or drushrc.php). Attached is a patch that fixes this, but maintains the rest of the behavior of the previous patch.

Here is what happens in each case:

server:~/workspace/drupal-6/sites$ ls *
all:
modules

default:
modules  settings.php

wia.6:
modules  settings.php

server:~/workspace/drupal-6/sites$ drush dl diff
Project diff (6.x-2.1-alpha1) downloaded to /var/www/workspace/drupal-6/sites/all/modules/.                                [success]

server:~/workspace/drupal-6/sites$ cd default/
server:~/workspace/drupal-6/sites/default$ drush dl diff
Project diff (6.x-2.1-alpha1) downloaded to /var/www/workspace/drupal-6/sites/all/modules/.                                [success]

server:~/workspace/drupal-6/sites/default$ cd ..
server:~/workspace/drupal-6/sites$ cd wia.6
server:~/workspace/drupal-6/sites/wia.6$ drush dl diff
Project diff (6.x-2.1-alpha1) downloaded to /var/www/workspace/drupal-6/sites/wia.6/modules/.                              [success]

server:~/workspace/drupal-6/sites/wia.6$ cd ..
server:~/workspace/drupal-6/sites$ drush dl diff --uri=wia.6
Project diff (6.x-2.1-alpha1) downloaded to /var/www/workspace/drupal-6/sites/wia.6/modules/.                              [success]

server:~/workspace/drupal-6/sites$ drush dl diff --destination=/var/www/workspace/drupal-6/sites/default/modules
Project diff (6.x-2.1-alpha1) downloaded to /var/www/workspace/drupal-6/sites/default/modules/.                            [success]

As the last example demonstrates, the only thing that is a little harder now is downloading to sites/default/modules in the case where you *do* want to do that. We could easily tweak the behavior so that it installs to sites/default/modules when you are running drush from within the sites/default directory (i..e the second example above), but sites/all/modules when your are anywhere else (except another valid multisite directory). Would folks on this ticket be happy with that, or would you still expect modules to be downloaded to sites/all/modules, even when you are invoking drush from the sites/default directory?

Either way, I think this is an improvement on the current drush dl behaviour!

AttachmentSize
dl_dest.patch 731 bytes

#9

mojzis - April 21, 2009 - 21:30

thanks a lot :-) and sorry for my ignoring the geniality of the uri detection :-)

BTW : i must have missed something, I do use multisite, but still have modules in all, not in default ....

and yes, I am happy about the solution :-)

#10

moshe weitzman - May 3, 2009 - 02:45
Status:needs review» fixed

Looks sane to me. Committed.

#11

Owen Barton - May 3, 2009 - 06:27
Status:fixed» needs review

Actually, I was waiting for feedback on if it is reasonable to tweak the behavior of this patch just a tiny bit further so that it installs to sites/default/modules when you are actually running drush from within the sites/default directory, but install to sites/all/modules when your are anywhere else (except another valid multisite directory).

This seems a bit more logical to me, but I wasn't 100% sure if this would mess things up for the sites/all/modules'ers :)

I can't see how it would, since they probably aren't spending much time in the sites/default directory if all their modules are in sites/all, I would assume that they would be calling drush from sites/all/module or from the drupal root or the sites directory (all of which would still default to sites/all/modules). Still, it seems good to check - no need to revert this patch though, it's certainly an improvement.

Also, what do people think about creating the sites/*/modules directory, if it doesn't exist - various people seem to bump into this issue. Of course, we could just improve the documentation, but I don't see any major issues if we just create it and move on.

#12

moshe weitzman - May 3, 2009 - 13:42

both of those tweaks sound good. putting download in sites/default when user runs command from there and auto creating sites/all/modules[themes]. note that d7 ships with both of those dirs.

#13

Owen Barton - June 8, 2009 - 22:02
Priority:normal» critical

#14

Owen Barton - June 9, 2009 - 02:27

OK, here is a patch with various heuristics that I think make this much more intuitive for most major usages of the all, default and multisite functionality.

// Attempt 0: Use the user specified destination directory, if it exists.
// Attempt 1: If we are in a specific site directory, and the destination directory already exists, then we use that.
// 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.
// Attempt 4: If sites/all exists, then we create destination in the sites/all directory.
// Attempt 5: If site directory exists (even default), then we create destination in the this directory.
// Attempt 6: If we didn't find a validdirectory yet (or we somehow found one that doesn't exist) we always fall back to the current directory.

AttachmentSize
482360.patch 4.89 KB

#15

moshe weitzman - June 9, 2009 - 02:35
Status:needs review» fixed

#16

Viybel - June 12, 2009 - 15:56

Wouldn't it be more logical to default to "sites/default" before "/sites/all" ?

When I type "drush dl thismodule", it says : "Initialized Drupal site at sites/default" and then "Using destination directory /srv/mysite/sites/all/modules/", which is contradictory.

#17

moshe weitzman - June 12, 2009 - 16:05

it isn't contradictory. in d7, we recommend people use sites/all. see earlier discussion on this. if you are still not pleased, just set a destination in your .drushrc and forget about it.

#18

Owen Barton - June 12, 2009 - 19:05

@Viybel

It's not hard to get it to use default, even without specifying a manual destination. If you run drush from the sites/default directory it will use that first. Also, if you delete your "all" directory it will also use the default directory if that correlates to the bootstrapped site.

#19

System Message - June 26, 2009 - 19:10
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.