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

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 📌 Improve error reporting from migrate_drupal_migration_plugins_alter() Needs work and #3221087: fallback on 'migrate' as the database key causes error messages if migrations use a different 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() → .

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Can the new functions in MigrateMissingDatabaseSource be typehinted.

Also was previously tagged for a change record.

@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 last submitted patch, 19: 3312733-19-test-only.patch, failed testing. View results →

The last submitted patch, 19: 3312733-19.patch, failed testing. View results →

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 looks good to me. Reran the tests for the full patch in #19 but seemed to be random ckeditor before.

The last submitted patch, 19: 3312733-19.patch, failed testing. View results →

It was a random test failure, restoring RTBC

The last submitted patch, 19: 3312733-19.patch, failed testing. View results →

This is another FJS test failure. It is the same test mentioned in 🐛 Occasional locks on SQLite Active . Back to RTBC.

Committed and pushed to 10.1.x and published the change record, thanks. Not eligible for backport as there is a change to exception handling.

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

Rolled this back; the tests here don't work on SQLite: https://www.drupal.org/pift-ci-job/2616192 →

1) Drupal\Tests\migrate\Kernel\MigrateMissingDatabaseTest::testMissingDatabase
Failed asserting that exception message 'Destination plugin 'null' did not meet the requirements' contains 'No database connection available for source plugin migrate_missing_database_test'.

1) Drupal\Tests\migrate\Kernel\SqlBaseTest::testBrokenConnection
Failed asserting that exception of type "Drupal\migrate\Exception\RequirementsException" is thrown.

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.

The last submitted patch, 36: 3312733-36-test-only.patch, failed testing. View results →

SQLite failed on Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest. I am retesting

Would it be better to set database, instead of host, to some invalid value? That should work on SQLite as well as MySQL and makes the test slightly more valid as it is testing the exception across all drivers.

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.

The last submitted patch, 41: 3312733-41.patch, failed testing. View results →

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.

I applied the patch in #36 and tested with

phpunit --filter testBrokenConnection core/modules/migrate/tests/src/Kernel/SqlBaseTest.php 

That failed with

1) Drupal\Tests\migrate\Kernel\SqlBaseTest::testBrokenConnection
Failed asserting that exception of type "Drupal\Core\Database\DatabaseConnectionRefusedException" matches expected exception "Drupal\migrate\Exception\RequirementsException". Message was: "SQLSTATE[HY000] [2002] php_network_getaddresses: getaddrinfo for no_such_host failed: No address associated with hostname [Tip: This message normally means that there is no MySQL server running on the system or that you are using an incorrect host name or port number when trying to connect to the server. You should also check that the TCP/IP port you are using has not been blocked by a firewall or port blocking service.] " at
/var/www/html/core/modules/mysql/src/Driver/Database/mysql/Connection.php:174
/var/www/html/core/lib/Drupal/Core/Database/Database.php:451
/var/www/html/core/lib/Drupal/Core/Database/Database.php:194
/var/www/html/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php:193
/var/www/html/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php:148
/var/www/html/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php:238
/var/www/html/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php:214
/var/www/html/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php:154
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

I then applied the patch in #41 and tested again with

phpunit --filter testBrokenConnection core/modules/migrate/tests/src/Kernel/SqlBaseTest.php 

and the test passed.

So, for this one test I am getting opposite results from what testbot reports.

It is too late to investigate further.

In Slack @benjifisher said that #36 passes on a checkout from March 10.

I used bisect to determine when the failures started. It was 🐛 Add informative error message for 'Connection refused' errors in MySQL Fixed in commit 442c0e41.

I've restarted tests on #36.

The patch in #41, passes when run on a checkkout before 🐛 Add informative error message for 'Connection refused' errors in MySQL Fixed .

So, the other issue has changed \Drupal\mysql\Driver\Database\mysql\Connection to catch the \PDOException, which we expect here, and rethrows a new DatabaseConnectionRefusedException. A similar change was not made for pgsql or sqllite.

Maybe just add a catch for a DatabaseException in \Drupal\migrate\Plugin\migrate\source\SqlBase::checkRequirements?

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.

I agree that `git bisect` can be fun to use. :-)

Yes, that is the change I was suggesting. I am pleased we agree. And I think that will cover future changes to the exceptions. Back to RTBC.

Hiding old patches, as it looks like the patch from #52 is the one!

Looks good, simplifies the code and lots more test coverage.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.