Deprecate file_managed_file_save_upload(), file_save_upload() and _file_save_upload_from_form() and replace with a service

Thank you fur schemas!

This feels like a good first step and is compatible with 📌 [META] Modernise file upload logic Active

This deprecates:

• file_managed_file_save_upload()
• _file_save_upload_from_form()
• and file_save_upload()

and replaces them with a few new services.

\Drupal\file\Upload\FileElementHelper::saveFileUploads(array $element, FormStateInterface $formState) replaces file_managed_file_save_upload($element, FormStateInterface $form_state) and _file_save_upload_from_form(). The intention is to separate all the form api and user-facing messaging into this service.

\Drupal\file\Upload\FormFileUploadHandler::saveFileUploads(string $uploadKey, array $validators, callable $errorHandler, ?string $destination = 'temporary://', string $replace = FileSystemInterface::EXISTS_RENAME) replaces file_save_upload($form_field_name, $validators = [], $destination = FALSE, $delta = NULL, $replace = FileSystemInterface::EXISTS_RENAME).

callable $errorHandler that accept three arguments:

• \Drupal\file\Upload\UploadedFileInterface $uploadedFile
• string $destination
• \Exception $e

We then have a \Drupal\file\Upload\MessageCollectingErrorCallback which we pass to collect any errors and allows us to remove a load of the messy message deleting, and re-adding that we had in _file_save_upload_from_form()

\Drupal\file\Upload\FormUploadedFileRetriever removes the duplicated code of retrieving the uploaded files from the request, and is injected into FileElementHelper and FormFileUploadHandler.

Lastly, in order to not have to add a message for each file rename in our API-level code, instead we dispatch a \Drupal\file\Upload\FileUploadedEvent and added a MessengerFileUploadedSubscriber to add a status message if a file was renamed.

I'm confused by the phpstan failure.

First it says its comparing against 10.2.x where the MR is for 11.x. Then its showing an error about a variable $file_upload which I couldn't find anywhere in file.module.

Running PHPStan on *all* files.
 ------ -------------------------------------------------------------------- 
  Line   core/modules/file/file.module                                       
 ------ -------------------------------------------------------------------- 
         Ignored error pattern #^Variable \$file_upload in empty\(\) always  
         exists and is not falsy\.$# in path                                 
         /var/www/html/core/modules/file/file.module was not matched in      
         reported errors

Looks like I have an issue somewhere with setting the form errors and them being double-escaped:

One or more files could not be uploaded.<ul><li>The file is <em class="placeholder">1.25 MB</em> exceeding the maximum file size of <em class="placeholder">1 MB</em>.</li></ul>

Seems to have some test failures.

@larowlan as a committer do you agree with the task being done?

Updating title and remaining tasks:

• Add tests for new services
• Add legacy tests for deprecated functions
• Update mermain diagrams with current architecture
• Update issue summary

The image test fail is due to changes in the messages being displayed:

Old:

The specified file image-test.jpg could not be uploaded.

    The image is too small. The minimum dimensions are 50x50 pixels and the image size is 40x20 pixels.

New:

One or more files could not be uploaded.

    The image is too small. The minimum dimensions are 50x50 pixels and the image size is 40x20 pixels.

Finally, tests passing! 😅

Updated the mermaid diagram to match the current MR.

Updated IS and CR

One or more files could not be uploaded.

The image is too small. The minimum dimensions are 50x50 pixels and the image size is 40x20 pixels.

This no longer tells you the filename of the problem file. If you uploaded several, how are you supposed to know which one to fix?

I fixed that. There is no difference between HEAD and this MR now as to what is output.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebase with 11.x

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Merge with 11.x

Postponed on 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC since we can remove some of the exception handling once that is in.

Blocker is in

Going to try and avoid the whole 'message collector' business, and just return a new FileResults (plural) object that has a ConstraintViolationList. Might be a nice utility to write violations to Drupal::addMessage() or something like that.

Removing tag until #28 is done

Finally tests back to green. ✅ Ready for reviews again.

Hiding files from the bot.

Removing tests tag as coverage appears to be there for the deprecations here https://git.drupalcode.org/issue/drupal-3375423/-/jobs/1646261

Did leave a few comments on the MR.

btw deprecations should be updated to 11.0.0 for removal in 12.0.0

According to @larowlan we should be deprecating in 10.4.0 for removal in 12.0.0. https://drupal.slack.com/archives/C1BMUQ9U6/p1716324332447859?thread_ts=...

Updated all deprecation versions to drupal:10.4.0 and is removed from drupal:12.0.0

I find it ready now

Couple of questions on the MR, nice work!

Updated IS

Re-reviewing and appears all feedback has been addressed.

CR is straight forward too.

This change feels like it is doing more that it should. It is added typehints which mean we have to consider multiple angles of BC. It is changing logic deep in security focussed code. This should not be necessary to achieve the aims of the issue.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.