Deprecate upload-related functions file.inc and move to a file upload service

1. +++ b/core/lib/Drupal/Core/File/FileUpload.php
   @@ -0,0 +1,110 @@
   +  public function __construct(ConfigFactoryInterface $configFactory, MessengerInterface $messenger) {
   +    $this->config = $configFactory->get('system.file');
   

   It brings state to service, maybe better to keep factory here?

2. +++ b/core/lib/Drupal/Core/File/FileUpload.php
   @@ -0,0 +1,110 @@
   +  public function unmungeFilename($filename) {
   +    return str_replace('_.', '.', $filename);
   ...
   +  public function getUploadMaxSize() {
   ...
   +      $max_size = Bytes::toInt(ini_get('post_max_size'));
   ...
   +      $upload_max = Bytes::toInt(ini_get('upload_max_filesize'));
   +      if ($upload_max > 0 && $upload_max < $max_size) {
   

   This both methods looks more like static functions, the second one is more related to php ini settings then file upload
   Maybe better to extract it to Php or Requirements class like #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions

  * @return
  *   An unmunged filename string.
+ *
+ * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\File\FileUploadInterface::mungeFilename() instead.
+ *
+ * @see https://www.drupal.org/node/3021652
  */
 function file_unmunge_filename($filename) {

Should reference unmungeFilename, right?

This is massively overlapping with #2940383: [META] Unify file upload logic of REST and JSON:API! Please continue working on that issue instead. That already has 2 proven consumers: the core REST module and the contrib-soon-core JSON:API module. Having that same service be used by everything else doing file uploads in core would be a huge win, not the least in terms of security hardening!

As I see it makes sense to split tasks - file uploader service in related issue use much more dependencies in constructor, guts tells it needs factoring

1. +++ b/core/lib/Drupal/Core/File/FileUpload.php
   @@ -0,0 +1,111 @@
   +  protected $messenger;
   

   If this service is ever going to be used in HTTP APIs, then this cannot be injected. Instead, an exception needs to be thrown that the caller needs to convert into calls to the messenger service. The caller knows whether we're operating in a HTML or API context.

2. +++ b/core/lib/Drupal/Core/File/FileUploadInterface.php
   @@ -0,0 +1,65 @@
   + * Provides methods to help with file uploads.
   ...
   +interface FileUploadInterface {
   ...
   +  public function mungeFilename($filename, $extensions, $alerts = TRUE);
   ...
   +  public function unmungeFilename($filename);
   ...
   +  public function getUploadMaxSize();
   

   These methods indeed are only helpers for file uploads, they don't do actual file uploads. The third method also seems pretty unrelated to the other two.

   Either this service needs to become more capable so that its name matches its capabilities, or it needs to be renamed.

   I think the first two methods could be on a UploadedFileNameMunger service.

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -300,7 +300,7 @@ public function getUploadValidators() {
   -    $max_filesize = Bytes::toInt(file_upload_max_size());
   +    $max_filesize = Bytes::toInt(\Drupal::service('file_upload')->getUploadMaxSize());
   

   🎉

4. +++ b/core/lib/Drupal/Core/File/FileUploadInterface.php
   @@ -0,0 +1,65 @@
   +interface FileUploadInterface {
   

   Is having an interface for this really necessary? What's the use case for overriding these? I think only a single concrete implementation is sufficient for now.

Apparently no more reviews are coming in, so marking NW.

Another issue impacted by this: #2492171-230: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc).

Yeah, what about file.module and _file_save_upload_single() itself? Should that be considered in scope for this effort, or do we want to handle that separately? It's a function I find myself way too familiar with after slogging it out in #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) (thanks, @Wim, for already marking that related).

#997900: Not possible to allow uploading files with any file extension is another one that overlaps/conflicts with this.

Re: #14. Currently there is almost zero overlapping between the patch here and your issue, biggest overlap is possibly the name but that's in flux anyway.

However, there so seem to be a real large overlap with #3032390: Add an event to sanitize filenames during upload which completely replaces the munge/unmunge functions and makes the whole thing an implementation detail instead of an API.

With that, the only thing that's left here is one tiny API function that has zero dependencies: file_upload_max_size(), I proposed in another issue recently that drupal_set_time_limit() could be added to that as well and possibly this max upload thing could do there as well?

@kim.pepper It's linked already #3000057-13: Deprecate drupal_set_time_limit() and file_upload_max_size() and move to Environment component

munge/unmunge is being completely refactored at #3032390: Add an event to sanitize filenames during upload

file_upload_max_size() now RTBC @ #3000057: Deprecate drupal_set_time_limit() and file_upload_max_size() and move to Environment component

I think "outdated" is the right status in here now, since everything is now handled elsewhere.

Thanks,
-Derek