Problem/Motivation
Drupal migration source plugins should use SourcePluginBase::getSourceModule()
to check their requirements instead of simply checking their plugin definition. (This method was unused until #2936365: Migrate UI - allow modules to declare the state of their migrations aka https://git.drupalcode.org/project/drupal/-/commit/c1734c61852faecf3f358... was commited).
Current checkRequirements only checks the plugin definition for the source module whereas getSourceModule will check configuration first, for an overridden source module, and then the plugin definition.
Proposed resolution
- Use getSourceModule in DrupalSqlBase
- Modify tests that need the source module enabled in the database so that requirements are met.
- Remove two lines from
core/modules/field/migrations/d6_field.yml
, which duplicate two lines incore/modules/field/migrations/state/field.migrate_drupal.yml
, since they break several of the improved tests.
Remaining tasks
Identify and address BC issues.
User interface changes
Nothing.
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#16 | 3176393-16.patch | 4.26 KB | quietone |
| |||
#16 | interdiff-10-16.txt | 1.18 KB | quietone |
#10 | 3176393-10.patch | 4.11 KB | quietone |
#10 | interdiff-9-10.txt | 781 bytes | quietone |
#9 | 3176393-9.patch | 3.75 KB | quietone |
Comments
Comment #2
huzookaBefore adding tests or doing anything more, let's see what this patch breaks.
Comment #4
quietone CreditAttribution: quietone as a volunteer commentedJust some thoughts.
I think that since this is specific to a Drupal database it should be in DrupslSqlBase. I know that this interface also has getSourceModule which is also specific to Drupal database, it shouldn't be here either. I'd rather not compound the error.
It is nice to have the comparison done on integers.
Comment #6
Wim LeersI didn't even know the migration system supported this! Very good to know 🤓
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedReroll and remove the source_module declaration from d6_field.yml and let's see what breaks.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedAh, those tests are nice proof that this is working as intended.
Do we need the tags since this in the migration system component?
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedMeant to change the name of the dblog test method. It looked like an old copy/paste error.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedComment #12
MatroskeenI wanted to review this issue, but I noticed that last patch doesn't match the issue summary.
I don't see these changes in the patch. If the proposed resolution was changed, we should probably reflect this in the issue summary. Or it still needs more work. In any case, I'm moving this to NR.
Comment #16
quietone CreditAttribution: quietone at PreviousNext commentedIS updated and new patch with just comment changes.
Comment #17
Wim LeersComment #19
Wim Leers#3283795: ComposerHooksTest is broken on latest DrupalCI PHP container fixed that (unrelated) fail on D10.
Comment #20
alexpottWhy are we removing this? I can't see how this change makes it not necessary more. I checked the issue comments and I couldn't see an explanation. It was added in #2936365-203: Migrate UI - allow modules to declare the state of their migrations maybe the comment #203 on that issue will shed some light as to whether this will break something.
Comment #21
quietone CreditAttribution: quietone at PreviousNext commentedI looked back at the issue that added the 'source_module: phone' and I am convinced it was an error. The correct entry is in core/modules/field/migrations/state/field.migrate_drupal.ymlcheckRequirementscheckRequirements.
Comment #22
benjifisherRegarding #20, #21:
The change is mentioned in #7. You will not see it in the interdiff attached to that comment, maybe because that patch was also a reroll.
Looking at the final patch from #2936365-309: Migrate UI - allow modules to declare the state of their migrations, I see this duplication:
I will mention the disputed change in the issue summary.
I also cannot think of any reason why the
field
module’s plugin, which includes handling for several other modules (nodereference
,number_integer
,email
, …) in theprocess
section, should declare one of them as its source module.I was not sure whether this change was related to this issue or an unrelated fix, like the fix mentioned in #10. I tried restoring the two lines to
d6_field.yml
, and several of the kernel tests in thefield
module failed. This change is in scope.In short, I do not see any reason to disagree with #21. Of course, @quietone is far more familiar with both issues than I am.
Back to RTBC.
Comment #23
alexpottCommitted and pushed 12df2086f0 to 10.1.x and 397b8d5959 to 10.0.x and 8ca4362795 to 9.5.x. Thanks!
Backported this to 9.5.x since this task is low risk and improves the migration system.