Skip to content
Snippets Groups Projects

Issue #3375447: FileUploadHandler::handleFileUpload() assumes an UploadedFile object

Closed Issue #3375447: FileUploadHandler::handleFileUpload() assumes an UploadedFile object
1 unresolved thread
Closed Kim Pepper requested to merge issue/drupal-3375447:3375447-uploadedfile-constraint into 11.x
1 unresolved thread

Merge request reports

Merge request pipeline passed for cfbe3ae2

Approval is optional
Code Quality is loading

Closed by Lee RowlandsLee Rowlands 1 year ago (Oct 26, 2023 4:36am UTC)

Merge details

  • The changes were not merged into 11.x.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
    • 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 call trigger_error to trigger a deprecation error to communicate that the calling code needs to update to use ::isValid or similar.

  • Kim Pepper added 6 commits

    added 6 commits

    Compare with previous version

  • Kim Pepper added 1 commit

    added 1 commit

    • c55757d9 - Add deprecation triggers and bc support for throws

    Compare with previous version

  • Kim Pepper added 1 commit

    added 1 commit

    • 38ef43d4 - Change signature of validate to only take options

    Compare with previous version

  • Kim Pepper resolved all threads

    resolved all threads

  • Kim Pepper added 1 commit

    added 1 commit

    • 4ced0fa1 - Adds legacy file upload handler test

    Compare with previous version

  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Lee Rowlands
  • Just a few minor 'modern php' things we can get in here, a question about phpstan ignores and a question about expanding the CR.

  • Kim Pepper added 1 commit

    added 1 commit

    Compare with previous version

  • Lee Rowlands resolved all threads

    resolved all threads

  • Kim Pepper added 83 commits

    added 83 commits

    Compare with previous version

  • Kim Pepper added 43 commits

    added 43 commits

    Compare with previous version

  • Kim Pepper added 1 commit

    added 1 commit

    • cfbe3ae2 - Change all deprecation messages from 10.2 to 10.3

    Compare with previous version

  • 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 comment :cry:

    Just one comment about the phpstan ignore lines

    Edited by Lee Rowlands
  • closed

  • Please register or sign in to reply
    Loading