SQL migrations cannot be instantiated if database is not available and Node, Migrate Drupal modules are enabled

Since we need to test this, let's put the status at NW and add the appropriate tag.

I stole copied some code from SqlBaseTest::testConnectionTypes(). In a stand-alone test, it looks a little odd that I did not change any of the strings. If anyone cares, then I am happy to change that.

I am also happy to add comments. It all seems so obvious after staring at testConnectionTypes() for half an hour.

From the several scenarios in that test, I chose the one that leads to the shortest decision path in SqlBase::getDatabase().

After the setup, the test calls getDatabase() and checks the exception. Easy peasy.

I am attaching a new patch and a test-only patch, which is also the interdiff.

P.S. There are two reasons to add a new test instead of adding a few lines to testConnectionTypes():

You can only use expectException() once in a test. If you want to test two exceptions, then use two tests or put at least one of them in a try/catch block.
testConnectionTypes() is all about testing the code paths in getDatabase(), and only incidentally ends with a test for an exception. The new test is looking for something different.

The test-only patch failed as expected:

There was 1 failure:

1) Drupal\Tests\migrate\Kernel\SqlBaseTest::testBrokenConnection
Failed asserting that exception of type "PDOException" matches expected exception "Drupal\migrate\Exception\RequirementsException". Message was: "SQLSTATE[HY000] [2002] php_network_getaddresses: getaddrinfo for no_such_host failed: Name or service not known" at
/var/www/html/core/modules/mysql/src/Driver/Database/mysql/Connection.php:165

@mikelutz:

Thanks for sharing Comment #10 here even though (as you were the first to admit) it was not entirely thought out. The comment is copied from #3326468: [meeting] Migrate Meeting 2022-12-15 1400Z.

It took me a while to make sense of "In reality, that’s the first place getDatabase() is called". It turns out that this is true when the node and migrate_drupal modules are installed. Here is part of the stack trace from #3327401: [meeting] Migrate Meeting 2022-12-22 2100Z:

Drupal\mysql\Driver\Database\mysql\Connection::open(Array) (Line: 445)
Drupal\Core\Database\Database::openConnection('upgrade', 'default') (Line: 188)
Drupal\Core\Database\Database::getConnection('default', 'upgrade') (Line: 203)
Drupal\migrate\Plugin\migrate\source\SqlBase->setUpDatabase(Array) (Line: 156)
Drupal\migrate\Plugin\migrate\source\SqlBase->getDatabase() (Line: 224)
Drupal\migrate\Plugin\migrate\source\SqlBase->checkRequirements() (Line: 105)
Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase->checkRequirements() (Line: 81)
Drupal\node\Plugin\migrate\D6NodeDeriver->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 255)
Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions() (Line: 181)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 102)
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array) (Line: 83)

Another point from that meeting: MigrationPluginManger::createInstance() (not to be confused with the method of the same name in MigratePluginManager) just calls createInstances().

Based on that, I am updating the title of this issue. Also the steps to reproduce in the issue summary. The problem is that createInstances() does not work for a SQL migration when the node and migrate_drupal modules are enabled and the source database is not available.

The attached patch catches either a PDOException or a ConnectionNotDefinedException in checkRequirements() and re-throws as a RequirementsException, as suggested in #10. This is different enough from the approach in #5 that I am not attaching an interdiff.

The patch also adds a test to SqlTestBasse and makes necessary changes to the existing tests, same as the patch in #5. The current patch adds two new kernel tests: one in the migrate module and one in the migrate_drupal module. The tests are very similar, and we probably do not want to keep both. I would like some opinions on that.

PHPStan caught a serious mistake on my part: copy/pasting code and forgetting to define one of the referenced variables.

We could do some restructuring, introducing a new, protected method. For future reference, I have done this in the attached "radical" patch. If we do that much, then we should probably go further: deprecate, then remove the protected method setUpDatabase(). But I think that is out of scope for this issue. I was already nervous about the changes to that function, from a BC point of view.

So the non-radical patch attached to this comment reverts some of the previous changes. The original try/catch is back where it started, and the corresponding change to the existing test is reverted.

I have attached interdiffs for both of the new patches, and also a new test-only patch.

I wonder whether fixing this issue will affect the annoying (but harmless) error message

Failed to connect to your database server. The server reports the following message: No database connection configured for source plugin variable.

discussed in #3198339: Improve error reporting from migrate_drupal_migration_plugins_alter() and #3221087: Suppress error about missing requirements for migrating variables when using Drupal 8+ as a source. I am adding those as related issues.

Fixing this issue does not affect the error message I mentioned in #16. For more details on my testing, see #3198339-15: Improve error reporting from migrate_drupal_migration_plugins_alter().

@smustgrave:

Thanks for the review!

I am uploading a new patch (also a new test-only patch and an interdiff). I added return-type declarations to all the new functions: those in the new test module and also new test methods. I will add a change record next.

I added a draft change record.

The test-only patch fails as expected. The full patch fails on CKEditor5AllowedTagsTest, which is a FJS test. That test does not enable the migrate module, so it should not be affected by this patch.

Back t o NR.

This is another FJS test failure. It is the same test mentioned in #3055983: Locks on SQLite - consistent fails on PHP 8.4 and PHP 8.5. Back to RTBC.

Thank you, @longwave! Now that this issue is fixed, I un-postponed #3063856: Add ability to view migrate_message table data.

Maybe the test fails with SQLite because it starts with the default database and then changes the host key. If so, then always using mysql for the test database connection should fix the test.

I was in a rush and uploaded the wrong patch. Try again ...

One test fixed, one to go.

I am uploading a new patch, an interdiff comparing it to the patch in #19 (not #35), and a test-only patch.

If this does not work, then I should re-postpone #3063856.

I am setting the status to NW for #39. I will update the patch, but maybe not today.

Is this what you had in mind in #39?

I considered using 'deadbeat_dad' as the missing database, but decided on the more literary 'godot' (as in Waiting for ...).

I am attaching an interdiff comparing to the patch in #19, since this patch un-does most of the change between #19 and #36. If this patch passed on MySQL and SQLite, then I will upload a test-only patch.

Both of the SQLite failures are the same:

Failed asserting that exception of type "Drupal\Core\Database\DatabaseNotFoundException" matches expected exception "Drupal\migrate\Exception\RequirementsException". Message was: "SQLSTATE[HY000] [14] unable to open database file" at
/var/www/html/core/modules/sqlite/src/Driver/Database/sqlite/Connection.php:112

I am keeping the status at NR to reconsider the patch in #36.

Alternatively, we could catch additional exceptions (and re-throw them as RequirementsExceptions) or maybe there is another interpretation of the suggestion in #39.

OK, the testbot shows that the patch in #41 is really a bad idea. (I wrote #42 while still waiting for the results of the MySQL test.)

Again: NR to reconsider the patch in #36.

While I am at it, I will update the list of remaining tasks in the issue summary.

Using git bisect is fun, isn't it? I identified the same commit before I reloaded this page and saw the last few comments. I did not get as far as examining what that commit changed.

We understand why the patch in #36 no longer works. But when I uploaded the patch in #41, there were 4 failures and now there is only one. I am still a little confused by that, but I am not going to track it down.

I think #49 is the right idea, if you mean DatabaseConnectionRefusedException. Here is a patch that does that, based on the patch in #41. (Reminder: the only difference between #36 and #41 is the tests.)

On second thought, maybe #49 did mean DatabaseException, the interface implemented by DatabaseConnectionRefusedException. Here is a patch that implements that, with an interdiff comparing to #41.

Try again, this time keeping CodeSniffer happy.