Problem/Motivation

From #2281691: User interface for migration-based upgrades. Much of the exception handling in the Migrate UI is problematic, including:

  • Throwing or catching \Exception instead of something more specific.
  • Very long try/catch blocks for unclear purpose.
  • Catching exceptions without reporting what they were or why.
  • Possibly relying on exceptions for normal/expected validation code flow?

The following points are from reviews on #2281691: User interface for migration-based upgrades:

  1. +++ b/core/modules/drupal_upgrade/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1196 @@
    +    try {
    +
    ...
    +    catch (\Exception $e) {
    

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

  2. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,205 @@
    +    catch (\Exception $e) {
    ...
    +      throw new \Exception('Source database does not contain a recognizable Drupal version.');
    

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

  3. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +    catch (\Exception $e) {
    +      // The table might not exist for example in tests.
    +    }
    

    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.

  4. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +      // Migrations which are not applicable given the source and destination
    +      // site configurations (e.g., what modules are enabled) will be silently
    +      // ignored.
    +      catch (RequirementsException $e) {
    +      }
    +      catch (PluginNotFoundException $e) {
    +      }
    

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

  5. +++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
    @@ -0,0 +1,222 @@
    +      catch (\PDOException $e) {
    +        $version_string = FALSE;
    +      }
    

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

  6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
    @@ -0,0 +1,1227 @@
    +    catch (\Exception $e) {
    +      $error_message = [
    +        '#type' => 'inline_template',
    +        '#template' => '{% trans %}Resolve the issue below to continue the upgrade.{% endtrans%}{{ errors }}',
    +        '#context' => [
    +          'errors' => [
    +            '#theme' => 'item_list',
    +            '#items' => [$e->getMessage()],
    +          ],
    +        ],
    +      ];
    

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

Proposed resolution

Evaluate the types of exceptions used, the reasons they are being caught, the use of try/catch for code flow, and the potential loss/hiding of debug information.

Remaining tasks

TBD

User interface changes

N/A

API changes

TBD

Data model changes

TBD

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes
mikeryan’s picture

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.

abhishek-anand’s picture

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Status: Active » Needs review
FileSize
1.27 KB

This patch only addresses item #5 in the IS.

maxocub’s picture

Assigned: Unassigned » maxocub

Assigning for review.

maxocub’s picture

Assigned: maxocub » Unassigned
Status: Needs review » Needs work

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

Issue tags: +Migrate UI
heddn’s picture

Status: Needs work » Needs review
FileSize
3.41 KB
4.61 KB

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.

Status: Needs review » Needs work

The last submitted patch, 12: 2679835-12.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Assigned: Unassigned » quietone

Assigning to myself.

quietone’s picture

Assigned: quietone » Unassigned

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Issue summary: View changes
Status: Needs work » Postponed
quietone’s picture

Issue summary: View changes
Status: Postponed » Needs work
Issue tags: +Needs reroll

No longer postponed, and will need a reroll.

quietone’s picture

Status: Needs work » Postponed

It was reverted, back to postponed.