Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center. $get_migration = $this->prophesize(MigrationLookup::class);
...
$process_plugin_manager->createInstance('get', ['source' => ['string_id', 'integer_id']], $migration_plugin->reveal())
->willReturn($get_migration->reveal());
We're calling createInstance for the 'get' plugin. So the return should be an instance of Drupal\migrate\Plugin\migrate\process\Get, not Drupal\migrate\Plugin\migrate\process\MigrationLookup.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2971338-4.patch | 1.26 KB | jofitz |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedLooks to me like it is creating a get plugin to reveal a migration_lookup plugin. How about changing the name from get_migration to lookup_migration.
Comment #3
joachim CreditAttribution: joachim as a volunteer commentedThe first param to createInstance() is the plugin ID, so this is a 'get' plugin that's being mocked.
Comment #4
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrect the plugin class.
This is unrelated to the patch in #2, hence no interdiff.
Comment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedThis test failure is caused by an 'error' in the annotation of the Get plugin.
I've created a ticket to handle that (because it is out of the scope of this issue) and will postpone this until that issue is resolved.
Comment #7
jofitz CreditAttribution: jofitz at ComputerMinds commented#2973366: GetTest.php should test Get plugin, rather than a class the extends it is now in so the patch in #4 should no longer fail. Let's see...
Comment #8
maxocub CreditAttribution: maxocub commentedThis looks good to me.
Comment #9
alexpottCrediting @joachim for finding and creating this issue.
Comment #10
alexpottCommitted and pushed e9b5e54b27 to 8.6.x and dd44e76f51 to 8.5.x. Thanks!
Comment #15
xjmThis broke 8.5.x HEAD. The issue is filed against 8.6.x so maybe it was backported by mistake. Otherwise, as with any issue, please set the branch to the earliest branch that includes the commit.
Reverted from both branches for further investigation.
Comment #16
alexpottThe error this caused on 8.5.x is
It's not always practical to set to the earliest branch as that often means that patches get to rtbc but then can't be applied to the latest branch which then results in re-work for contributors which can be frustrating after getting a patch to rtbc. That said, the HEAD fail is my mistake because I should have triggered a test run on 8.5.x before committing to 8.5.x.
Looking at the error on 8.5.x I think it worth understanding why this is happening before committing this patch to 8.6.x. Since on first sight it seems surprising this is occurs.
Comment #17
jofitz CreditAttribution: jofitz at ComputerMinds commentedThis broke 8.5.x HEAD because the patch on which this depends, #2973366: GetTest.php should test Get plugin, rather than a class the extends it, could not be applied to to 8.5.x.
My understanding of the underlying issue is that there is a fault when parsing the line
@, that character must be escaped with @@.because it happens to begin with the "@" character so something like @code or @see is expected.
I couldn't find a solution to the problem, so my workaround was to surround the @ with quotes.
As I stated in #2973366: GetTest.php should test Get plugin, rather than a class the extends it comment #2:
I'm happy to reroll the patch in #2973366: GetTest.php should test Get plugin, rather than a class the extends it for 8.5.x if a maintainer would reopen the ticket, should that be the chosen course of action.
I'm happy to discuss this further here or on Slack, if I can be of further assistance.
Comment #18
alexpott@Jo Fitzgerald thanks for the sleuthing. I think given that this is test-only and the related changes are also test-only doing this in 8.6.x is fine. I was just concerned that the fail pointed to some real issues. It doesn't so all is good here - we were just unlucky the previous issue didn't apply to 8.6.x.
Going to mark rtbc and will commit again to 8.6.x only. Thanks!
Comment #19
alexpottCommitted 6bf15d5 and pushed to 8.6.x. Thanks!