Problem/Motivation

Currently, FileUploadHandler provides a method \Drupal\file\Upload\FileUploadHandler::moveUploadedFile() to allow an extension point to move both FormUploadedFiles and yet to be implemented InputStreamUploadedFiles that will be used to DRY up file upload handling for both form file uploads and JSON API and REST file uploads.

The todo reads:

   * @todo Allows a sub-class to override this method in order to handle
   * raw file uploads in https://www.drupal.org/project/drupal/issues/2940383.
  

FormUploadedFile is required to use \Drupal\Core\File\FileSystemInterface::moveUploadedFile() in order to support PHP's standard move_uploaded_file() method.

The proposed InputStreamUploadedFile cannot use this, and is required to use \Drupal\Core\File\FileSystemInterface::move() instead.

However, since this was proposed as the solution, we have added \Drupal\file\Upload\UploadedFileInterface::validate() to validate file uploads (before they become file entities).

We can follow this pattern for moving file uploads too.

Steps to reproduce

Proposed resolution

Add a new method \Drupal\file\Upload\UploadedFileInterface::moveUploadedFile(FileSystemInterface $fileSystem, string $destination) that allows the implementing class to provide the correct way of moving uploaded files.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3440955

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

kim.pepper created an issue. See original summary.

kim.pepper’s picture

Title: Provide a method for FileUploads to be moved » Provide a method for FileUploads to be moved in UploadedFileInterface

kim.pepper’s picture

Status: Active » Needs review
alexpott’s picture

I wonder if this is a good idea from a security implication. My concern is that only the FileUpload service should do this - which is separate to the validate method. I think it might be best to continue to handle the moves in FileUpload but we could have the type of move determined by the class or maybe we want an interface to indicate this. I think I'd use classes until we've got 3 or more objects implementing UploadedFileInterface.

I considered an alternative where we invert the control flow between the FileUploadHandler and instead have a UploadedFileInterface factory that creates objects that have the FileUploadHandler injected but I'm not sure that that gets us anywhere.

kim.pepper’s picture

Created a new MR that just checks if it is a FormUploadedFile and uses \Drupal\Core\File\FileSystemInterface::moveUploadedFile() or otherwise \Drupal\Core\File\FileSystemInterface::move()

alexpott’s picture

Can we do this in an issue that creates InputStreamUploadedFiles otherwise we have to test via mocking or awkwardness?

I think the approach is fine and can be improved upon as needed.

kim.pepper’s picture

Status: Needs review » Closed (won't fix)
Related issues: +#3401734: Refactor FileUploadResource to use FileUploadHandler