#5686 Fix issue "Drush installation is not detected for other site al… #5687
Conversation
…ias when having multiple local Drupal installations with aliases"
|
fixes #5686 |
src/Runtime/Runtime.php
Outdated
| @@ -55,11 +55,11 @@ public function run($argv): int | |||
| protected function doRun($argv, $output): int | |||
| { | |||
| // Do the preflight steps | |||
| $status = $this->preflight->preflight($argv); | |||
| $preflightSuccess = $this->preflight->preflight($argv); | |||
There was a problem hiding this comment.
false does not indicate failure, so I would rename this something like $preflightDidRedispatch
There was a problem hiding this comment.
Thanks for the feedback, this should be fixed now.
src/Runtime/Runtime.php
Outdated
|
|
||
| // If preflight signals that we are done, then exit early. | ||
| if ($status) { | ||
| return $status; | ||
| if ($preflightSuccess) { |
There was a problem hiding this comment.
Here is the real bug. The original code was if ($status !== false). That is, Preflight would return bool false if doRun should continue, and an integer status code if a redispatch happened. This sort of coding convention was common in the PHP 5 era, but is no longer considered de rigor in PHP 8, and in fact is a deprecated practice.
There was a problem hiding this comment.
Thanks for the feedback, this should be fixed now.
| // integer. This method is supposed to return true when successful, so | ||
| // we need to convert it to a boolean accordingly. | ||
|
|
||
| return $status ? false : true; |
There was a problem hiding this comment.
This function should always return true if it gets to this point. The comment in the header that says "and was returned successfully" was in error. See my other comment for a description of how the code used to work.
I think that the best fix would be to change this function to use return [false, 0]; wherever it currently uses return false, and change this line to return [true, $status];. Then, doRun must record both return values, call them $preflightDidRedispatch and $status or something similar. It should then return $status if $preflightDidRedispatch is true.
There was a problem hiding this comment.
Thanks for the feedback, this should be fixed now.
There was a problem hiding this comment.
Thanks for looking at this. The original bug was caused by optimizing away a type check (!== false). I suggest a better fix would be to change the return tupe of the preflight function. Since preflight is an internal function, I don't think we need to consider this a breaking change.
There was a problem hiding this comment.
LGTM, Thank you for working on this problem.
@weitzman Does this resolution look OK to you?
|
Yes, LGTM. The one suggesion I have is to replace the various new |
There was a problem hiding this comment.
Use \Drush\Commands\DrushCommands::EXIT_SUCCESS as suggested by @weitzman
…ias when having multiple local Drupal installations with aliases"