Problem/Motivation

I was reviewing #3151979: Make allow_insecure_uploads a required variable in system file migrations and this setting is being used to skip the migration if the variable is missing. That's not the usual meaning of required (in the way it is used in modules, config dependencies and migration dependencies). If something is required and it is missing then the system will error not ignore.

This feature was added in #3152789: Add required variables config to Variable migrate source plugin, and if one of those are missing, return zero rows

Proposed resolution

Come up with a better name. The other important thing is that if any of these variables are missing then we return an empty row no row.

Rename variables_required to variables_no_row_if_missing

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 3182891-4.patch7.59 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

@mikelutz pointed out that it is not an empty row - it's no row... updated options accordingly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

I don't think the any is important for the no_row option.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3182891-4.patch7.59 KB

Discussed with @catch. We settled on variables_no_row_if_missing

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php
@@ -80,28 +81,20 @@ class Variable extends DrupalSqlBase {
   /**
-   * The optional variables.
+   * The variables that result in no row if any are missing from the source.
    *
    * @var array
    */
-  protected $optionalVariables;

The optionalVariables property is never used. Having a variable named like this is confusing so I've removed it.

catch’s picture

catch
he/him
Primary language English
commented

#4 looks good. I found the issue title confusing for the original issue, but then when reading the patch it all fell into place. But someone else having the same reaction to the variables_required when seeing it for the first time confirms I should've held onto that initial feeling. Since this hasn't been in a stable release it seems least impact to just rename it asap.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record updates

While I never had a problem with variables_required, I can understand the confusion around it, and have no objection to a naming change. We need a new Change Record, or we need to update the original at https://www.drupal.org/node/3174294

  • catch committed bbf50c9 on 9.2.x 
    Issue #3182891 by alexpott: The variables_required setting is a tricky...

  • catch committed 2b40c4f on 9.1.x 
    Issue #3182891 by alexpott: The variables_required setting is a tricky...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs change record updates

#7: Done: https://www.drupal.org/node/3174294 — diff at https://www.drupal.org/node/3174294/revisions/view/12112670/12128176

Status: Fixed » Closed (fixed)

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