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.
Comment | File | Size | Author |
---|---|---|---|
#21 | 3254986-21.patch | 4.55 KB | Matroskeen |
|
Issue fork drupal-3254986
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:
Comments
Comment #2
MatroskeenComment #4
MatroskeenI 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.
Comment #5
danflanagan8Nice find, @matroskeen. Your approach looks totally reasonable to me. The failing test is asserting the wrong message now, but that's an easy fix.
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.
Comment #7
paulocsjust fixed the tests :)
Comment #8
Matroskeen@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.
Comment #9
danflanagan8Good idea, @matroskeen. And I'll set the status to NR, seeing as the tests have been updated and pass. (Thanks, @paulocs!)
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #11
quietone CreditAttribution: quietone at PreviousNext commentedSmall update to IS and reviewed the MR.
Comment #12
MatroskeenI'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?
Comment #13
MatroskeenComment #14
MatroskeenI opened a similar issue in Drush: https://github.com/drush-ops/drush/issues/5051
Comment #15
MatroskeenI'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.
Comment #16
MatroskeenIf 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=Comment #17
benjifisherWe 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 usedsource_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.
Comment #18
mikelutzDo you think this is done then, @benjifisher?
Comment #20
mikelutzI think this is fine, but it needs to be rebased and reset for 9.5
Comment #21
MatroskeenThe MR is rebased and targeted to 9.5.x. I'm also uploading a patch to queue a test for 10.0.x.
Comment #22
benjifisherI 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 anddrush
:Of course, the changes to the kernel tests make it clear how the message is updated.
Comment #24
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!
Comment #26
catchReverted the 9.5 commit - breaks aggregator tests (which aren't in 10.x).