Some migrations may not define a source. This can happen in the case of derived migrations or those that inherit configuration from a migration group.
In these instances, NoSourcePluginDecorator::getDefinitions() calls $source_plugin_manager->hasDefinition($definition['source']['plugin']);, which first throws a notice because source is not defined in the config migration at all. Then the definition gets filtered out because there is no source plugin with an empty id.
Make NoSourcePluginDecorator::getDefinitions() account for migrations that do not define a source.
Review
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !11362
Comments
Comment #2
mikeryanComment #3
mikeryanActually, that inappropriately filters *in* the node derivers that were the cause of so much pain to begin with... This version will allow a migration which has a deriver and no source.
Comment #5
mikeryanComment #6
chx commentedI think we should have an issue which filters out migrations with nonexistent derivers.
Aside from that, would the following work, I wonder:
it says nothing about derivers and doesn't need to.
Comment #7
chx commentedComment #8
phenaproximaComment #9
phenaproximaYeah...this needs tests.
Comment #16
mkalkbrennerI see tons of these warnings in our logs when we switch from PHP 7.4 to 8.0 to run our existing migrations. It happens for all our source plugins that are derived from SqlBase. It seems that the notice became a warning in PHP 8.0:
Applying the patch from #7 leads to fatal errors as it changes the current behavior. A patch that just prevents the warnings and doesn't change anything is attached.
Comment #18
mikelutzNot sure if this is the right approach here, I don't completely understand the issue, but definitely needs tests at minimum.
Comment #20
id.conky commentedOn my project we have migration where source plugin key not defined directly in migration config, but inherited from migration group shared_configuration.
So I faced with that error:
I updated patch to check if we actually have defined plugin and then call $source_plugin_manager->hasDefinition().
Comment #25
dcam commentedI converted #20 into an MR and added a unit test.
Comment #26
dcam commentedRemoving patches
Comment #27
smustgrave commentedI typically avoid migration reviews as not my best topic but think I can cover this one.
The solution seems pretty straight forward. Looking at the test coverage https://git.drupalcode.org/issue/drupal-2797421/-/jobs/4553477 and seems to be there, and even better a unit test.
Don't see anything off so will go ahead and mark it.
Thanks
Comment #28
ghost of drupal pastCan we do without the test as per #2972776: [policy, no patch] Better scoping for bug fix test coverage ? Is this something that needs a test?
Comment #29
smustgrave commentedWould leave for now and see what committers think.
Comment #30
dcam commentedAt least it's a unit test so the overhead is low.
Comment #31
mikelutzThe test seems fine. We had no test coverage around NoSourcePluginDecorator before, so this is a good opportunity to add some.
I would say this exposes a lack of test coverage, but since the class essentially exists for the one line we are changing here, it's best to add the coverage here. I would say the majority of the seven questions are no's in the new guidelines, which means the tests are okay to add
Is the fix is easy to verify by manual testing?
no
Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc.
no
Is the fix achieved without adding new, untested, code paths?
no
Is an explicit 'regression' test needed?
no
Is it easy for someone who did not work on the original bug report to add the test coverage in a followup issue?
yes
Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue?
yes/no
If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed?
yes
Comment #32
alexpottCommitted and pushed c42f5d9328c to 11.x and 8f3ccf4c4b4 to 11.1.x and f26adfd7445 to 10.5.x and a09b4608957 to 10.4.x. Thanks!