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
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:
- 3440955-move-uploaded-file
changes, plain diff MR !7494
- 3440955-2-move-uploaded-file
changes, plain diff MR !7513
Comments
Comment #2
kim.pepperComment #4
kim.pepperComment #5
alexpottI 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.
Comment #7
kim.pepperCreated 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()Comment #8
alexpottCan 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.
Comment #9
kim.pepperYeah let's do this in #3401734: Refactor FileUploadResource to use FileUploadHandler