Redispatch to site-local drush if it exists

[greg-1-anderson]

This was actually very simple.

Drush already has a redispatch mechanism in place for calling remote drush commands (e.g. "drush @Remote cc all"). This patch leverages this mechanism, and redispatches to a site-local Drush if it exists.

The paths checked for local Drush installations are ROOT/drush, and ROOT/site/aliases/drush. So, the actual path to the local Drush executable should be ROOT/drush/drush/drush, or ROOT/site/aliases/drush/drush/drush.

If there isn't a site-local drush installed, then this patch should have no affect on existing code -- but we'll see what Travis and our unit tests have to say about that.

[greg-1-anderson]

One minor error -- stuff is getting executed in simulated mode in some instances. Will fix it shortly (but not right now).

[weitzman]

I'm hesitant to go down this road. The PR is clever, but I'm wary.

1. We worked hard years ago to get Drush out of the docroot. This feels like a step backwards. I'm OK with looking for it in /vendor but not ROOT/drush and sites/all/drush
2. Today, we ship with examples/drush which is a wrapper script that facilitates calling the local Drush. It reads vendor/bin/drush --local $@. I'm OK with expanding this script so it is useful in more situations, or adding another example.

This also overlaps with site aliases. Would a redispatch happen today if you call drush @local-site status when @local-site references its own local vendor/bin/drush? If yes, then perhaps we encourage folks be explicit like this in their aliases.

[greg-1-anderson]

I agree that we should look in vendor, and get rid of the references to ROOT/drush and sites/all/drush. Alternately (or additionally), we could look in ROOT/../drush, which would keep drush out of the docroot entirely. This would be good for security purposes. This location doesn't work for folks using the "split core" schema for managing sites with composer, because in this case, composer.json is in the docroot, and I don't think composer will install to its parent directory (or we should not encourage use of such a feature, if it does). So, I'd say that checking both vendor and ROOT/.. would be a good idea.

Today, %drush-script is ignored unless the invocation is remote. We could change this.

I think we should still have the automatic behavior in this patch, though (once adjusted for location), because some folks still like to select their site based on the cwd. Maybe we should enable the automatic drush-locating feature ONLY when finding the docroot based on the cwd (and --root on the command line?), and require the user to explicitly us %drush-script whenever local site aliases are used?

[greg-1-anderson]

The solution I suggested above introduces inconsistent behavior depending on whether you are using aliases or --root / cwd. This isn't good, as an alias is supposed to just be a collection of configuration.

A better idea:

If the user sets drush-script, either via %drush-script in an alias, or via a drush-script option in a site-local configuration file, then set --local and redispatch to drush.

Skip the redispatch if --local is set, or if drush-script points to the same executable that is currently running.

[greg-1-anderson]

The commit above removes the "magic locations", and instead depends on 'drush-script' set in a drushrc.php file or site alias.

[greg-1-anderson]

The one failing test right now is testShellAliasDrushRemote(). With my change, the cli value is passed to the remote invocation of Drush, whereas previously it was not. This reveals a basic problem. Given the following example:

drush @remote foo --config=/path/to/aliases-and-such

The way Drush works today, --config specifies a local configuration path. Since the configuration path is local, it is never passed on to the remote side. However, what if you want to explicitly pass a remote config path from the cli, to send to the remote Drush? There is currently no way to do this.

I don't recall anyone ever asking for this feature in all of the years that Drush has behaved this way, so I'm going to adjust the commit above appropriately, so that config paths are never propagated locally (as they currently are--a bug, I think) or remotely.

If anyone thinks we need a --remote-config option or anything like that, please chime in.

[greg-1-anderson]

Never propagate is not the right thing; the unit tests presume that --config is propagated locally, and folks are likely depending on this behavior as well.

[greg-1-anderson]

Okay, now https://travis-ci.org/drush-ops/drush/jobs/49215825 fails because it expects --config to be passed to remote systems. I guess Drush was inconsistent here.

Thoughts on how this should work? I want to turn off propagation of discovered configuration paths, but we can either propagate, or not propagate --config on the cli to the remote system. If we propagate it, then it will apply to both the local and remote system. Of course, there is no harm if the propagated path does not exist in one place.

Perhaps I should just roll back to the first version, where --config from the cli is consistently propagated, and fix the unit tests to reflect that. Any comments on the behavior of the rest of this PR? I think it's about ready to go in, once we make a decision about how --config should work.

[greg-1-anderson]

Put this back so that --config cli options are remotely propagated, consistently now, and fixed tests to reflect this. I'd like to commit this PR like this, if the general strategy looks sound.

[weitzman]

As for --config, I think there are use cases for passing it to remote sites and use cases for not doing so. Could we have --config which does not pass along and --config-remote which does?

The PR looks fine now but I admit I'm still a bit wary of recommending this. Do you have a need for this? At a high level, I think redispatch is confusing for people and for us to help them. Specifically, the logs get really verbose and confusing when there are two preflights and postflights. So this encourages a practice which I dont love. I'd love for something else to call into the local Drush, not the global drush.

I'm happy to discuss this and other issues by phone anytime this week.

[greg-1-anderson]

Yes, let's talk on the phone sometime.

Some analysis and brainstorming about possible alternate solutions. Feel free to alter the base assumptions, if you have any ideas for simplifications that make this better.

Drush Bootstrapping
DX we wish to preserve:

• ‘drush’ available on the global path
• Flexible site selection, supporting any of the traditional methods:
  • drush @site …
  • drush —root= —uri= …
  • cd /path && drush …
  • $options[‘root’] = …; (some folks still use this, i.e. in multisite situations?)
• We also want to support Drush commands that work without any selected site
• We want to be able to continue to move procedural code into classes
• Site-local drush should be ‘require’d from the Drupal project’s composer.json
• composer install will resolve version dependencies between libraries needed by Drush, Drupal, Drupal modules and Drush extensions.

Characteristics of the redispatch code, as it exists today

• Drush loads its autoload classes first thing (n.b. Drush does not currently take advantage of this fact, but Drush could determine right away whether it is running as a site-local Drush or as a global / launcher Drush simply via the relative location of the autoload.php file.)
• Drush parses all of its configuration options from global locations
• Drush loads all of its command files (Maybe we could delay loading command files until after preflight?)
• Drush locates the site it is going to bootstrap (command files might be used here, e.g. to identify Drupal vs. Backdrop, etc.)
• Site-local options are loaded (but not command files, yet)
• Redispatch happens here, prior to any site bootstrap phases being executed. Command-specific options from configuration files or aliases are passed along on the command line to the site-local Drush. (Command files are used here, to determine validity of command-specific options.)
• In a local redispatch, the —local flag causes Drush to skip loading most of the configuration files parsed last time. Site-local configuration files are parsed in both phases.

Consideration: Could we autoload from the selected Drupal site, instead of redispatching?

• We would need to postpone bringing in the autoload classes until after site selection.
• Config file parsing would have to happen without autoload classes. This is a lot of code, and we would like to eventually move aliases into classes. This implies that maybe some classes are not autoloader? This is inconvenient.
• The non-autoload code in the global Drush would have to remain compatible with the autoload code in the site-local Drush. This implies the same, or very similar version of Drush in the global and site-local locations. This is impractical.

Consideration: Could we make a tool that is not Drush that launches Drush after site selection?

• Tool would need to duplicate Drush config file parsing and alias parsing. Perhaps if we factored this out into classes, as we have wanted to do, we could put them in a dependent library that both the Drush launcher, and core Drush could share.
• The operation of the tool would be analogous to the current Drush preflight.
• If there was no selected site, the Drush launcher would need to know how to find some other Drush that could run commands that do not have a local Drush, (quick-drupal, make, etc.) or for Drupal sites that do not have a site-local Drush.
• Is there any win to be had by repackaging like this? Any simplification that we could make here I would think we could also make to Drush’s preflight, and end up with the same behavior in the end.

Preliminary conclusion: I don’t think that we can make the code much better by changing the redispatch technique unless we remove desirable features (e.g. drush @site …). The flexibility of using Drush as the Drush launcher means that we don't need to solve the problem of what to do for commands that do not have a site, or sites that do not have a site-local Drush.

[weitzman]

Do we really want to grab from GLOBALS here? Maybe drush_build_drush_command() is helpful?

[greg-1-anderson]

Yeah, thanks; I was lazy and then forgot to go back and fix it. I'll improve it in the next commit.

[greg-1-anderson]

Committed this from another branch (to clean up the reverts), and fixed the GLOBALS thing.