Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
file.module
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
10 Aug 2023 at 05:55 UTC
Updated:
12 Jan 2024 at 01:27 UTC
Jump to comment: Most recent
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