Closed (fixed)
Project:
Drush
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
9 Oct 2009 at 01:23 UTC
Updated:
19 Dec 2009 at 12:00 UTC
Jump to comment: Most recent file
Hi,
I'd like to add an extra search path of $PWD/drushrc.php during bootstrap.
The reason we need this is we have a site layout as follows:
/home/drupal/core/drupal-5.20/sites/my5site.com
/home/drupal/core/drupal-6.14/sites/my6site.com
.. etc
For convenience, we automatically create symlinks in our user home dirs:
~malclocke/sites/my5site.com -> /home/drupal/core/drupal-5.20/sites/my5site.com
~malclocke/sites/my6site.com -> /home/drupal/core/drupal-6.14/sites/my6site.com
In each sites/* dir we have a drushrc.php with $options['r'] and $options['l'] set correctly.
If I do:
cd /home/drupal/core/drupal-5.20/sites/my5site.com
drush sql conf
Everything works fine. If I do:
cd ~malclocke/sites/my6site.com
drush sql conf
Nothing works. As far as I can tell, drush needs to find the root before it can find the site dir, but it cannot find the root when cwd is not a child of the root, as is the case when cwd is one of our symlink dirs.
Patch attached adds cwd to the list of candidates for containing a drushrc.php during bootstrap.
Malc
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drush-HEAD-2009_12_02.patch | 3.44 KB | greg.1.anderson |
| #10 | drush-HEAD-2009_12_01.patch | 3.01 KB | greg.1.anderson |
| #5 | cwd.patch | 3.52 KB | brad.bulger |
| #4 | drush-599698-4.patch | 639 bytes | malclocke |
| drushrc.cwd_.patch | 2.5 KB | malclocke |
Comments
Comment #1
moshe weitzman commentedComment #2
adrian commentedthis looks like it will cause the drushrc to be loaded twice if your current working directory matches on of the paths it searches already
Comment #3
adrian commentedit would probably be better if you changed the logic of how it detects a sites directory , but this is also going to cause problems elsewhere.
i think this is actually a pretty esoteric issue, in that you are trying to get drush to pick up that it is in a sites directory while removing nearly all of the things that drush can use to detect that it is in one.
Comment #4
malclocke commentedOK, dug around a bit more. Seems like the case of the drush shell script itself being a symlink was non-esoteric enough to get some special handling, so how about something similar?
Basically, cd into the canonical path for $PWD for the duration, then cd back when finished.
Comment #5
brad.bulger commentedI was going to ask to add this very thing, but for different reasons. We use several of the export/import related commands in directories completely unrelated to where the sites are installed, with local drushrc.php files to define options. There are probably ways to make things work using the -c flag, this is just more convenient.
I did notice that it tries to load the Drupal root drushrc.php file twice if that's my current directory. I added a declaration of $options as an empty array above the require_once() call in drush_load_config(), which seemed like a reasonable thing to do anyway since the following code assumes it will exist. That was the only bad side-effect I noticed.
I also changed the context name to 'cwd' to match the way it's referred to elsewhere.
Comment #6
moshe weitzman commentedI have no particular objection to this. I'd like for adrian or owen to make a final call.
Comment #7
adrian commentedas i said, there needs to be some form of detection when the CWD and one of the other contexts map to the same file so the file is not loaded multiple times.
Comment #8
brad.bulger commentedi will work on putting that in.
but i also wanted to say, @malclocke's original problem is really a separate issue, and i don't want to obscure it. would it be easier to manage to make a second issue that is strictly only about adding a 'cwd' context?
Comment #9
greg.1.anderson commentedI agree with Brad, and will go one step further and say that loading drushrc.php from cwd is a poor way to solve malclocke's original problem. I have created a new incident, #648280: drush should find the drupal root correctly, even when symlinks are in use., that includes a patch with a more direct solution.
Now, on to Brad's issue in #5. Loading a drushrc.php from cwd is one way to address his issue, but I do not think it is the best way. What he is really trying to do is introduce the concept of contexts that are used the same way that fabric contexts are used (see #628262: Study fabric and build similar on top of backend.inc), with the cwd also implying the context. I'm not completely opposed to loading drushrc.php from cwd; if drush is to do this, I would recommend that instead of loading drushrc.php from cwd that it instead loading a context file with a different filename, say
drush_context.php. This would greatly simplify the modification mentioned above in #7.Comment #10
greg.1.anderson commentedOkay, this has been nagging at the back of my brain; after some reflection, I decided that I don't like the idea of automatically loading drushrc.php (or a similar, differently named file) from cwd. If you happened to have a stray drushrc file somewhere, and ran drush from that directory, you could inadvertently load up a config file you had not intended to run. Likely? Maybe not, but I think it's kind of like putting "." on your $PATH; some people like to do it, others think it's not good. In any event, I do not think that this should be default behavior for drush.
I therefore submit for your consideration a patch that I believe is better. It is very simple, and does only two things. [*]
1. If you specify a directory for -c rather than a file, then drush will load the file "drushrc.php" from the specified directory.
2. drush_load_config('custom') is now done after 'drush', 'system' and 'user' contexts rather than before.
Change #1 allows you to easily load a configuration file from the cwd via
drush -c . status, and change #2 allows you to add$options['c'] = '.';to the config file in the drush folder, the system folder or the user folder if you would also like to enable loading a config file from the cwd. This is sort of like adding . to your $PATH; you can enable this behavior if you want it, but it's not done by default.@adrian: there is no need to worry about loading configuration files multiple times; require_once already does that. I tested it.
@Brad: declaring $options before including the configuration file had nothing to do with loading the same configuration files multiple times; you were actually working around a bug of mine where I checked $options before it was declared. I also fixed that bug in this patch, so pre-declaring $options is no longer necessary.
[*] In addition to changes #1 and #2, this patch also includes the aforementioned bugfix, and some documentation in example.drushrc.php for change #2.
Comment #11
brad.bulger commentedactually, i am pretty sure that Change #2 would give me what i was getting from the cwd context (apart from the less typing) - being able to have the custom drushrc.php file build on settings from those other default config files. so that alone would probably do me fine.
Comment #12
brad.bulger commentedconfirming, this works fine.
although, you still get the undefined variable notice because $options is not declared, from the drush_set_context() call. which can just be ignored, i suppose. otherwise, if you don't want to predeclare $options, then that plus the site-aliases merge above it should both be inside a
if (isset($options)) {test, seems like.Comment #13
greg.1.anderson commentedHm, okay. I'm not against pre-initializing $options if it is necessary. If I'm not mistaken, you will only get the above warning if you load a drushrc.php that does not define any $options at all. I don't have any config files like this, so I never saw this error. All the same, it might be worth handling, just for the sake of robustness. You can't just set $options = array() in drush_load_config, though, as that would blow away all of the settings from any config file previously loaded.
Here is a patch that stores the path to all of the config files loaded inside of $options, which has the nice side effect of always initializing $options, avoiding the warning message you mention. This patch is otherwise identical to #10.
Comment #14
brad.bulger commentedwhere i was seeing the undefined notice was by setting $options['c'] to '.' in the drush folder drushrc.php and then running in the Drupal root directory, where a second drushrc.php file existed. it tries to load that file more than once, as cwd and as drupal, and so the second require_once() doesn't load anything, thus no value for $options.
confirm that this patch stops that. i tried all the variations of using this i could think of, everything looks good.
Comment #15
greg.1.anderson commentedAh, okay. I misdiagnosed the problem, but I'm glad that my fix worked anyway. Setting $options = array() would also work, as I now see.
Comment #16
moshe weitzman commentedcommitted to HEAD.
please reopen if there is more to do.