Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
In #3221796: [META] Modernise file upload logic we identified the need for stream file uploads to get the client original filename from the content-disposition header for REST and JSON API file uploads.
Lets create a service to do this.
Steps to reproduce
Proposed resolution
Create a content disposition filename parser.
Remaining tasks
Create the content disposition filename parserReplace usage in REST moduleReplace usage in JSON API module
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3380379
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:
- 3380379-create-content-disposition
changes, plain diff MR !4575
Comments
Comment #3
kim.pepperComment #4
kim.pepperComment #5
smustgrave commentedSeems like a good change.
CR reads well also and includes removed functions.
Comment #6
quietone commentedI'm triaging RTBC issues. I re-read the IS and the comments. I didn't find any unanswered questions.
I skimmed the MR. At first I thought that the constant REQUEST_HEADER_FILENAME_REGEX would need to be renamed because of the coding standards for Constants. That states that "Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them." But the I searched core and found it used in two other files, one in the files module and one in jsonapi. It would be better to keep the name the same for now and fix it later.
I think the CR could use a more succinct title and before/after code samples.
Comment #7
kim.pepperThat makes sense if it's in a
*.modulefile, but this will always be called with\Drupal\file\Upload\ContentDispositionFilenameParser::REQUEST_HEADER_FILENAME_REGEXwhich clearly shows the module and the class it comes from.In regards to the CR, I wasn't sure if this issue actually justified one. I don't think there is a before and after needed as no one is calling this code. Its an internal protected method that is duplicated in two places:
TemporaryJsonapiFileFieldUploaderandFileUploadResource. This issue removes the duplication into shared code.Comment #8
quietone commented@kim.pepper, thanks for the explanations. I do see that I must read more carefully!
Comment #9
kim.pepperBlocking a critical #2940383: [META] Unify file upload logic of REST and JSON:API therefore this is also critical.
Comment #10
kim.pepperI changed this to a static method, as we are only using the
FileSystemdependency for thebasenamemethod. And since we are just parsing a filename in content disposition header, we don't need to support StreamWrappers. Therefore the standard PHPbasenamefunction should be fine.One less service to inject!
Comment #11
smustgrave commentedSeems change to static method is fine. Not sure if it should be noted in the CR or if that matters.
Comment #12
larowlanNeeds a re-roll after #3380345: Create an InputStreamFileWriter for writing the input stream to a file
Comment #13
kim.pepperRe-rolled
Comment #15
larowlanCommitted and pushed to 11.x
Published change record
Comment #16
larowlanUpdated the change record to reflect this uses a static now and not a service