Problem/Motivation
As a migration related module developer, I don't want errors to be logged and exceptions been thrown when a Drupal 6|7 variable that is required for a migration is missing from the source: #3151979: Make allow_insecure_uploads a required variable in system file migrations; #3151980: System mail settings migration (d7_system_mail) assumes that the mail_system variable is always available; #3151993: Search settings migration (d7_search_settings) assumes that the search_default_module variable is always set.
Right now, these migrations aren't handle required variables. If the variable is missing, they just simply throw a MigrateSkipRowException, and an error message is logged.
Proposed resolution
Add an optional variables_required
configuration option to the variable
migrate source plugin. If one of the required variables is missing from the source, the source plugin should tell that there isn't anything to migrate (so return zero rows).
This way the logged (imho misleading) errors can be avoided.
BC
The change won't affect any preexisting migrations. Every migration definition that uses the variable
source plugin only with the variables
configuration will behave the same way as before, so they will return a single row even when all of the defined Drupal6|7 variables are missing.
Remaining tasks
- Patch + test
- Subsystem maintainer feedback and review if this feature request is acceptable
- Change record
User interface changes
Nothing.
API changes
A new, optional variables_required
key on the Variable migrate source plugin.
Data model changes
Nothing.
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff.3152789.14-19.txt | 1.1 KB | mikelutz |
#19 | 3152789-19.drupal.patch | 8.75 KB | mikelutz |
Comments
Comment #2
huzookaComment #3
Wim LeersDevil's advocate: does this have backwards compatibility implications? If so, how are we ensuring that existing migration projects are not affected?
Can you include an explanation/justification for the BC aspects in the issue summary? Core committers will want to see that answered too before they commit this 🤓
Comment #4
huzookaIS updated to address#3.
Comment #5
huzookaComment #6
DamienMcKennaFWIW this solved my problem when a D8 site's dependencies added the Language module, which wasn't installed on the D7 site it was migrating from. Thank you huzooka!
Comment #7
DamienMcKennaJust to mention it, to problem with the Language module's dependencies expanded because the LanguageTypes plugin required an array argument, so I needed to modify d7_language_types.yml to use "variables_required" instead of just "variables", see #3155300 for details.
Comment #8
quietone CreditAttribution: quietone commentedThe tags migrate-d*-d* were created for identifying issue migrating specific data, like block or taxonomy, from d6 or d7 to the latest Drupal and so doesn't really apply here. What is being changed here is a source plugin for all earlier Drupal sources. Any objection to removing them?
Comment #9
mikelutzI confess, I thought I was going to have an objection to this, but it seems to be really well done, is backwards compatible, very well tested, well documented, and with #3155300: D7->D8 Language module migration runs when not appropriate We have a use for it in core.
Comment #11
mikelutzThe test failure is not due to this issue, but as I was going to re-rtbc, I realized there were some coding style issues.
There are several places that do not have the proper space after =>
Comment #12
huzookaThe test runned does not reports them I guess becasue these are pre-existing violations?..
Comment #13
huzookaWell, I was wrong. ^^ Those would be new violations.
Comment #14
ankithashettyFixed the coding style issues mentioned in #11. Kindly review.
Thank you!
Comment #15
huzooka@ankithashetty please be more patient and don't work on issues when it is assigned to someone else.
Comment #16
ankithashettyHi @huzooka, very sorry for the trouble... I really didn't notice that the issue was already assigned! Will be more careful henceforth...
Comment #17
mikelutzBack to RTBC though.
Comment #18
catchSeconding #3152789-9: Add required variables config to Variable migrate source plugin, and if one of those are missing, return zero rows, the issue title was not encouraging, but the patch is surprisingly straightforward and looks great.
However, cspell doesn't like 'misi' and 'mici' in the test data, can we use something else or cspell ignore?
Comment #19
mikelutzHere you go. A minor text change only, so I'll break the rules and RTBC my own patch, but I'll try and grab a +1 in slack.
Comment #20
quietone CreditAttribution: quietone commentedYes, that looks good to me too.
RTBC +1
Comment #21
catchCommitted ac1ee04 and pushed to 9.1.x. Thanks!
Comment #22
catchArrgh after committing I realised we still need a change record here.
Comment #24
huzookaWow, the credit system is really weird...
Anyway, could you please check (and correct) the draft change record I just created?
Comment #25
mikelutzRtbc just so catch will see it and has a chance to approve the cr.
Comment #26
huzooka👍 Thanks!
Comment #27
catchCR looks great, thanks!
Comment #29
alexpottSeeing this in action on #3151979: Make allow_insecure_uploads a required variable in system file migrations illustrates that the name
variables_required
is tricky. Opened #3182891: The variables_required setting is a tricky name to hopefully address this before 9.1.0.