Fixes #5999: Allow drush site:install to work with recipes. #6026
Conversation
greg-1-anderson
force-pushed
the
si-with-recipe
branch
from
12ccd26
to
3ba4101
Compare
|
I did a little more ad-hoc testing, and also provided a functional test. Seems to work as one might expect. |
|
Right now the code reverts to the profile-checking code if the provided path isn't a recipe. This leads to an odd error if the path is wrong, for example. Since profile names cannot contain /, perhaps we shoudl give a better error message when a path is used. Testing for Drupal >= 10.3.0 would also be a good idea. |
|
But |
There was a problem hiding this comment.
This looks great .
- I'm a little hesitant to re-use the argument for both profile and recipe. An alternative would be to add a new --recipe option. If we do that we would want a -r short option because those accept spaces before the value and thus are easier to tab complete with a path. This approach is more compatible with bash completion as well.
- Does this PR work as expected these re-install scenarios? (Update: MW: recipes have no persistence at all so I guess these work fine)
- User does not specify a profileOrRecipe
- User specifies a recipe that is different from the installed site
- Once recipes support user input (see https://www.drupal.org/project/distributions_recipes/issues/3304895), are we well positioned to support it? We already accept input via trailing optional key=value arguments. I guess approach works for recipes as well.
- We should add that improved error message when the recipe (or profile) is not found
| @@ -50,7 +50,7 @@ public function __construct( | |||
| * Install Drupal along with modules/themes/configuration/profile. | |||
| */ | |||
| #[CLI\Command(name: self::INSTALL, aliases: ['si', 'sin', 'site-install'])] | |||
| #[CLI\Argument(name: 'profile', description: 'An install profile name. Defaults to <info>standard</info> unless an install profile is marked as a distribution. Use <info>minimal</info> for a bare minimum installation. Additional info for the install profile may also be provided with additional arguments. The key is in the form <info>[form name].[parameter name]</info>')] | |||
| #[CLI\Argument(name: 'profile', description: 'An install profile name, or a path to a directory containing a recipe. Defaults to <info>standard</info> unless an install profile is marked as a distribution. Use <info>minimal</info> for a bare minimum installation. Additional info for the install profile may also be provided with additional arguments. The key is in the form <info>[form name].[parameter name]</info>')] | |||
There was a problem hiding this comment.
Lets rename this arg to recipeOrProfile, if we are keeping this multi-purpose argument.
There was a problem hiding this comment.
I can go with either decision on whether profile and recipe are combined or separate, but I would lean towards keeping them combined for uniformity with the "drupal install" command in Drupal core. Agree about the rename.
… e.g. Drupal versions and missing recipes.
|
I'm not sure why the failing code was running in fdd05b0, seems like a pre-existing edge case. Just marked the test skipped in cce1a82. I'd suggest that we merge this to 13.x-dev to get a little broader testing, and handle input for recipes as follow-on work, once it's supported in Drupal. LMK if there are any further changes you'd like to see here before we merge. |
| @@ -70,12 +70,13 @@ public function __construct( | |||
| #[CLI\Usage(name: 'drush si --account-pass=mom', description: 'Re-install with specified uid1 password.')] | |||
| #[CLI\Usage(name: 'drush si --existing-config', description: 'Install based on the yml files stored in the config export/import directory.')] | |||
| #[CLI\Usage(name: 'drush si standard install_configure_form.enable_update_status_emails=NULL', description: 'Disable email notification during install and later. If your server has no mail transfer agent, this gets rid of an error during install.')] | |||
| #[CLI\Usage(name: 'drush si core/recipes/standard', description: 'Install from the core Standard recipe.')] | |||
There was a problem hiding this comment.
The path is relative to the CWD and not Drupal root, right? Lets state this like we do for other path related arguments.
There was a problem hiding this comment.
The path is currently relative to the Drupal root. Relative to CWD is more consistent to other Drush commands, though. I'll adjust the code and comment.
There was a problem hiding this comment.
Decided to search both cwd and Drupal root. Core recipes are in core/recipes, so Drupal-root-relative recipe paths are most convenient for installing those. If a module provided a recipe, then a Drupal-root-relative path would also be helpful. The Starshot prototype ships recipes in the "recipes" directory, located at the project root, so cwd-relative recipe paths are useful for anything that follows that convention.
We could consider refining this further in the future, e.g. maybe automatically searching DRUPAL_ROOT/core/recipes, or CWD/recipes when a simple name is provided. Note, however, that "standard" exists as both a recipe and a profile. The result of installing either of these looks about the same; I am not sure if there might be differences, or if shifting the default from the standard profile to the standard recipe would have any noticeable impact.
There was a problem hiding this comment.
If we did the above, and someone converted demo_umami to a recipe, then @rfay wouldn't need to learn to type a different drush si command. This was an unintended side benefit of the thought, not the primary motivator.
There was a problem hiding this comment.
@rfay The above was just a humorous reference to #6026 (comment); that exact command would work as written to install from a recipe, if demo_umami were reimplemented as a recipe. You might have to type "starshot" instead of "demo_umami", though, as things evolve. Who knows, someone might build "starshot_umami".
tests/functional/SiteInstallTest.php
Outdated
| */ | ||
| public function testSiteInstallRecipe() | ||
| { | ||
| if (version_compare(\Drupal::VERSION, '10.3.0') < 0) { |
There was a problem hiding this comment.
Lets use $this->isDrupalGreaterThanOrEqualTo().
Co-authored-by: Moshe Weitzman <weitzman@tejasa.com>
* Allow drush site:install to work with recipes. * Code style * Add a test for installing from a recipe. * Touch up comments * Code style * Skip recipe tests on versions of Drupal that do not yet support recipes. * Rename "profile" arg to "recipeOrProfile". Add better error checking, e.g. Drupal versions and missing recipes. * Code style * Use test recipe on recipe-not-supported test. * Mark the recipe-not-supported test as skipped on sqlite. * Update src/Commands/core/SiteInstallCommands.php Co-authored-by: Moshe Weitzman <weitzman@tejasa.com> * Search paths relative to cwd when looking for recipes. --------- Co-authored-by: Greg Anderson <greg.anderson@greenknowe.org> Co-authored-by: Moshe Weitzman <weitzman@tejasa.com>
POC was pretty straightforward. Lightly tested. Seems to work with core standard recipe. Have not tried with Starshot. Additional work may be needed. Needs tests.