Suggestions to improve core-directory command

This is a good start, but more is needed.

drush_core_directory should call _drush_core_evaluate_path to find its path. This would allow you to pass things such as "@alias:%file" to get the files directory for the specified site alias. In order for this to work cleanly (that is to say, compatibly with the existing core-directory command), the any target that does not begin with "@" or "/" should be converted to "@self:%$target" prior to calling _drush_core_evaluate_path. _drush_core_evaluate_path should be renamed to drush_sitealias_evaluate_path and moved to sitealias.inc. It should also be improved so that anything supported in the current core-directory command (e.g. the d7 'private' target) is also supported in path aliases. The global $option used to specify special targets should be unified with site aliases so they have the same name (currently 'target' and 'path-aliases').

A bit ambitious for drush-3 stable, but would be good...

Thanks for the code and ideas, guys. I was pretty excited when I wrote this (I did not recall jonhattan's prior work) and am happy for the feedback. This is really nifty IMO. And your ideas make it much better.

I committed my progress but it needs work to fully implement what Greg proposed. Outstanding issues are below. Help is welcome. I've run out of time for now. As for rename, I'm open to that. path-to gets confusing IMO because path has a different meaning in drupal: $_GET[q].

1. `cdd` bash function reverses argument order for alias and target because i was too lazy to add a conditional to splice alias into right place. see the docs at end of core_drush_help()
2. Rename $path['rsync-path'] to something not rsync specific.
3. _drush_rsync_get_option() was moved to sitealias.inc and not renamed.
4. bring back drush_get_option('target') and unify with path-aliases as per end of #7
5. bring back support for 'private' target
6. 'files' target is slow because drush_sitealias_evaluate_path() does a backend_invoke (executes 'core-status' command) on itself to resolve the file path. Ideally, this would be avoided entirely, but if it has to happen, we only need to bootstrap to 'site' level to get the info we need. This backend invoke does a full bootstrap.

Regarding the performance of files, I'll experiment with an improvement to evaluate path that special-checks for %files being the only path alias, and if the alias is for @self and we're already bootstrapped to at least the site level, I'll make a direct call to get the value.

I'll also see what other work I can get done on this issue today.

Working on this, but probably won't have a patch until tomorrow.

Here's a patch that considerably improves drush cd, but it does not address all open issues. Of your list in #2:

1. Not addressed.
2. Fixed.
3. Not addressed.
4. Edit: Now I understand this one. Not addressed yet.
5. Not addressed.
6. I skip backend invoke if the target site has already been bootstrapped; however, to get %files we need to bootstrap all the way to DRUSH_BOOTSTRAP_DRUPAL_FULL, so this optimisation is only a little bit faster.

The main thing that this patch accomplishes is the unification of rsync path aliases and drush cd targets. So, with this patch you can also do something like this:

drush rsync @site1:%my_zen @site2:%themes

That would copy your custom zen subtheme to the themes directory of some other site. Any target available in cd is also available in rsync. This also works on remote machines (e.g. if @site2 is remote, the above rsync command still works).

I would recommend putting this much into RC4 (if it tests out); I probably won't have time to do any more tomorrow.

I need to install a D7 site to try this, but the private path could be supported by inserting something like this into _core_site_status_table():

      if (drush_drupal_major_version() >= 7) {
        $paths['%files'] = DrupalPublicStreamWrapper::getDirectoryPath();
        $paths['%private'] = DrupalPrivateStreamWrapper::getDirectoryPath();
      }
      elseif (function_exists('file_directory_path')) {
        $paths['%files'] = file_directory_path();
      }
      if (isset($paths['%files'])) {
        $status_table['File Directory Path'] = $paths['%files'];
      }
      if (isset($paths['%private'])) {
        $status_table['Private File Directory Path'] = $paths['%private'];
      }

Perhaps function_exists('DrupalPublicStreamWrapper::getDirectoryPath') would be a better switch than drush_drupal_major_version.

This patch merges #5 and #6, and also moves _drush_rsync_get_option to context.inc and renames it to drush_get_option_override.

I had %files working before recent commit with just bootstrapping to SITE, and including file.inc by hand. A one line kludge that made sense at the time.

I'll try out this new code today and commit if it goes well. The unification of cdd targets and rsync paths sounds real nice.

I checked cvs history, and grok what you did with including file.inc by hand. The same thing could be done today; I'll experiment with it (and the items below) if I have time in the next few minutes.

I made a quick 'lsd' script:

#!/bin/bash

DEST=`drush core-directory $1`
if [ $? != 0 ] || [ -z "$DEST" ]
then
  echo $1 "was not found."
else
  ls $DEST;
fi

It's useful. It would be nice if it would pass flags to ls. It could be a bash function, or even a drush command. cdd should also check $?.

#7 has bugs; I'll try to fix this afternoon.

cdd without any arguments should take you to the root of the current site. i think thats what you are asking about with $?

No; cdd with or without args will print an error message if drush could not bootstrap any Drupal site. Checking $? prevents cdd from trying touse the error message as the parameter to cd.

#7 works in non-optimised form (drush cd @alias:%file) but fails in optimised form (drush @alias cd %file). It works equally well with and without the include_once files.inc, so I'll post a patch that fixes ?$ in cdd as well shortly.

This patch should take care of all issues except for the first one in #2.

Here is a fixed cdd:

function cdd() {
  TARGET=
  PARAM=$1
  if [ $# -gt 1 ] ; then
    TARGET=$1
    PARAM=$2
  fi
  DEST=`drush $TARGET core-directory $PARAM`
  if [ $? != 0 ] || [ -z "$DEST" ]
  then
    echo $1 "was not found."
  else
    cd $DEST;
  fi
}

It could use a little more refinement; cdd @alias would ideally work like cdd @alias:%root (currently cdd works like cdd root). However, I think that the above is already a little too long to jam into 'help'. Maybe we should make a 'scripts' folder to put cdd and lsd, and .bat variants of the same?

I'm cool with a scripts folder. Only issue is with its name - could be confused with script command. I don't see a good alternative after 5 seconds of pondering

Quick coding standards review. Code quality looks good. Will try it out in a minute

+++ commands/core/core.drush.inc	14 Apr 2010 18:55:43 -0000
@@ -159,13 +159,14 @@ function core_drush_command() {
+      'target' => 'A module/theme name, or special names like root, files, private, or an alias : path alias stirng such as @alias:%files. Defaults to root.',

typo: stirng

+++ commands/core/core.drush.inc	14 Apr 2010 18:55:43 -0000
@@ -358,7 +359,7 @@ function _core_site_credentials() {
-      $credentials .= sprintf("  %-18s: %s\n", $key, $value);

too much indentation

+++ includes/sitealias.inc	14 Apr 2010 18:55:46 -0000
@@ -1402,22 +1458,28 @@ function drush_sitealias_evaluate_path($
+  //	evaluated-path:		machine:/path
+  //	server-component:	machine
+  //	path-component:		:/path
+  //	path:			/path
+  //	user-path:		path (as specified in input parameter)
+  //

tabs instead of spaces

Powered by Dreditor.

Looking good.

On drush core-status, I see

Modules Path                  :  sites/default/sites/all/modules                     
 Themes Path                   :  sites/default/sites/all/themes 

The values are wrong, and also I don't think this belongs in the plain st command. Can we print it only if --pipe?

drush cd --help gives a bootstrap error which I am investigating. Further, we should document the special strings %modules and %themes in this help text.

would be quite appropriate to unveil an 'lsd' script in san fran :)

lets do a scripts folder in the root directory. we'll put a README.txt in there which states its purpose.

Thanks for the review; I'll take care of this stuff tomorrow.

This patch should take care of all of the issues above. I removed cdd from 'help' and skipped the 'scripts' folder in favor of... drush cli. I implemented the cli by spawning a bash subshell with dynamically-generated aliases, as I described in #543550: Drush interactive mode. (Actually, I defined functions instead of aliases; that worked better.)

In order to make the cli work right, I had to rename the 'cd' alias to 'dd' in order to avoid collision with the built-in cd command. I renamed the main command from 'core-directory' to 'drupal-directory' to match.

drush cli is working pretty well, but there are a bunch of minor interaction decisions to work out. We could either put drush cli into 3.0-stable right now, and adjust it slightly in 3.1, or maybe move cli to drush extras for DrupalCon, and then drop the whole thing into drush core for 3.1. Your choice. I bundled them together for this patch due to the interaction between drush cli and the 'cd' alias.

great!. some quick thoughts. i'm supposed to be on vacation now

Drush Version                 :  3.0-dev                                             
 Drush configuration           :  /Users/mw/.drushrc.php /Users/mw/.drush/drushrc.php 
                                                                                      
 Paths:                                                                               
 Drupal Root                   :  /Users/mw/htd/prfr    

`drush st` has an empty line and an odd row for 'Paths'. Using D7.

'command-line-interpreter' is too verbose. i prefer core-cli. matches sql-cli

lets ahead and follow through on '// TODO: Remove this old code, which is only useful when talking to old (remote) versions of drush-3.0-rc3 and earlier'.

typing 'help' or any command in our customized bash shell gives me 'bash: drush: command not found'. i typically use an alias to drush.php' and cli might require drush on the $PATH. if so, we should consider documenting this technique instead of documenting how to make an alias in README.txt.

I deliberately put in the blank line and the "Paths:" line. I can take out just the blank line, or I can take out both if you'd like.

I'll rename to core-cli and yank the old code.

It was careless of me to call drush via "drush". I'll use DRUSH_COMMAND, just like backend invoke does.

Do you want me to separate out core-cli, or do you think it's okay for rc4?

In any event, I'll have those changes ready in a minute.

Yeah, I just added drush to my $PATH and the cli works nicely.

We should probably remove the sections of the README that recommend adding an alias to 'drush'.

I'm happy to add cli command to drush3 as this patch proposes. We can refine it in 3.1 and so on.

i want to remove the blank line and the Paths line in status output

After my feedback is incorporated, feel free to commit.

Committed. Once I used DRUSH_COMMAND instead of "drush", aliases worked nicely, so I saw no reason to remove the advice about them from the README file. Everything else was done as described above.

Great stuff. We should brainstorm a bit on what other tricks we can add for our custom shell.

We need a drush_command_invoke_all() so that contrib/custom commands can add cdd/lsd like functions to the custom .bashrc, and its help command.

I added a 'cli_bashrc' hook where commandfiles can add their own bash functions. I see now that our documentation for lsd and cdd is located at drush cli --help. That makes some sense, but its a bit hidden. I'll mull on alternatives. I kinda want these functions to be listed in the main 'help' command while in the shell. In the meanwhile, command files can add help to cli --help using the usual help hook.

lsd would work best as a regular drush command. cdd needs to be a shell function. We'd need to enhance help to detect the cli-mode arg and show the 'extra' help. Maybe it could/should call the same cli_bashrc hook, and change that to return both help and bashrc info. Maybe. Still considering too.

Excuse me, this is a small patch to add functionality to the drupal-directory command in multi-site environments. First verifies the existence of the directories sites/example.com/themes/ or sites/example.com/modules/.

Then:

shell> drush dd @example:%modules  [OR drush @example dd modules]
/path/to/drupal/root/sites/example.com/modules

shell> drush dd @example:%themes  [OR drush @example dd themes]
/path/to/drupal/root/sites/example.com/themes

Sorry to reopen, if I should create another issue let me know.
Greetings.

Luciano.

A new issue would have been better, but we can keep going here...

It's a little strange that this code might return sites/all/modules or modules. Actually, modules should always exist, so the path that returns sites/all/modules will never run. @site:%modules isn't really better than @site:modules, so I'd say that %modules should always return sites/all/modules, unless the site in question is using sites/sitefolder/modules, in which case it should return that. The question is, how do you tell which to return? isdir(sites/sitefolder/modules), probably...

Better.

Code looks good.

I don't see this as significantly better than what we have now but alrighty - committed.