The migration of system.mail settings migration (d7_system_mail) assumes that the mail_system variable is always set, and it always has a default-system key. This is not correct. I checked the most recent Drupal 7 codebase, and the only place where it is used (excluding tests) is the drupal_mail_system() function, but that function only reads this variable.
Change the interface/default process pipeline: If the mail_system variable is not null, try to explode it first before applying the static_map. The static map should allow bypassing, and have a trailer default value process (with the Drupal 8|9 default value). If the mail_system variable doesn't exist (so it is NULL), then we also have to use the Drupal8|9 default value.
variables_required to variables_no_row_if_missing after #3182891: The variables_required setting is a tricky name (Novice)| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 3151980-19.patch | 443 bytes | anmolgoyal74 |
| #11 | core-migrate_system_mail_settings-3151980-11.patch | 434 bytes | huzooka |
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
Comments
Comment #2
huzookaComment #3
quietone commentedHmm, I grepped Drupal 7 as well and found that mail.inc does set the variable mail_system.
drupal/includes/mail.inc: $configuration = variable_get('mail_system', array('default-system' => 'DefaultMailSystem'));And even if the variable is not set the source plugin will return NULL. Then the static map will throw MigrateSkipRowException so nothing is written to configuration on the destination.
Comment #4
huzooka@quietone, that' is a variable_get() with a default value, not a setter.
Comment #5
huzookaI don't really agree with that throwing an error is the right solution.
Comment #6
quietone commentedThe goal is to migrate the value from the source site. If the source site value is not set then there is no data to migrate and skipping the row is the correct action.
Comment #7
huzookaI can't agree.
If the source site value is not set (so when is no data to migrate) then the correct action is to tell that there are zero rows to migrate in this migration and not throwing exceptions and logging errors.
Comment #8
huzookaAnd especially, what should happen if a contrib module wants to provide migration path for its mailing system?..
Comment #9
wim leersThis seems to be once again the problem that the migration system's handling of the
variabletable insists on having a single row just to signal migration status, even if there literally is nothing to migrate.Why can't we use
\Drupal\migrate\Plugin\RequirementsInterfacefor this?Comment #10
quietone commented@huzooka, I see your point. I am just so used to it being this way.
Comment #11
huzookaComment #12
huzookaComment #14
wim leers#3152789: Add required variables config to Variable migrate source plugin, and if one of those are missing, return zero rows landed a month ago — no blockers left for this! Been using this for months and it works great 👍
Comment #15
benjifisherI do not know why the testbot did not set the status to NW with the failing test.
Just a guess based on the previous comment: this issue may need an update after #3182891: The variables_required setting is a tricky name.
Comment #16
benjifisherComment #17
benjifisherA quick look at the patch confirms my guess. This update looks like a Novice task, so I am adding the tag and updating the "Remaining tasks" in the issue summary.
Comment #18
wim leersCorrect :) Thanks for adding the tag!
Comment #19
anmolgoyal74 commentedComment #20
benjifisher@anmolgoyal74:
Thanks for updating a patch!
We now have the option of using merge requests (MRs). Look for the green "Create issue fork" button. But if you continu using the old patch-based approach, then please attach an interdiff along with the patch. See Creating an interdiff.
Comment #22
benjifisher@anmolgoyal74:
I should have suggested that a Novice task like this is a good time to try something new, like creating a MR, but it looks as though you did not need that nudge.
It seems that creating a MR does not automatically set the issue status to NR. That may change, but for now, please check that when you create a new MR.
Back to RTBC based on #14.
Comment #23
alexpottComment #24
alexpottCommitted and pushed 36b600e480 to 9.2.x and 749fc09ae7 to 9.1.x. Thanks!
Comment #27
wim leers🥳