As part of #2940383: [META] Unify file upload logic of REST and JSON:API we want to refactor common code. As a smaller step to that goal, we can refactor \Drupal\file\Plugin\rest\resource\FileUploadResource to use the existing \Drupal\file\Upload\FileUploadHandler::handleFileUpload() and thereby reduce the size of the larger refactoring.
Refactor FileUploadResource to use FileUploadHandler
Show commandsStart 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:
compare
Comments
Comment #2
kim.pepperFound a mismatch in behaviour with the
filenamefield on theFileentity not being updated when we do a rename.I think this is now blocked on #3402981: Filename property is not updated when a duplicate file is renamed In REST and JSON API file uploads
Comment #4
kim.pepperCreated a MR that combines #3402981: Filename property is not updated when a duplicate file is renamed In REST and JSON API file uploads
Comment #5
larowlanBlocker is in
Comment #6
kim.pepperThere is a test failure due to a behaviour change. I'm scratching my head to work out why.
Before:
example.phpgets renamed:example.php => example.php_.txtAfter:
example.phpgets rejected because the.phpextension isn't in the allow extensions list.Comment #7
kim.pepperMany thanks to @larowlan for helping resolve the file extensions issues. 🙌🏻🥳
This is ready for reviews now.
Comment #8
kim.pepperChanging this to critical as it is a part of #2940383: [META] Unify file upload logic of REST and JSON:API which is critical.
Comment #9
alexpottAdded a review to the MR... this is looking lovely.
Comment #11
kim.pepper@alexpott are you happy if we look into your concerns in #3403253: Simplify how 'allow all extensions' file upload validation works?
Comment #12
alexpottRe #11 yeah - I don't think your code is changing any of the current behaviour ... it's just whenever I see code allowing all extensions I get very security nervous.
Comment #13
kim.pepperRe-organizing parent meta issue.
Comment #14
kim.pepperOnce this goes in, we can start on #3444748: Refactor JSON-API file uploads to use FileUploadHandler
Comment #15
longwaveThis looks great, unifying all this is definitely the way to go, but the patch does not apply to 10.3.x/10.4.x so we will need a separate MR.
I'm also in two minds about removing the deprecation in 11.x at this stage. https://www.drupal.org/project/file_upload_options does extend this plugin in contrib but only has a beta release and 123 users.
If we do remove the deprecation in 11.x then we will need an 11.x MR with the deprecations removed. Otherwise, the deprecations need updating to 12.x
Comment #17
alexpottI think since it is constructor deprecations and not actually covered by our BC promise we should deprecate in 10.x and remove in 11.x. Contrib does not need to override the constructor - they can do what they need in the create method.
Removed deprecations from the 11.x branch and created an 10.4.x branch resolving the conflict.
Comment #18
larowlanCouple of deprecations still in the 11.x branch - other than that looks good to me
Comment #19
alexpottremoved the deprecated functions from 11.x branch. nice catch @larowlan
Comment #20
longwaveCommitted f757b3d and pushed to 11.x. Thanks!
Committed and pushed 8f38f5d839 to 10.4.x and 48c58073ee to 10.3.x. Thanks!
Comment #23
longwaveComment #28
kim.pepperWoohoo! 🎉 See you in #3444748: Refactor JSON-API file uploads to use FileUploadHandler
Comment #29
kim.pepperWe left in the method
InputStreamUploadedFile::supportsMoveUploadedFile()from an approach that we ditched in favour of theInputStreamUploadedFile::validate()approach. Created #3446962: Remove incorrectly added InputStreamUploadedFile::supportsMoveUploadedFile() to clean this up.