Issue #3375447: FileUploadHandler::handleFileUpload() assumes an UploadedFile object
Merge request reports
Activity
added 82 commits
-
b3e943e9...e646c8e5 - 82 commits from branch
project:11.x
-
b3e943e9...e646c8e5 - 82 commits from branch
added 1 commit
- b3efa9dc - Replace exception with violations for entity validate too
added 98 commits
-
b3efa9dc...72e7c019 - 92 commits from branch
project:11.x
- cae66659 - Use uploaded file validator
- 0904ba7f - Fix test fails
- 795bc379 - Improve how we test maxSize
- 27ff03dd - Add deprecations for interface methods
- aaa969d4 - Swap exceptions with violations
- c2db92f5 - Replace exception with violations for entity validate too
Toggle commit list-
b3efa9dc...72e7c019 - 92 commits from branch
added 143 commits
-
c2db92f5...e3e45ab3 - 135 commits from branch
project:11.x
- 755e2a1c - Use uploaded file validator
- eda4028d - Fix test fails
- 53ecbd7f - Improve how we test maxSize
- 7bf6259e - Add deprecations for interface methods
- 9d44346f - Swap exceptions with violations
- b3efa9dc - Replace exception with violations for entity validate too
- 61283421 - Merge branch '11.x' into 3375447-uploadedfile-constraint
- dd781d86 - Merge branch '3375447-uploadedfile-constraint' of...
Toggle commit list-
c2db92f5...e3e45ab3 - 135 commits from branch
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
Looks good, I think there's one BC concern we're missing here.
Previously ::handleFileUpload used exceptions to control flow in the invalid cases, but now it does not (by design).
However, calling code might be relying on those exceptions to check if the result is valid, and as such won't know it needs to call $result->getViolations()> 0 (or a new ::isValid as discussed above).
I think we need an additional parameter to ::handleFileUpload that is
bool $throw
which defaults to TRUE. In new code we can pass FALSE.In old code, it will get the default of TRUE.
If
$throw
is TRUE, we should check the violation list and throw an Exception if the file isn't valid.Additionally, when
$throw
is true we can also calltrigger_error
to trigger a deprecation error to communicate that the calling code needs to update to use ::isValid or similar.
added 6 commits
-
f5ba86c3...22576037 - 5 commits from branch
project:11.x
- 8dd36b27 - Merge branch '11.x' into 3375447-uploadedfile-constraint
-
f5ba86c3...22576037 - 5 commits from branch
added 1 commit
- c55757d9 - Add deprecation triggers and bc support for throws
added 1 commit
- 38ef43d4 - Change signature of validate to only take options
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Lee Rowlands
- Resolved by Kim Pepper
- Resolved by Kim Pepper
- Resolved by Kim Pepper
added 83 commits
-
cb596c21...5e8d5e8a - 82 commits from branch
project:11.x
- 6e635225 - Merge branch '11.x' into 3375447-uploadedfile-constraint
-
cb596c21...5e8d5e8a - 82 commits from branch
added 43 commits
-
6e635225...1bdb6ed2 - 42 commits from branch
project:11.x
- 7ae06c70 - Merge branch '11.x' into 3375447-uploadedfile-constraint
-
6e635225...1bdb6ed2 - 42 commits from branch
added 1 commit
- cfbe3ae2 - Change all deprecation messages from 10.2 to 10.3
169 171 * @throws \Drupal\file\Upload\FileValidationException 170 * Thrown when file validation fails. 172 * Thrown when file validation fails and $throws is TRUE. 171 173 */ 172 public function handleFileUpload(UploadedFileInterface $uploadedFile, array $validators = [], string $destination = 'temporary://', int $replace = FileSystemInterface::EXISTS_REPLACE): FileUploadResult { 174 public function handleFileUpload(UploadedFileInterface $uploadedFile, array $validators = [], string $destination = 'temporary://', int $replace = FileSystemInterface::EXISTS_REPLACE, bool $throw = TRUE): FileUploadResult { 173 175 $originalName = $uploadedFile->getClientOriginalName(); 174 175 if (!$uploadedFile->isValid()) { 176 // @phpstan-ignore-next-line 177 if ($throw && !$uploadedFile->isValid()) { 178 @trigger_error('Calling ' . __METHOD__ . '() with the $throw argument as TRUE is deprecated in drupal:10.3.0 and will be removed in drupal:11.0.0. Use \Drupal\file\Upload\FileUploadResult::getViolations() instead. See https://www.drupal.org/node/3375456', E_USER_DEPRECATED); 179 // @phpstan-ignore-next-line 176 180 switch ($uploadedFile->getError()) { 177 181 case \UPLOAD_ERR_INI_SIZE: 182 // @phpstan-ignore-next-line Just a few minor 'modern php' things we can get in here, a question about phpstan ignores and a question about expanding the CR.Gee thanks gitlab for remembering my last commentJust one comment about the phpstan ignore lines
Edited by Lee Rowlands