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

  1. Patch + test
  2. Subsystem maintainer feedback and review if this feature request is acceptable
  3. 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

CommentFileSizeAuthor
#19 interdiff.3152789.14-19.txt1.1 KBmikelutz
#19 3152789-19.drupal.patch8.75 KBmikelutz
#14 interdiff_3152789_2-14.txt1.77 KBankithashetty
#14 3152789_14.patch8.75 KBankithashetty
#2 core-migration_source_variable_required_variables-3152789-2.patch8.74 KBhuzooka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Status: Active » Needs review
Related issues: +#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
FileSize
core-migration_source_variable_required_variables-3152789-2.patch8.74 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Devil'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 🤓

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Issue summary: View changes

IS updated to address#3.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Issue tags: -migrate +migrate-d6-d8, +migrate-d7-d8, +migrate-d7-d9, +Needs subsystem maintainer review
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

FWIW 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!

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Just 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.

quietone’s picture

quietone CreditAttribution: quietone commented

The 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?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -migrate-d6-d8, -migrate-d7-d8, -migrate-d7-d9, -Needs subsystem maintainer review

I 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-migration_source_variable_required_variables-3152789-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented

The test failure is not due to this issue, but as I was going to re-rtbc, I realized there were some coding style issues.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/VariableTest.php
@@ -78,13 +79,130 @@ public function providerSource() {
+        'configuration' =>[
...
+        'configuration' =>[
...
+        'configuration' =>[
...
+        'configuration' =>[
...
+        'configuration' =>[
...
+        'configuration' =>[

There are several places that do not have the proper space after =>

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: Unassigned » huzooka

The test runned does not reports them I guess becasue these are pre-existing violations?..

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

Well, I was wrong. ^^ Those would be new violations.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer commented
Status: Needs work » Needs review
FileSize
3152789_14.patch8.75 KB
interdiff_3152789_2-14.txt1.77 KB

Fixed the coding style issues mentioned in #11. Kindly review.

Thank you!

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned

@ankithashetty please be more patient and don't work on issues when it is assigned to someone else.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer commented

Hi @huzooka, very sorry for the trouble... I really didn't notice that the issue was already assigned! Will be more careful henceforth...

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC though.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Seconding #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?

          'variables' => ['misi', 'mici'],
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs work » Reviewed & tested by the community
FileSize
3152789-19.drupal.patch8.75 KB
interdiff.3152789.14-19.txt1.1 KB

Here 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.

quietone’s picture

quietone CreditAttribution: quietone commented

Yes, that looks good to me too.

RTBC +1

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed ac1ee04 and pushed to 9.1.x. Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Needs work
Issue tags: +Needs change record

Arrgh after committing I realised we still need a change record here.

  • catch committed 8ecf754 on 9.1.x 
    Issue #3152789 by mikelutz, ankithashetty, huzooka, DamienMcKenna,...
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs work » Needs review

Wow, the credit system is really weird...

Anyway, could you please check (and correct) the draft change record I just created?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Rtbc just so catch will see it and has a chance to approve the cr.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

👍 Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

CR looks great, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Related issues: +#3182891: The variables_required setting is a tricky name

Seeing 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.