[PP-1] Provide commands which are helpful for core development

Makefile

Sorry, meant to paste link https://github.com/alexpott/d8githooks/issues/21

Such a makefile is just a neat way of storing aliases.

The point of this issue is to have core offer a replacement for drush en some_module (for example) that doesn't actually require drush. A Makefile isn't going to help here as you need something to bootstrap Drupal and call the module installer with the provided argument. Your makefile could just offer a shorter way of calling that command then although it should be as simple as ./scripts/enable-module views.

right, I've missed the point then - carry on :)

Here's the start of a patch that adds an optional db-url option to the wonderful install command that's already done. The default is the current sqlite usage.

Probably this should be split into an issue for each command. I couldn't find another issue that is more recent.

My reason to come back to this is that it's getting hard to juggle local drush/console and core development for novice contributors. I think this would help a lot to simplify quicksprint tools. In that use case, the environment is setup already and a script runs commands for the user to install the site (requested by mentors that novices have the environment with Drupal already installed).

Back to needs work because it's not complete even though it passes current tests.

+++ b/core/lib/Drupal/Core/Command/InstallCommand.php
@@ -51,6 +51,7 @@ protected function configure() {
+      ->addOption('db-url', NULL, InputOption::VALUE_OPTIONAL, 'Set the database to use for the install. This is an advanced option suitable for Drupal core development.', 'sqlite://localhost/files/.sqlite')

Should be sqlite://localhost/.sqlite as site path will get prefixed anyway.

I am working on this issue

I re-rolled #15 because I wasn't able to apply the patch on 8.9.x anymore. I'm going to try to work on a test for this today.

+++ b/core/lib/Drupal/Core/Command/InstallCommand.php
@@ -51,6 +51,7 @@ protected function configure() {
+      ->addOption('db-url', NULL, InputOption::VALUE_OPTIONAL, 'Set the database to use for the install. This is an advanced option suitable for Drupal core development.', 'sqlite://localhost/.sqlite')

Looking at how we would be able to get a test written. I think that db-url is probably not the right parameter name because it does not give a chance to add prefix.

I think there was a movement to get rid of db urls in general so it probably wouldn't make sense to introduce it here.

A JSON string isn't really usable so I guess multiple options for

driver, user, password, port, host, database name and prefix.

I updated the patch to splits db-url option into the parts that can be set in the core installer tasks form. I'm getting 1 fail locally so flipping to Needs review to see if I can replicate on the testbot and then back to Needs work if it passes.

I tried to refactor QuickStartTest::tearDown per the @todo in #2926633: Provide a script to install a Drupal testsite for writing a InstallCommandTestBase that could test each core driver functionally, but I didn't get too far.

@xjm suggested creating issues for each common command in #3089277: Provide core CLI commands for the most common features of Drush (#36) so maybe just focusing on quick-start / install refactor and additional tests would be good scope for this issue? Most of the commands are implemented with the separate console app in #2242947: Integrate Symfony Console component to natively support command line operations.

Mysql and Sqlite don't have any test regressions though on the testbot, yay. Back to Needs work for tests.

It looks like I couldn't instantiate and run the TestSiteApplication class directly without phpunit AbstractPhpProcess dying. I followed the only usage in core by running test-site.php in a separate process.

This patch refactors QuickStartTest to use new InstallCommandTestBase base test class.

1. +++ b/core/lib/Drupal/Core/Command/InstallCommand.php
   @@ -50,9 +50,17 @@ protected function configure() {
   +      ->addOption('database-driver', 'db-driver', InputOption::VALUE_REQUIRED, 'Sets the database driver to use.', 'sqlite')
   +      ->addOption('database-username', 'db-user', InputOption::VALUE_REQUIRED, 'Sets the database user.', '')
   +      ->addOption('database-password', 'db-pass', InputOption::VALUE_REQUIRED, 'Sets the database password.', '')
   +      ->addOption('database-host', 'db-host', InputOption::VALUE_REQUIRED, 'Sets the database host name.', 'localhost')
   +      ->addOption('database-port', 'db-port', InputOption::VALUE_REQUIRED, 'Sets the database port.', '')
   +      ->addOption('database-name', 'db-name', InputOption::VALUE_REQUIRED, 'Sets the database to use either a database name or file path.', 'drupal')
   +      ->addOption('database-prefix', 'db-prefix', InputOption::VALUE_REQUIRED, 'Sets an optional database prefix.', '')
   

   Shortcuts are short options, not aliases. Maybe just leave NULL for now since there are a lot of short options to come up with letters and numbers for.

2. +++ b/core/lib/Drupal/Core/Command/InstallCommand.php
   @@ -141,9 +169,16 @@ protected function install($class_loader, SymfonyStyle $io, $profile, $langcode,
   +            'advanced_options' => [
   ...
   +            ],
   

   Form name is collapsed as "DRIVER[host]" not "DRIVER[advanced_options][host]".

- Added a basic install command tests for mysql and pgsql drivers.
- Fixed issues in #23.

Need to add functional tests to assert error messages returned from missing or bad options.

+++ b/core/tests/Drupal/Tests/Core/Command/InstallMysqlCommandTest.php
@@ -0,0 +1,83 @@
+ * @requires extension pdo_mysql

+++ b/core/tests/Drupal/Tests/Core/Command/InstallPgsqlCommandTest.php
@@ -0,0 +1,83 @@
+ * @requires extension pdo_pgsql

I guess this fails the test rather than skipping it. Maybe should change to an extension_loaded check?

Maybe instead of making multiple (skipped) tests for each driver, it should just be one test and our test infrastructure ensures it works for each core driver...

It looks like I forgot to pass in host. That'll be a good negative test case to add.

This should resolve getting the test started on drupalci.

I'm not sure why it's getting stock on the language page. This isn't happening on the quick start test though quick-start is always passing in langcode. However, I was able to reproduce the test failure locally and passing langcode didn't help. It doesn't seem to be an issue with the InputOption::VALUE_OPTIONAL (which should be VALUE_REQUIRED because an input options are always optional and that state means that langcode _could_ be NULL if --langcode is provided). But despite all of that the test is still getting stuck on the Choose language page.

I also added a second test to test Spanish in order to expand the test coverage.

Both are probably going to fail at the language page, but QuickStartTest still succeeds (probably because the wrapped command passes langcode as an InputArray?).

1. +++ b/core/tests/Drupal/Tests/Core/Command/InstallCommandTest.php
   @@ -0,0 +1,149 @@
   +    $base_url = getenv('SIMPLETEST_BASE_URL');
   ...
   +    define('DRUPAL_TEST_IN_CHILD_SITE', FALSE);
   ...
   +    $response = $guzzle->get($base_url, ['cookies' => $cookieJar]);
   ...
   +    $base_url = getenv('SIMPLETEST_BASE_URL');
   ...
   +    define('DRUPAL_TEST_IN_CHILD_SITE', FALSE);
   ...
   +    $response = $guzzle->get($base_url, ['cookies' => $cookieJar]);
   

   I had figured that the installer wasn't working at all, but it was, and the request redirected to the install page because it didn't detect a valid install.

   Removing the tearDown reveals a valid site that works when doing ln -s sites/simpletest/MY_PREFIX/settings.php sites/default/settings.php

   So something mus tbe wrong with the base url used or detecting that the site should use a the test database.

2. +++ b/core/tests/Drupal/Tests/Core/Command/InstallCommandTestBase.php
   @@ -0,0 +1,87 @@
   +        $command_line = $this->php . ' core/scripts/test-site.php tear-down ' . $prefix . ' --db-url "' . $db_url . '" --keep-lock';
   
   +++ b/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php
   @@ -23,63 +19,13 @@
   -        // @todo use the tear down command from
   -        //   https://www.drupal.org/project/drupal/issues/2926633
   

   A consequence of this change is that run-tests.sh --keep-results and --die-on-fail are no longer viable as the tearDown method will remove the site.

Patch no longer applies. After I re-rolled, I stumbled across the misnamed QuickStartTestBase and the BuildTests test suite. If we write the install command tests based on that test suite, then we won't need to worry about as much cleanup! Unfortunately (and ironically) QuickStartTest can't be migrated to use QuickStartTestBase because BuildTests creates a php server on its own. It might be worth it to do it as a follow-up. I reverted that part of #32, and focused soley on InstallCommandTest.

Note that despite its name QuickStartTestBase only uses the install command and not the quick-start command. I changed the documentation to be less confusing.

1. +++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
   @@ -302,21 +302,25 @@ public function assertCommandExitCode($expected_code) {
   -   * @param string $working_dir
   +   * @param null $working_dir
   

   PhpStorm autocorrected this incorrectly, and it should be reverted :(

2. +++ b/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php
   @@ -302,21 +302,25 @@ public function assertCommandExitCode($expected_code) {
   +  public function executeCommand($command_line, $working_dir = NULL, $timeout = 300, $idleTimeout = 300) {
   ...
   -      ->setTimeout(300)
   -      ->setIdleTimeout(300);
   +      ->setTimeout($timeout)
   +      ->setIdleTimeout($idleTimeout);
   

   I was going to bump the timeout of the process, but it looks like things are passing for me locally.

   This probably can be reverted or maybe put into use?

This patch removes changes from BuildTestBase, moves the pdo_sqlite extension requirement into the QuickStart command rather than the InstallCommand, and adds InstallCommandTest coverage for site already installed. I removed the explicit langcode option as it should come across as the default option for InstallCommand.

+++ b/core/tests/Drupal/BuildTests/QuickStart/QuickStartTestBase.php
@@ -6,7 +6,7 @@
- * Helper methods for using the quickstart feature of Drupal.
+ * Helper methods for using the install feature of Drupal.

@@ -25,7 +25,7 @@ abstract class QuickStartTestBase extends BuildTestBase {
-   * Install a Drupal site using the quick start feature.
+   * Installs a Drupal site using the install feature.

this kind of fixes better off to separate issue

Addressed nitpicks mentioned in #40 & resolved custom commands failures in provided patch

+++ b/core/tests/Drupal/BuildTests/Core/Command/InstallCommandTest.php
@@ -119,8 +119,8 @@ public function testInstallWithLangcode() {
-      '--langcode=es',
-      "--site-name='Prueba del Sitio {$this->testDb->getDatabasePrefix()}'",
+      '--langcode=en',
+      "--site-name='Test site {$this->testDb->getDatabasePrefix()}'",

@@ -135,11 +135,11 @@ public function testInstallWithLangcode() {
-    $this->assertErrorOutputContains('¡Enhorabuena, ha instalado Drupal!');
+    $this->assertErrorOutputContains('Congratulations, you installed Drupal!');
...
-    $this->assertStringContainsString('Prueba del Sitio ' . $this->testDb->getDatabasePrefix(), $content);
+    $this->assertStringContainsString('Test site ' . $this->testDb->getDatabasePrefix(), $content);

This isn't right. We do want to test the install in a different language using a non-default langcode. This should be reverted.

The major issue with the patch is that test classes don't create an environment correctly and the test environment is mutated.

Hiding patches after opening merge request.

Applied a suggestion from @Mile23.

I think this should be postponed on #3453474: CLI entry point in Drupal Core, and possibly closed as duplicate in favor of multiple issues per command to reduce scope.

Closing as outdated. Some of the work on the install refactor I did probably can be used in a child issue of #3582246: [meta] CLI in Core community initiative.