Improve and generalize database dump tools

Found some nits, but only one thing glaringly wrong. There needs to be a way to specify certain tables which only exist in D6 and D7 should be dumped schema-only. Maybe a --schema-only option?

1. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,64 @@
   + * Contains \Drupal\Core\Command\DbCommandBase
   

   Nit: Needs a period at the end.

2. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,64 @@
   +  protected function configure() {
   

   No doc comment.

3. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,64 @@
   +  protected function getDatabaseConnection(InputInterface $input) {
   +
   +    // Load connection from a url.
   

   Nit: There's an extra line here.

4. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,64 @@
   +      // ensure database connection isn't set.
   

   Nit: s/ensure/Ensure

5. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,64 @@
   +      if (Database::getConnectionInfo('db-tools')) {
   +        throw new \RuntimeException('Database "db-tools" is already defined. Can not define database provided.');
   +      }
   

   Why are we relying on a specific connection ID being defined? This at least needs a comment to explain.

6. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -280,21 +261,23 @@ protected function getTableCollation($table, &$definition) {
   +    $query = $connection->query("SELECT * FROM {" . $table . "} " . $order );
   

   Nit: There's an extra space before the closing parenthesis.

7. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -388,8 +377,6 @@ protected function getTemplate() {
     * installation of Drupal 8. It has the following modules installed:
   - *
   -{{MODULES}}
   

   If we're removing the list of modules, we shouldn't say "It has the following modules installed" :)

8. +++ b/core/lib/Drupal/Core/Command/DbImportCommand.php
   @@ -0,0 +1,74 @@
   +  protected function execute(InputInterface $input, OutputInterface $output) {
   +
   +    $script = $input->getArgument('script');
   

   Nit: There's an extra line.

9. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
   @@ -0,0 +1,182 @@
   +  public function testDbImportCommand() {
   +
   +    /** @var \Drupal\Core\Database\Connection $connection */
   +    $connection = $this->container->get('database');
   

   Nit: Empty extra line.

10. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
    @@ -0,0 +1,182 @@
    +        ->tableExists($table), strtr('Table @table created by the database script.', ['@table' => $table]));
    

    strtr()? Why not sprintf()?

This allows us to remove massive WTF-ery from Migrate, so escalating to major.

Pending DrupalCI's OK for SQLite and PostgreSQL, I approve. This beats the tar out of migrate-db.sh.

Dammit! I have kick this back to NW because of something we still need. To quote myself in #9:

There needs to be a way to specify certain tables which only exist in D6 and D7 should be dumped schema-only.

This would give the tool parity with migrate-db.sh, and thus we'll be able to remove it quickly and painlessly :)

Found a lot of nits, but nothing functionally wrong!

1. +++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
   @@ -0,0 +1,65 @@
   +        throw new \RuntimeException('Database "db-tools" is already defined. Can not define database provided.');
   

   s/Can not/Cannot. Fixable on commit.

2. +++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
   @@ -56,80 +41,77 @@ class DbDumpCommand extends Command {
   +      if (empty($schema_only_patterns) || preg_replace($schema_only_patterns, '', $table)) {
   

   Why use preg_replace() here? What's wrong with preg_match()?

3. +++ b/core/lib/Drupal/Core/Command/DbImportCommand.php
   @@ -0,0 +1,73 @@
   +  /**
   +   * Run the database script.
   +   *
   +   * @param \Drupal\Core\Database\Connection $connection
   +   *   ...
   +   * @param string $script
   +   *   The PHP script.
   +   * @return mixed
   +   */
   +  protected function runScript(Connection $connection, $script) {
   

   $connection needs a proper doc comment :)

   $script should be specify that it's the path to a dump file script, not a string of PHP code to be executed.

   Why @return mixed? As far as I can tell, this method returns nothing.

4. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
   @@ -0,0 +1,137 @@
   +    $command = new DbCommandBaseTester();
   +    $command_tester = new CommandTester($command);
   

   This is repeated throughout the test, can it be a protected property created in setUp()?

5. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbDumpCommandTest.php
   @@ -0,0 +1,85 @@
   +      $this->markTestSkipped("Skipping test since the DbDumpCommand is currently only compatible with MySql");
   

   Nit: s/MySql/MySQL

6. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbDumpCommandTest.php
   @@ -0,0 +1,85 @@
   +    $output = $command_tester->getDisplay();
   +    $this->assertTrue(strpos($output, "createTable('router"), 'Table router found');
   +    $this->assertTrue(strpos($output, "insert('router"), 'Insert found');
   +    $this->assertTrue(strpos($output, "'name' => 'test"), 'Insert name field found');
   +    $this->assertTrue(strpos($output, "'path' => 'test"), 'Insert path field found');
   +    $this->assertTrue(strpos($output, "'pattern_outline' => 'test"), 'Insert pattern_outline');
   

   I can't tell what exactly this is testing. It would benefit enormously from an explanatory comment.

7. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbDumpCommandTest.php
   @@ -0,0 +1,85 @@
   +    $output = $command_tester->getDisplay();
   +    $this->assertTrue(strpos($output, "createTable('router"), 'Table router found');
   +    $this->assertFalse(strpos($output, "insert('router"), 'Insert found');
   +    $this->assertFalse(strpos($output, "'name' => 'test"), 'Insert name field found');
   +    $this->assertFalse(strpos($output, "'path' => 'test"), 'Insert path field found');
   +    $this->assertFalse(strpos($output, "'pattern_outline' => 'test"), 'Insert pattern_outline');
   +
   +    $command_tester->execute(['--schema-only' => 'route.*']);
   +    $output = $command_tester->getDisplay();
   +    $this->assertTrue(strpos($output, "createTable('router"), 'Table router found');
   +    $this->assertFalse(strpos($output, "insert('router"), 'Insert found');
   +    $this->assertFalse(strpos($output, "'name' => 'test"), 'Insert name field found');
   +    $this->assertFalse(strpos($output, "'path' => 'test"), 'Insert path field found');
   +    $this->assertFalse(strpos($output, "'pattern_outline' => 'test"), 'Insert pattern_outline');
   

   Same here.

DrupalCI approves! This patch makes me starry-eyed and sets my heart a-flutter. Totally, unabashedly RTBC.

This looks really awesome. I'll admit I didn't do a line-by-line comparison with migrate-db.sh but there's not a lot to complain about in this patch.

My one concern with this patch is it doesn't also remove migrate-db.sh. Which means if that doesn't get done by Sunday/Monday, we're at risk of shipping RC1 / 8.0.0 with both scripts, which is pretty silly.

Adam explained that this is because the patch to remove migrate-db.sh also needs to re-generate all the dumps and that's going to be an 11,000 GB patch so best to do on its own. That makes sense, but if folks could try really hard to get this done over the next 24-48 hours, it would be very much appreciated.

Committed and pushed to 8.0.x. Thanks!