Make allow_insecure_uploads a required variable in system file migrations

If allow_insecure_uploads is not set the migration will skip the row.

Sorry, meant to add that the skip will happen in StaticMap.

But a "skipped row" signals there is a row to migrate … even if there literally is nothing to migrate.

If I as an end user see "skipped row", then I think that there's something to migrate, and it was skipped … for an obscure reason:

throw new MigrateSkipRowException(sprintf("No static mapping found for '%s' and no default value provided for destination '%s'.", Variable::export($value), $destination_property));

That message very much sounds like an error/failure, not like a "oh, there's nothing to do here".

If there's nothing to do, I expect to see nothing.

#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 👍

Back to RTBC as per comment #14.

It's vital that the default value of allow_insecure_uploads is FALSE. The issue summary states that it is TRUE. It took me a bit of time reading #3152789: Add required variables config to Variable migrate source plugin, and if one of those are missing, return zero rows since that is what we are using here. I'm not that keen on the name variables_required because:

• It's not a requirement - if the variable is not there then we don't halt the migration
• It made me do a big double take and think if this is required do we default the value to TRUE?

Anyway that's a separate issue. This works as advertised and if the variable is missing but we do need an issue summary update that details the actual solution and the correct default value.

Adding tag for the IS updated asked for in #17. Setting to NW for that.

Really set to NW

I opened #3182891: The variables_required setting is a tricky name to discuss the naming of variables_required.

#3182891: The variables_required setting is a tricky name just landed!

Rerolled for #3182891: The variables_required setting is a tricky name.

This issue still needs an issue summary update, as stated in #17-#19.

If you update the summary, then I will review the issue (unless @quietone beats me to it).

I think this title is clearer. Please revert the change if I misunderstand.

The proposed solution simply predates #3182891: The variables_required setting is a tricky name, which is why it was so confusing! 😅

Issue summary updated :)

@Wim Leers atm if the d7 or d6 site is missing this variable what happens? Do they can a migration which sets allow_insecure_uploads to TRUE. If that's true then we need a text to prove this is fixed and this is way more important than a minor.

#27: Very good point. It's scary that there is not already a test for this, but now is the best time ever to add it if it does not already exist 😊👍

\Drupal\Tests\system\Kernel\Migrate\d7\MigrateSystemConfigurationTest is the existing test. It was added in [#2500533. But that is indeed testing with

->values(array(
  'name' => 'allow_insecure_uploads',
  'value' => 'i:1;',
))

We need a test where that variable does not exist in the Drupal 7 DB.

Question on the testing. Seems the testing mentioned in #28 moved to the aggregator module that sound right? And is the test to check if allow_insecure_uploads is not set?

The migration of system.file settings migration (d6_system_file and d7_system_file) assumes that allow_insecure_uploads variable is always set.

I disagree. When the variable is not set the value on the row is NULL and, in this case, the static map plugin throws a MigrateSkipRowException. The row is not migrated and the destination is not changed.

Drush shows this as 'ignored'.

$ ddev drush mim d7_system_file
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
 [notice] Processed 1 item (0 created, 0 updated, 0 failed, 1 ignored) in 0.2 seconds (390.7/min) - done with 'd7_system_file'

In #6 this is considered a problem because the user thinks there is a row to migrate, when there wasn't.

I then applied that patch, rebuilt cache and re-ran the migration.

$ ddev drush mim d7_system_file
    0 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]
 [notice] Processed 0 items (0 created, 0 updated, 0 failed, 0 ignored) in 0 seconds (0/min) - done with 'd7_system_file'

The user no longer has an ignored row. That seems to be the desired result, a change in what the user sees. I do not see that as a bug. Currently, the migration will migrate the source value when the variable is set and will do nothing when the variable is not set. With the patch the migration will do the same thing.

This is tagged for tests. I do not think that is necessary because we would only be testing that the Variable source plugin is not returning a row. And that is already tested.

For myself, I do not see the need for this change (unless I am missing something). But what I don't like is that this would change the messages of the 2 system_file migrations. And, worse, it would make these 2 migrations behave different from the other 47 migrations that are using the Variable source plugin.

Seems to be back n forth if this is needed or should be done. Before reviewing is this something that could be finalized?

I think this needs the opinion of another migration maintainer, changing status and adding tag.

i checked the senerios and studied the older issue with patch, looks that it is already done in d6 and d7 file system, coming back to the " When the variable is not set the value on the row is NULL and, in this case, the static map plugin throws a MigrateSkipRowException " please elaborate more so I can work on this issue, thanks.

The issue summary suggests that the proposed change affects the result of the migration. In fact, it only affects the messages from the migration. See #34 for detailed manual testing.

In #17, #18, and #23 we asked for an update to the issue summary to clarify this, and in #24 I changed the title for the same reason.

The summary was updated in #26, but only the "Proposed resolution" section. The "Problem/Motivation" still suggests that there is a problem with the result of the migration.

In short, it has been about 2.5 years and no one has taken the trouble to explain the point of this change in the issue summary. This issue is more of a distraction than it is worth, so I am closing it. Given the issue summary, I think that "works as designed" is appropriate.

I repeated some of the manual testing from #34, to confirm the "works as designed" resolution. I installed fresh Drupal 7 and Drupal 10 sites (using the latest 7.x and 10.1.x branches). I enabled the migrate_drupal_ui module and configured it, starting at /upgrade.

I did not test the proposed patch.

Here is what I got:

$ ddev drush ms d7_system_file
 ---------------- -------- ------- ---------- ------------- --------------- 
  Migration ID     Status   Total   Imported   Unprocessed   Last Imported  
 ---------------- -------- ------- ---------- ------------- --------------- 
  d7_system_file   Idle     1       0          1                            
 ---------------- -------- ------- ---------- ------------- --------------- 
$ ddev drush cget system.file allow_insecure_uploads
'system.file:allow_insecure_uploads': false

$ ddev drush mim d7_system_file
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
 [notice] Processed 1 item (0 created, 0 updated, 0 failed, 1 ignored) in 0.1 seconds (1188.4/min) - done with 'd7_system_file'
$ ddev drush cget system.file allow_insecure_uploads
'system.file:allow_insecure_uploads': false