Improve exception handling in Migrate UI

This is a MASSIVE try block. Which calls are we catching exceptions for? Why can't they be individually wrapped with classed exceptions?

It originally wrapped a single call to $this->createMigrations(). At the moment, I think only the getMigrationTemplates() call at the beginning of the try needs to be in here, and that's throwing \Exception (see next point).

Is there a classed exception to catch or throw instead? Using \Exception is not as ideal

There isn't any existing exception that's relevant here - we should create a DrupalVersionException or somesuch.

I think @tim.plunkett mentioned this too, but this is way too broad a catch. We should have a followup issue to fix this because it is just swallowing any error. If there is a specific exception that is happening then we should handle it more directly or look at underlying causes.

@abhishek-anand? Did you have a particular testing problem inspiring that catch?

This seems a fragile way to handle this; is this related to the existing followup about detecting applicable migrations?

I.e., catching PluginNotFoundException and RequirementsException - yes, this is for skipping inapplicable migrations. There are two scenarios - where a migration references a source or destination plugin which is defined by a module which is not enabled (thus, we do not want to perform migration for that module), we get PluginNotFoundException. And if the plugin's module is enabled but there's a missing requirement (e.g., the corresponding module is not enabled in the source database), checkRequirements() throws RequirementsException.

We are doing this catch if the system table exists, but not if the key_value table exists.

Yes, the key_value branch should also be prepared for PDO exceptions.

Also (again), \Exception is way generic, and using a try/catch like this to handle expected validation code flow is problematic.

See the first two points above.

Hi Mike,

Did you have a particular testing problem inspiring that catch?

No, I had to get the system data, so I reused to code from \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::getSystemData

Didn't pay enough attention to the catch, just replicated it.

This patch only addresses item #5 in the IS.

Assigning for review.

I think the item #5 from the issue summary is fixed with this patch, but we need to keep thinking about the other points and @mikeryan's suggestions in #3. Back to needs work.

I think this addresses all the points in the IS. Let's see how tests fair. I did all the work, then reviewed back to see that there was already a patch in #7 that does about the same thing. So, there's also an interdiff also against @quietone's work.

Assigning to myself.

Postponed on #2918761: Break up MigrateUpgradeForm into smaller forms

No longer postponed, and will need a reroll.

It was reverted, back to postponed.

#2918761: Break up MigrateUpgradeForm into smaller forms has been committed.

The try catch blocks in the IS have moved to new files. As a first step to identify what needs to be done, add the new filenames to the IS.

Let's try a reroll.

Didn't think that was going to work. How about this.

And again.

Add a catch for BadPluginDefinitionException to CredentialForm and a wider catch \Exception in the IncrementalForm.

I think I uploaded a wrong patch.

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -199,29 +202,50 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    catch (\Exception $e) {
   

   Do we still need to catch /Exception here or can we catch something else?

2. +++ b/core/modules/migrate_drupal_ui/src/Form/IncrementalForm.php
   @@ -105,7 +105,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      catch (\Exception $exception) {
            // Hide DB exceptions and forward to the DB credentials form. In that
            // form we can more properly display errors and accept new credentials.
   

   We need to update the comment here then. If we are catching \Exception, we are hiding all errors, not just from the database.

1. Changed to DatabaseException
2. Now that the next step for the Incremental form is the Credential form, there isn't anything to validate here. The validate function can be removed.

There are no more large try/catch blocks. There is no more generic catching of \Exception. The only thing I could find was a few unused Use statements, but that can be fixed on commit. IS seems up to date. LGTM.

BTW, I did some work on this way back in #10, but things have changed dramatically since then and I don't think any of my changes from that patch are actually even in the final version. So I think that still means I can RTBC this thing.

CredentialForm:

use Drupal\Core\Database\DatabaseNotFoundException;
use Drupal\Core\Database\DatabaseAccessDeniedException;

Removing the two unused statements.

Testbot had a hiccup re-rtbcing

1. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -199,29 +202,49 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    $error_message = [
   +      '#title' => $this->t('Resolve the issue below to continue the upgrade.'),
   +      '#theme' => 'item_list',
   +    ];
   

   We never have more than one item in the list as far as I can see so I think we don't need the item list anymore. Which means we can remove all the rendering.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -199,29 +202,49 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    catch (BadPluginDefinitionException $e) {
   

   PHPStorm reckons this is never thrown by setupMigrations. Which is interesting.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -199,29 +202,49 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +    catch (RequirementsException $e) {
   

   How can this be thrown? It's explicitly caught in \Drupal\migrate_drupal\MigrationConfigurationTrait::getMigrations()?

I think this is just a normal task. There's no bug that we're fixing explicitly here.

Postponing on #2562845: Validate file path on Credential form which is also changing the error messages.

#2562845: Validate file path on Credential form is now in so no longer postponed.

I'll take a look at the reroll.

Re-rolled.

Fix test failures.

Resolve the latest test failures.

Hmm, I'm not sure I've grasped this fully. @quietone would you mind taking over?

Not at all.

@Jo Fitzgerald, I don't know why but the migrations aren't getting the database connection.

Let's try this.

Now that it has passed tests, time for some cleanup and I wasn't working from head when making the previous patch. This removes the renderer service from the Credential form and adds another if block in the CredentialFrom:validateForm() for readability. At least I find it easier to skim through that code now.

Haven't run the test locally so here goes.

All ready for a review.

Assigning to myself for review this week.

This is a definite improvement. Awesome work!

1. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -56,7 +57,7 @@ protected function getSystemData(Connection $connection) {
   +    catch (\PDOException $e) {
   

   Is this the exception we should catch? I know DatabaseExceptionWrapper is a thing, and it always wraps a \PDOException, so it might help to catch that instead (or its interface, DatabaseException)? Not sure what the best course of action is here.

2. +++ b/core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
   @@ -183,10 +184,18 @@ protected function getLegacyDrupalVersion(Connection $connection) {
   +      catch (\PDOException $e) {
   +        $version_string = FALSE;
   +      }
   

   Same here.

3. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -220,31 +211,48 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +        $this->errors[$database['driver'] . '][database'] = $e->getMessage();
   

   The key here is very hard to parse at a glance. Can we do this in two lines, and re-use $error_key elsewhere in the method:

   $error_key = $database['driver'] . '[database';
   $this->errors[$error_key] = $e->getMessage();
   

4. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -220,31 +211,48 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      // Get the Drupal version of the source database.
   

   Let's move this comment above the if (!$this->errors) and expand it to "Validate the Drupal version of the source database."

5. +++ b/core/modules/migrate_drupal_ui/src/Form/CredentialForm.php
   @@ -220,31 +211,48 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      // Setup migrations and save form data to private store.
   

   Let's move this above the if (!$this->errors).

Unassigning.

@phenaproxima, Thanks for the quick review.

1 and 2. I wasn't sure either but the DatabaseExceptionWrapper doc states "This wrapper class serves only to provide additional debug information."" and that seems like a good thing. Therefor these exceptions are changed to the DatabaseExceptionWrapper.
3,4,5. Fixed.

I like it! RTBC once green on all backends.

Committed 543a559 and pushed to 8.7.x. Thanks!
Fixed this on commit:

FILE: .../core/modules/migrate_drupal/src/MigrationConfigurationTrait.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 197 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: 'system'
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------