DrupalSqlBase::getSystemData() has a design quirk that it eats exceptions and just treats the failures as an empty system data array. This works fine in practice but its also the core of the logic in DrupalSqlBase::checkRequirements() where this is a usability issues.

For example, a connection string error shows the following for the d6_filter_format migration:

[ok] Running d6_filter_format
[error]  Migration d6_filter_format did not meet the requirements. The module filter is not enabled in the source site. source_module: filter. 
 

It would be a lot more useful for the requirements check to check the database first and see something like:

[error]  SQLSTATE[HY000] [1045] Access denied for user 'migration_user'@'localhost' (using password: YES) 
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Status: Active » Needs review
FileSize
892 bytes

Good news, SqlBase already does this check, we just run it in the wrong place to ever catch it.

quietone’s picture

I thought a test might be useful and tried to make one.

The last submitted patch, 3: fail-test.patch, failed testing. View results

neclimdul’s picture

That's awesome! I was kinda dreading what a test might look like but that's not bad.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -63,6 +63,28 @@ public function testSourceProviderNotActive() {
+    try {
+      $plugin->checkRequirements();
+    }
+    catch (RequirementsException $e) {
+      // Re-throw so PHPUnit can assert the exception.
+      throw $e;
+    }

Do we need to catch a re-throw? seems like if we just let it go it'll work as expected already.

heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -63,6 +63,28 @@ public function testSourceProviderNotActive() {
+    try {
+      $plugin->checkRequirements();
+    }
+    catch (RequirementsException $e) {
+      // Re-throw so PHPUnit can assert the exception.
+      throw $e;
+    }

Yeah, I think removing the try catch would be better.

dhirendra.mishra’s picture

Assigned: Unassigned » dhirendra.mishra

workinmg on it.

dhirendra.mishra’s picture

Assigned: dhirendra.mishra » Unassigned
Status: Needs work » Needs review
FileSize
2.14 KB

Removed the try catch as suggested in previous comment. Please review my patch.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Assuming this comes back green, onward!

neclimdul’s picture

This might be the fastest to RTBC code related issue I've ever been on. Thanks! Awesome work and +1 RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 5123f7568d to 8.7.x and b6f8992600 to 8.6.x. Thanks!

  • alexpott committed 5123f75 on 8.7.x
    Issue #3008720 by quietone, neclimdul, dhirendra.mishra, heddn:...

  • alexpott committed b6f8992 on 8.6.x
    Issue #3008720 by quietone, neclimdul, dhirendra.mishra, heddn:...

Status: Fixed » Closed (fixed)

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