Problem/Motivation

It would be great if messages would be more helpful:

Migration d6_node did not meet the requirements

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#13 interdiff.txt3.49 KBbenjy
#13 2321609-13.patch18.77 KBbenjy
#9 interdiff.txt4.57 KBbenjy
#9 2321609-9.patch17.13 KBbenjy
#7 interdiff.txt1.32 KBbenjy
#7 2321609-6.patch15.08 KBbenjy
#5 migrate-2321609-5.patch13.76 KBdawehner

Comments

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented

Can you Please elaborate the same ?

rteijeiro’s picture

rteijeiro commented
Title: Provide a helpful message in case requirments are not met » Provide a helpful message in case requirements are not met
hussainweb’s picture

hussainweb
Primary language English
commented
Related issues: +#2282051: Handle skipped migrations as non-failures

Adding related issue in Migrate Upgrade module.

benjy’s picture

benjy commented

How about we refactor checkRequirements() to throw exceptions instead of returning false?

<?php
  public function checkRequirements() {
    // Check whether the current migration source and destination plugin
    // requirements are met or not.
    try {
      if ($this->getSourcePlugin() instanceof RequirementsInterface && !$this->getSourcePlugin()->checkRequirements()) {
        return FALSE;
      }
      if ($this->getDestinationPlugin() instanceof RequirementsInterface && !$this->getDestinationPlugin()->checkRequirements()) {
        return FALSE;
      }

      /** @var \Drupal\migrate\Entity\MigrationInterface[] $required_migrations */
      $required_migrations = \Drupal::entityManager()->getStorage('migration')->loadMultiple($this->requirements);
      // Check if the dependencies are in good shape.
      foreach ($required_migrations as $required_migration) {
        if (!$required_migration->isComplete()) {
          return FALSE;
        }
      }
    }
    catch (\Exception $e) {
      return FALSE;
    }

    return TRUE;
  }
?>
dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new migrate-2321609-5.patch13.76 KB

I thought about introducing an RequirementsResult value object, but indeed an exception works fine here, because of the semantic meaning of it.

Status: Needs review » Needs work

The last submitted patch, 5: migrate-2321609-5.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2321609-6.patch15.08 KB
new interdiff.txt1.32 KB

Added a try/catch to the executable to catch the exceptions.

Status: Needs review » Needs work

The last submitted patch, 7: 2321609-6.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2321609-9.patch17.13 KB
new interdiff.txt4.57 KB

Couple more fixes. I added a try/catch to LoadEntity, previously we were just silently ignoring the the migration if the source plugin didn't meet the requirements. Maybe we should log that error somewhere?

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -233,11 +234,18 @@ public function getSource() {
+        $this->t('Migration @id did not meet the requirements. @message', array(
+          '@id' => $this->migration->id(),
+          '@message' => $e->getMessage(),
+        )), 'error');
       return MigrationInterface::RESULT_FAILED;

Did we considered to pass along the missing requirements, maybe someone wants to use it? In that case we probably need a new getter method on the exception (forgot that).

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/load/LoadEntity.php
@@ -79,11 +80,15 @@ public function loadMultiple(EntityStorageInterface $storage, array $sub_ids = N
+      catch (RequirementsException $e) {
+
+      }

Do we really want to do nothing? Just curious.

benjy’s picture

benjy commented

Yeah, passing along the requirements sounds like a good idea.

I asked the question in my previous comment about whether we should maybe log it somewhere? Previously, we were just ignore migrations that failed the requirements check.

Status: Needs review » Needs work

The last submitted patch, 9: 2321609-9.patch, failed testing.

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2321609-13.patch18.77 KB
new interdiff.txt3.49 KB

Fixed the remaining test and added getRequirements() and getRequirementsString() to RequirementsException. I've put the requirements into the message in MigrateExecutable but it doesn't seem to be any additional information other than what the message already gives us at this point.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -239,9 +239,10 @@ public function import() {
-        $this->t('Migration @id did not meet the requirements. @message', array(
+        $this->t('Migration @id did not meet the requirements. @message @requirements', array(

Given that the requirements for example are in english anyway did you thought of dropping the translatablity here? For sure this would be out of scope, just asking.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed ffde907 and pushed to 8.0.x. Thanks!

  • alexpott committed ffde907 on 8.0.x 
    Issue #2321609 by benjy, dawehner: Fixed Provide a helpful message in...

Status: Fixed » Closed (fixed)

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