Error responses are stored when using the Download migration process

This would likely be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

I ran into this issue as well even without using the file_existing flag. The end result of running my file migration was that I had a bunch of image requests producing 404 responses, so the migration rows failed due to the MigrateException being thrown, but since the file stream was open already and used directly by Guzzle with the sink option the file was created with the invalid response data that came through. This left my file system after the migration full of files with corrupted content even if they weren't represented in the file_managed table.

The merge request I submitted is intended as a starting place for this issue to prevent invalid files from being left on the filesystem after failed download requests. It simply deletes the created file if an exception is caught from the download request.

For reference, this is a trimmed down example of the migration being used to produce this issue:

langcode: en
status: true
dependencies: {  }
id: upgrade_d7_file
class: Drupal\migrate\Plugin\Migration
field_plugin_method: null
cck_plugin_method: null
migration_tags:
  - 'Drupal 7'
  - Content
label: 'Public files'
source:
  plugin: d7_file
  scheme: public
  constants:
    source_base_path: 'https://example.com/'
process:
  filename:
    plugin: callback
    callable: basename
    source: uri
  source_full_path:
    -
      plugin: concat
      delimiter: /
      source:
        - constants/source_base_path
        - filepath
    -
      plugin: urlencode
  uri:
    -
      plugin: download
      source:
        - '@source_full_path'
        - uri
destination:
  plugin: 'entity:file'
  validate: true
migration_dependencies:
  required: {  }
  optional: {  }

It makes sense to not have errors exist in the filesystem as files with image extensions. My gut says rather than deleting the file from the filesystem, we should download the file to the temp directory, and then move it once we know there is no errors. I'll leave at NR for others to comment.

This was discussed a while back in the weekly meeting #3277845: [meeting] Migrate Meeting 2022-05-05 1400Z @heddn pointed out that it's very possible for the temp filesystem and the public file system to be on different disks, so we can't count on the final move being free, and this would definitely half the speed of a file migration in such cases. @heddn was inclined to leave the bad file, and potentially warn/log. I'd prefer to fix it, but I'm always wary of writing code in core that deletes things, even if it is a file we just created. I wouldn't mind some thoughts from a framework manager here, on how important it is that we don't store these responses and whether deleting them after the download is a good and safe idea.

So to recap, three options as I currently see it, though if anyone can think of a 4th, I'd love to hear it.

1. Do nothing, potentially add a log message or warning if we detect a non-200 response while downloading a file.
   • Benefits: it's easy.
   • Downside: you may have files in your filesystem named my_file.jpg that contain the body of a 404 or 403 response.
2. Delete the file on non-200 responses.
   • Benefits: No error messages stored in files with an image/document extension after the migration completes.
   • Downside: downloading and then deleting a file from the filesystem. The file will have to exist in the public file system, for at least a short while. Could we accidentally delete a file we don't want to? could we potentially overwrite a good file with a bad one and then delete it? What are the edge cases to handle?
3. Download files to the temporary file system, verify integrity, then move to the final destination.
   • Benefits: safest option, could eliminate files with bad responses from ever being in the filesystem, could also do integrity checks against the file to prevent malicious scripts or XSS attacks from being migrated into the filesystem. Currently migrated files are only as safe as your file source.
   • Downsides: potential slowdown of file migrations (which already can take a long time for large filesystems), particularly if the temp directory and the public directory are on different filesystems, which is often likely to be the case.

Discussed this with @alexpott. tldr; We should delete the file on a guzzle exception since we created it above. Saving a 404 response as a .jpg file is a bug, we don't need any BC compatibility layer to fix it. NW to tighten the exception handling.

@alexpott Could I bug you for your thoughts on what we should do about #3114887: Error responses are stored when using the Download migration process sometime if you possibly get a chance? No rush, it’s not new, I’m just trying to figure out the best thing (if anything) to do, and I could use the opinion of framework manager.

Participants:

I rebased @slucero's branch on the current 11.x and made a new MR.

I support the "delete on error" approach in MR 9785.

Discussed above in 19 is the idea of using a HEAD request to check the file's existence. (I realise I'm responding to a Slack thread from two years ago here, but wanted to put this on record in case the solution turns back from the current MR approach.)

Working recently with Migrate in Islandora and large file ingests (>1GB assets) recently, there's a case where an initial HEAD will not prevent this error: if the file URL is correct and the HEAD returns 200, then retrieval fails for eg timeout or resource limits (server returns a 200 for the second GET, but the content length is incorrect). My recollection is that Guzzle does correctly throw an exception and terminate the stream in this case.

The current behaviour of the Download plugin is that the terminated stream will be written as a partial download when the request exits. This MR looks like it would correctly ensure the failed download is removed.

Just stumbled over this issue when migrating files.
I'm bold enough to claim this is RTBC:

• ✓ There's consent about the fact that this behavior is a bug
• ✓ The implemented approach in the MR isn't contested - despite alternatives
• ✓ The change ensures that no matter which file_exists is used, no file with unexpected / invalid / incomplete content remains.
• ✓ Code looks fine to me - comment properly explains why and not what is done ;)
• ✓ Has a test
• ✓ Works. Have just tested it in the migration which made me aware of this issue and it behaves as expected now.
• ? Does this need a change record? Is the change in behavior significant and / or unexpected enough?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I rebased on the main branch. The automated tests are passing, so back to RTBC.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There seem to be problems with the bot. See this thread in Slack: https://drupal.slack.com/archives/C079NQPQUEN/p1769810618759899.

I merged the current main branch, then pushed both the main branch and the feature branch to the issue fork. The CI jobs all pass.

Back to RTBC.

Removed the "Needs framework manager review" tag because #19 has sign off.

Committed and pushed to main, 11.x, and 11.3.x. It also applied cleanly to 10.6.x, so pushed there as well. Thanks everyone!