Problem/Motivation

Migrate source plugins use the checkRequirements() method and throw RequirementsException to notify migration runners that migration requirements are not met.

However, if I run drush ms for a migration that doesn't meet the requirements, it won't show up in the list and I won't see any additional messages. If I add -vvv flag, it's getting better:
[debug] Migration 'foo' is skipped as its source plugin has missed requirements: . [0.13 sec, 24.65 MB]

As we can see, it says the requirements are missing. By why? 🤷‍♂️

I looked at Drush code. They do this:

foreach ($migrations as $migrationId => $migration) {
    try {
        $sourcePlugin = $migration->getSourcePlugin();
        if ($sourcePlugin instanceof RequirementsInterface) {
            $sourcePlugin->checkRequirements();
        }
    } catch (RequirementsException $exception) {
        $this->logger()->debug("Migration '{$migrationId}' is skipped as its source plugin has missed requirements: " . $exception->getRequirementsString());
        unset($migrations[$migrationId]);
    }
}

To get the details, they use only $exception->getRequirementsString(). But Drupal\migrate\MigrateExecutable::import(), uses $exception->getMessage() as well, and this gives a more helpful message:

$this->message->display(
        $this->t(
          'Migration @id did not meet the requirements. @message @requirements',
          [
            '@id' => $this->migration->id(),
            '@message' => $e->getMessage(),
            '@requirements' => $e->getRequirementsString(),
          ]
        ),
        'error'
      );

It should be fixed in Drush. Here is the issue: https://github.com/drush-ops/drush/pull/5052

Looks good so far, but do we really need to print the @requirements array in that case? Here is current message:
Migration foo did not meet the requirements. The module bar is not enabled in the source site. source_module: bar

Why would we need to print source_module: bar if there is a message that explains what is missing?
I think this should be enough: Migration @id did not meet the requirements. @message.

It was introduced in #2321609: Provide a helpful message in case requirements are not met with the following comment:

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.

The author confirmed this list does not provide any additional data. Let's remove it then :)

Proposed resolution

1) Change the requirements message pattern from:
Migration foo did not meet the requirements. The module bar is not enabled in the source site. source_module: bar
To:
Migration foo did not meet the requirements. The module bar is not enabled in the source site.
2) Apply the same format in Drush: https://github.com/drush-ops/drush/pull/5052

Remaining tasks

Review / commit.

CommentFileSizeAuthor
#21 3254986-21.patch4.55 KBMatroskeen

Issue fork drupal-3254986

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Matroskeen created an issue. See original summary.

Matroskeen’s picture

Issue summary: View changes

Matroskeen’s picture

Status: Active » Needs review

I don't see why the requirements array keys should be in snake case format, we can probably change that. In the meantime, moving to review to validate the approach and bring someone's attention.

danflanagan8’s picture

Status: Needs review » Needs work

Nice find, @matroskeen. Your approach looks totally reasonable to me. The failing test is asserting the wrong message now, but that's an easy fix.

I don't see why the requirements array keys should be in snake case format,

The weird thing to me is that there's not a defined list of allowed $requirement_type values. It all seems pretty willy nilly.

I'll set to NW since the failing test needs to expect a new message. But I also suspect that @matroskeen is looking for review of the approach from a maintainer.

paulocs made their first commit to this issue’s fork.

paulocs’s picture

just fixed the tests :)

Matroskeen’s picture

@danflanagan8 exactly, I think we need input (and hopefully an agreement) about the expected format.
It would be great to involve migrate maintainers, so I'll add this to the next meeting agenda #3255041: [meeting] Migrate Meeting 2021-12-23 1400Z.

danflanagan8’s picture

Status: Needs work » Needs review

Good idea, @matroskeen. And I'll set the status to NR, seeing as the tests have been updated and pass. (Thanks, @paulocs!)

quietone’s picture

I have a cursory look and this makes sense to me. One thing that I'd like to see added is better documentation about the missing requirements array in RequirementsException. It should explain how it is used, with an example.

   * @param array $requirements
   *   (optional) The missing requirements.
quietone’s picture

Issue summary: View changes
Status: Needs review » Needs work

Small update to IS and reviewed the MR.

Matroskeen’s picture

Title: Some RequirementsException messages are not visible in Drush logs » Remove requirements array from RequirementsException message
Issue summary: View changes
Issue tags: +LutskGCW22

I'm updating the issue summary and title after some investigation and looking at #2321609: Provide a helpful message in case requirements are not met.
Also, creating another issue to deal with the requirements keys: #3260964: Add consistent requirements to RequirementsException .

Not sure where to discuss/proceed with a list of predefined constants - should it be moved into a third task?

Matroskeen’s picture

Issue summary: View changes
Matroskeen’s picture

I opened a similar issue in Drush: https://github.com/drush-ops/drush/issues/5051

Matroskeen’s picture

Issue summary: View changes
Priority: Normal » Minor
Status: Needs work » Needs review

I'm updating the merge request to reflect my proposed resolution.

While looking at #2321609: Provide a helpful message in case requirements are not met, I discovered something else, which is described here: #3261419: [PP-1] Missing requirements message should not be translatable.

Matroskeen’s picture

If we agree on the proposed resolution, we'll have to suggest the same change in migrate_manifest module: http://grep.xnddx.ru/search?text=getRequirementsString&filename=

benjifisher’s picture

Issue summary: View changes

We discussed this issue at #3263731: [meeting] Migrate Meeting 2022-02-17 1400Z. That issue will have a transcript of the meeting.

I agree that we should remove the redundant part of the message.

Looking at the changes to the tests, I noticed that some used source_module and others used source_module_additional. This difference is irrelevant: see #3260964: Add consistent requirements to RequirementsException , which already links to this issue.

I made a few clarifications in the issue summary, no substantive changes.

mikelutz’s picture

Issue tags: +Portland2022

Do you think this is done then, @benjifisher?

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mikelutz’s picture

Status: Needs review » Needs work

I think this is fine, but it needs to be rebased and reset for 9.5

Matroskeen’s picture

Version: 9.5.x-dev » 10.0.x-dev
Status: Needs work » Needs review
FileSize
4.55 KB

The MR is rebased and targeted to 9.5.x. I'm also uploading a patch to queue a test for 10.0.x.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

I did not update the status in #17 because I did not have a chance to review and test at that time. I have done so now.

The patch in #21 looks good to me, and it is the same as the diff shown on the MR. Let's stick with the patch.

I have two test migrations lying around from other issues. I made one of them depend on the other, installed a fresh copy of Drupal 10.1.x with the patch from #21, and installed the migrate module and drush:

$ drush mim test_user_role
 [error]  Migration test_user_role did not meet the requirements. Missing migrations test_download. 

Of course, the changes to the kernel tests make it clear how the message is updated.

  • catch committed ac56e5a on 10.0.x
    Issue #3254986 by Matroskeen, paulocs, mikelutz, benjifisher,...
  • catch committed 4c0f507 on 10.1.x
    Issue #3254986 by Matroskeen, paulocs, mikelutz, benjifisher,...
  • catch committed 0f10593 on 9.5.x
    Issue #3254986 by Matroskeen, paulocs, mikelutz, benjifisher,...
catch’s picture

Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

  • catch committed aec9ebc on 9.5.x
    Revert "Issue #3254986 by Matroskeen, paulocs, mikelutz, benjifisher,...
catch’s picture

Version: 9.5.x-dev » 10.0.x-dev

Reverted the 9.5 commit - breaks aggregator tests (which aren't in 10.x).

Status: Fixed » Closed (fixed)

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