Remove dual autoloader feature from global Drush installs #5521

greg-1-anderson · 2023-04-01T03:29:01Z

Do not allow Drush to combine autoload files from a non-site-local Drupal; only bootstrap Drupal sites with a shared vendor.

Continuation of #5138. No changes yet, just rebased on 12.x branch.

c.f. #5138 (comment)

…upal; only bootstrap Drupal sites with a shared vendor.

weitzman · 2023-04-01T03:52:56Z

Thanks for getting this going again. The simplification looks great.

I'm uneasy with blessing drush as a launcher. We achieved a lot of hard fought clarity in our install docs which now read "Drush only supports one install method. It requires that your Drupal site be built with Composer and Drush be listed as a dependency.". They also say "To be able to call drush from anywhere, install the Drush Launcher.". The problem with the old way is that bug reporters had no idea which drush was giving an error (launcher or site). They also had mismatched versions between launcher and site. I am loathe to going back to those days. I can tolerate if Drush happens to work as a launcher.

greg-1-anderson · 2023-04-01T04:41:39Z

I am reluctant to remove all of the launcher stuff primarily due to the handling of remote site aliases. Perhaps we could remove the launcher-related code that was not related to remote sites. We could also consider moving remote site aliases to the Drush launcher, and remove all of the launcher code from Drush core.

Failures here are due to some situations that weren't quite right in the 11.x version of this PR, and are now being caught by strict type checking in the 12.x branch. Looking into those now.

greg-1-anderson · 2023-04-01T04:50:02Z

Another thing about remote dispatches: in 11.x, we run:

ssh drush --root=/path/to/drupal ...

In 12.x, we should probably change this to:

ssh /path/to/drupal/vendor/bin/drush ...

greg-1-anderson · 2023-04-01T13:59:47Z

I spent some time considering the user journeys related to remote dispatch and site aliases that are and are not important to me.

Important use cases:

drush rsync a b, where a and b might be a path, an alias, or @self, or a mixture of an alias and a path
drush sql:sync @a @b, where one of @a or @b is @self
drush sql:sync fetching database credentials from the non-self site (to support providers like Pantheon where the database credentials may change)
drush sql:conf with an alias to test to see if sql:sync can fetch the database credentials, for diagnosing problems with sql:sync.

Unimportant use cases:

A global Drush that can launch a site-local Drush
drush sql:sync @a @b, where both @a and @b are remote sites

I think I am holding on to the Drush launcher because I am concerned about harming use cases 3 and 4.

Proposal:

Make use case 3 more stable / less dependent on a launcher by making remote Drush calls use ssh /path/to/drupal/vendor/bin/drush ... instead of ssh drush --root /path/to/drupal
Introduce drush remote @alias <command> as a way to run remote commands.

If we did those two things, then I think we could completely remove drush @alias <command> and the --root option.

What do you think?

weitzman · 2023-04-01T14:08:54Z

Sounds good to me.

Introduce drush remote @alias as a way to run remote commands.

Would we obsolete the HandleRemoteCommands attribute because there would be no such thing as remote dispatches? The only way would to remote dispatch would be via drush remote? I refer to https://github.com/drush-ops/drush/blob/12.x/src/Commands/core/BrowseCommands.php#L31, for example

greg-1-anderson · 2023-04-01T15:16:55Z

I'm not sure if we should maintain the syntax drush @alias browse, or force everyone to switch to drush browse @alias. If we do allow drush @alias browse, should we also allow a few common remote commands to persist, such as drush @alias sql:conf and drush @alias status? I'm not sure, but I think my inclination would be to lean towards backwards compatibility and keep some instances of drush @alias <command. Maybe we remove the general-purpose remote dispatch, but add HandleRemoteCommands to a few select commands, such as sql:conf and status. I am undecided.

There's another important use-case I forgot about. Drush 11.x should be able to use sql:sync to target a Drush 12.x site, and visa-versa. It is not necessary for Drush 12.x to be able to target a remote site that does not have a site-local Drush, though. To support this transition, Drush 12.x should include a redundant --root option when calling a remote Drush, and Drush 12.x should also accept a --root option on the command line, although we could make it always fail-fast if the path was different than the site that shares the vendor directory with Drush.

greg-1-anderson · 2023-04-01T21:24:22Z

Use case: Running Drush when there is no Drupal site at the cwd.

Drush 11.x:

$ drush --simulate --uri=dev --no-interaction 'user@server/path/to/drupal#sitename' non-existent-command foo --bar=baz
 [notice] Simulating: ssh -o PasswordAuthentication=no user@server 'drush --no-interaction non-existent-command foo --bar=baz --uri=dev --root=/path/to/drupal'

Drush 12.x:

$ drush --simulate --uri=dev --no-interaction 'user@server/path/to/drupal#sitename' non-existent-command foo --bar=baz
PHP Fatal error:  Uncaught TypeError: is_dir(): Argument #1 ($filename) must be of type string, bool given in /Users/ga/Code/open-source/drupal/drush11/src/Config/ConfigLocator.php:262

I am running into a variation of this situation with the failing tests in this PR. While I am reluctant to move forward and remove too much functionality without the replacement behavior described above, I think I am going to go one step further in this PR and remove more of the bootstrap capabilities:

Drush 12 won't run at all unless it shares a vendor directory with a Drupal site. (Drupal code must exist; database et. al. does not necessarily have to be set up)
The code to select a Drupal site based on cwd will be removed completely
Selecting another Drupal site via --root or via an alias will still work, but will always redispatch

Removing #3, if we decide to do so, will be follow-on work. Doing #1 and #2 now will reduce the total amount of work needed to do to fix the tests & etc.

…upal.

greg-1-anderson · 2023-04-02T01:45:13Z

Changed bootstrapping only slightly, only broke one test. Onward to make more changes.

greg-1-anderson · 2023-04-02T14:33:42Z

The archive restore test is failing, because archive dump, when run on the Drush project, captures the SUT without an embedded Drush. This PR makes it impossible to bootstrap the restored archive. To fix this test, I'll have to inject Drush into the restored site. I think this is reasonable. Injecting Drush into the SUT so that the archive restores cleanly seems undesirable. If I do it post restore, I can use a path repository to include Drush from source.

…sh command on a Drupal site at a different location than the SUT, use the copy of Drush that's in that Drupal site.

greg-1-anderson · 2023-04-03T03:13:17Z

src/Preflight/Preflight.php

+        $root = $this->preferredSite();
+
+        // TODO: We might redispatch to some other site later, but THROW
+        // now if there is no root. Drush 12.x won't run if it is in a vendor


greg-1-anderson · 2023-04-03T03:24:41Z

Almost done; a few more adjustments to do.

I'm uneasy with blessing drush as a launcher.

Were you referring specifically to the long comment that explained how the Drush preflight worked? I interpreted your comment to refer to the functionality of the launcher itself. I removed mention of the launcher from that comment. We can continue towards the deprecation and removal of the launcher as follow-on work as described above. I'm still a little leery to remove the launcher until Drush launcher supports remote aliases, but we can at least do the pre-req work I described above before we decide what our Drush 12 strategy will be.

weitzman

Yes I was referring to that long comment. Your plan sounds good.

weitzman · 2023-04-03T07:23:06Z

src/Preflight/Preflight.php

@@ -313,6 +334,7 @@ public function preflight($argv): bool
        // redispatch to a site-local Drush, then we cannot continue.
        // This can happen when using Drush 9 to call a site-local Drush 8
        // using an alias record that is only defined in a Drush 8 format.
+        // TODO: We can remove this when we drop support for Drush 8 aliases (already done?)


yes, already done afaik

This comment appears to be incorrect; we get here any time the user tries to use an alias that cannot be found.

weitzman · 2023-04-03T07:25:28Z

tests/functional/ArchiveTest.php

+        // any site other than the one it shares its vendor directory with. We
+        // must therefore add Drush via a Composer PATH repository in the site
+        // created via archive restore before we can run Drush commands on it.
+        $this->execOnRestoredSite(['composer', 'config', 'repositories.drush', 'path', dirname(self::getDrush())]);


This is editing the composer.json in our root? If so does the test reliably undo the changes?

This is editing the composer.json on a copy / fixture Drupal site, not the SUT. The fixture is thrown away after the test.

greg-1-anderson · 2023-04-03T15:20:12Z

src/Boot/BootstrapManager.php

@@ -109,16 +109,6 @@ public function getComposerRoot(): string
        return $this->drupalFinder()->getComposerRoot();
    }

-    public function locateRoot($root, $start_path = null): void


This function was unused.

greg-1-anderson · 2023-04-03T15:37:12Z

I think this is ready to merge, after review comments, if any, are resolved.

Follow-on PRs:

Make remote dispatches launch __PROJECT_ROOT__/vendor/bin/drush instead of the drush in the PATH when drush_script is not specified.
Clone Drush 12.x and rename it drush-launcher. Remove bootstrapping. Keep preflight, alias handling and redispatch. Keep a few select commands (drush browse, drush uli, etc) that operate primarily by making remote calls to another instance of Drush.
Remove redispatch from Drush 12 preflight. (Note that drush browse et. al. are not strictly redispatches, they just make calls to other Drush instances. These sorts of commands may remain for convenience. Could consider putting them in a separate project so that the same code may be shared between Drush and the launcher.)

weitzman

Clone Drush 12.x and rename it drush-launcher. Remove bootstrapping. Keep preflight, alias handling and redispatch. Keep a few select commands (drush browse, drush uli, etc) that operate primarily by making remote calls to another instance of Drush.

Benefits of the current launcher:

rarely has an SA that it needs to respond
It likely that the launcher and the site-local Drush get out of sync. We need to keep reliably launching in this scenario.
We need to get @webflo on board with major changes to the launcher.

Make remote dispatches launch PROJECT_ROOT/vendor/bin/drush instead of the drush in the PATH when drush_script is not specified.

FYI composer config bin-dir gets you the configured bin dir. This is surely available in PHP as well.

drush.php

weitzman · 2023-04-03T16:21:07Z

drush.php

+ *                 └── drush
+ *                     └── drush.php
+ *
+ * Drush project: (Only used when developing Drush; `composer install --dev`)


Maybe remove composer install --dev? I'm not sure how it distinguishes from a regular site.

This differentiated this use case from the one below, which used to say composer install --no-dev before it was removed. We can remove this modifier here now.

weitzman · 2023-04-03T16:23:57Z

src/Preflight/Preflight.php

+
+            // Now that we have our final Drupal root, check to see if there is
+            // a site-local Drush. If there is, we will redispatch to it.
+            // NOTE: termination handlers have not been set yet, so it is okay


Can we get rid of ther Drush termination handlers (in a separate issue). I refer to the unusual exit code checking.

Maybe; need to check and see what role they are still playing. Would be simpler to not have them.

weitzman · 2023-04-03T16:25:48Z

src/Preflight/Preflight.php

-        $fallBackPath = $this->preflightArgs->selectedSite($this->config()->get('runtime.drush-script'));
-        return $this->setSelectedSite($selectedRoot, $fallBackPath);
-    }
+        $projectRoot = dirname($this->environment->vendorPath());


Composer has a configurable location for vendor https://getcomposer.org/doc/06-config.md#vendor-dir

There is no API to determine the project root from the vendor directory. Two options:

$a = include $vendor/composer/installed.php; $project_root = $a['root']['installed_path'];

Walk up from $vendor until we find a composer.json file.

The problem with 1. is that it breaks if Composer changes its internal implementation of what it writes to the vendor directory; installed.php is not guaranteed to exist, and not guaranteed to have the same contents.

The problem with 2. is that there's nothing preventing a user from putting vendor outside the project root, or deep inside a directory that might encounter some other composer.json before it gets to the root.

It's pretty rare for Drupal sites to relocate the vendor directory at all; maybe option 2 is good enough?

Maybe I'll try option 1 and fall back to 2 if $installed.php doesn't return recognizable data. That would mean that the code path in 2 would be seldom-exercised, although we could cover it in the tests.

Neither 1 nor 2 look appetizing to me. I say leave it as is.

Co-authored-by: Moshe Weitzman <weitzman@tejasa.com>

greg-1-anderson · 2023-04-03T21:21:05Z

FYI composer config bin-dir gets you the configured bin dir. This is surely available in PHP as well.

We can't check the configured Composer bin dir for remote sites. We'll have to assume vendor/bin for aliases that do not set drush-script. Aliases that do set drush-script already work.

weitzman · 2023-04-04T00:00:30Z

We can't check the configured Composer bin dir for remote sites.

Right. But we could assume that the composer codebase is the same for remote and local. Thats quite a safe assumption in the modern era. It would be slightly better than assuming vendor/bin. Happy to defer this to another day.

greg-1-anderson · 2023-04-04T01:49:43Z

We could assume that the composer codebase is the same for remote and local.

Oh, good point. $_composer_bin_dir is available for that; that variable should be set pretty much all the time, except for in the Drush project itself. We could just assume vendor/bin when it's not set. This is part of the follow-on work anyway, though.

…ger relevant.

greg-1-anderson · 2023-04-04T01:54:10Z

Removed the comment about composer install --dev; this resolves all of the review comments.

Do not allow Drush to combine autoload files from a non-site-local Dr…

105b9bb

…upal; only bootstrap Drupal sites with a shared vendor.

greg-1-anderson requested a review from weitzman as a code owner April 1, 2023 03:29

$root is not guarenteed to be set in getCommandFilePaths

fe66992

greg-1-anderson mentioned this pull request Apr 1, 2023

Remove dual autoloader feature from global Drush installs #5138

Closed

Further simplify preflight by requiring that Drush be bundled with Dr…

df25bc4

…upal.

Add drush to restored Drupal site in Archive test. When running a Dru…

1e4e1b8

…sh command on a Drupal site at a different location than the SUT, use the copy of Drush that's in that Drupal site.

greg-1-anderson commented Apr 3, 2023

View reviewed changes

greg-1-anderson marked this pull request as draft April 3, 2023 03:14

Reject global installs of Drush.

6e05427

weitzman reviewed Apr 3, 2023

View reviewed changes

greg-1-anderson added 3 commits April 3, 2023 06:24

Simplify preflight and remove conditionals

b0b87e2

More simplifications now that there must always be a Drupal root.

2187f75

Simplify CacheCommands

2f5c6c2

greg-1-anderson commented Apr 3, 2023

View reviewed changes

greg-1-anderson marked this pull request as ready for review April 3, 2023 15:31

Merge branch '12.x' into single-autoloader-only-12x

53d8ffe

weitzman reviewed Apr 3, 2023

View reviewed changes

Update drush.php

d6946a4

Co-authored-by: Moshe Weitzman <weitzman@tejasa.com>

Remove reference to "composer install --dev" in comment; it is no lon…

241141b

…ger relevant.

weitzman merged commit d439bb3 into 12.x Apr 4, 2023
1 check passed

weitzman deleted the single-autoloader-only-12x branch April 4, 2023 03:43

weitzman mentioned this pull request May 1, 2023

Remove support for Drush as a global project #4674

Closed

weitzman mentioned this pull request May 12, 2023

Execute drush with symlinked root results in recursive loop #4758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dual autoloader feature from global Drush installs #5521

Remove dual autoloader feature from global Drush installs #5521

greg-1-anderson commented Apr 1, 2023

weitzman commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023 •

edited

weitzman commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023 •

edited

greg-1-anderson commented Apr 2, 2023

greg-1-anderson commented Apr 2, 2023

greg-1-anderson Apr 3, 2023

greg-1-anderson commented Apr 3, 2023

weitzman left a comment

weitzman Apr 3, 2023

greg-1-anderson Apr 3, 2023

weitzman Apr 3, 2023

greg-1-anderson Apr 3, 2023

greg-1-anderson Apr 3, 2023

greg-1-anderson commented Apr 3, 2023

weitzman left a comment

weitzman Apr 3, 2023

greg-1-anderson Apr 3, 2023

weitzman Apr 3, 2023

greg-1-anderson Apr 3, 2023

weitzman Apr 3, 2023

greg-1-anderson Apr 3, 2023

greg-1-anderson Apr 3, 2023 •

edited

weitzman Apr 4, 2023

greg-1-anderson commented Apr 3, 2023

weitzman commented Apr 4, 2023

greg-1-anderson commented Apr 4, 2023

greg-1-anderson commented Apr 4, 2023

Remove dual autoloader feature from global Drush installs #5521

Remove dual autoloader feature from global Drush installs #5521

Conversation

greg-1-anderson commented Apr 1, 2023

weitzman commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023 • edited

weitzman commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023

greg-1-anderson commented Apr 1, 2023 • edited

greg-1-anderson commented Apr 2, 2023

greg-1-anderson commented Apr 2, 2023

Choose a reason for hiding this comment

greg-1-anderson commented Apr 3, 2023

weitzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson commented Apr 3, 2023

weitzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson Apr 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-1-anderson commented Apr 3, 2023

weitzman commented Apr 4, 2023

greg-1-anderson commented Apr 4, 2023

greg-1-anderson commented Apr 4, 2023

greg-1-anderson commented Apr 1, 2023 •

edited

greg-1-anderson commented Apr 1, 2023 •

edited

greg-1-anderson Apr 3, 2023 •

edited