Make allow_insecure_uploads a required variable in system file migrations

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