Move _file_save_upload_single to a service and deprecate

Great! it will help to centralize access to request's files and transition to entity

The testbot is not happy.

Fix linter, and removed string translation dependency and exceptions now using sprintf() (for linter)

Missing @group annotation in Drupal\Tests\file\Kernel\FileUploaderTest in /var/www/html/core/lib/Drupal/Core/Test/TestDiscovery.php:362

I was able to figure out why the tests are failing in RemoteFileSaveUploadTest::testDrupalMovingUploadedFileError but not in RemoteFileSaveUploadTest::testHandleExtension and RemoteFileSaveUploadTest::testHandleDangerousFile.

1. +++ b/core/modules/file/src/Upload/FileUploadResult.php
   @@ -0,0 +1,133 @@
   +   * Creates a new instance of FileUploadResult.
   ...
   +   * @return $this
   ...
   +  public static function create(): FileUploadResult {
   +    return new static();
   
   +++ b/core/modules/file/src/Upload/FileUploader.php
   @@ -0,0 +1,396 @@
   +    $result = FileUploadResult::create()
   

   Not sure having this is a good idea, it exposes extra API which is not so much handy (only 1 usage to replace)

2. +++ b/core/modules/file/src/Upload/FileUploader.php
   @@ -0,0 +1,396 @@
   + * Provides a file uploader.
   

   Could use better docs - it's kinda converter to value object

1. +++ b/core/modules/file/file.module
   @@ -928,9 +984,14 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +  @trigger_error(__METHOD__ . '() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\file\Upload\FileUploader::createFromUpload() instead. See https://www.drupal.org/node/123', E_USER_DEPRECATED);
   

   Needs a CR and updated URL.

2. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,396 @@
   +    // A file URI may already have a trailing slash or look like "public://".
   ...
   +    throw new FileException();
   

   Should this be throw new SymfonyFileException();?

+++ b/core/modules/file/file.services.yml
@@ -4,3 +4,6 @@ services:
+  file.uploader:

Should the service be named file.upload_handler to match the class name?

RE: #30 I was thrown off my the use statements in file.module, ignore me.

There's a subtle behaviour change. In _file_save_upload_single() we set $file->source = $form_field_name;. This is no longer happening in the new code. This line has always kinda bothered me and it would be great to remove it. But we need to be sure that contrib is not depending on it for something. I think this was introduced in https://git.drupalcode.org/project/drupal/-/commit/41d2b2a349564c7a55df9... and seems to be used once in contrib - see http://grep.xnddx.ru/search?text=file-%3Esource&filename=

Didn't review yet (sorry), but FWIW, $file is not actually a file entity, that's a raw DB result that it loaded directly from file_managed, so I have no idea where it thinks ->source is coming from.

Doesn't mean that something else isn't using it, but I don't think that one is something we need to be concerned about.

@Berdir yep the thing from contrib in #34 is bogus. You're right - the $file seems to the result of selecting directly from the file_managed table which does not have a source column and did not in D7 either. Odd.

I think we still need to document this on the CR and issue summary.

I have lots of thoughts about translatable messages being used as exception messages. I don't think this is how we should do that. I think we should have a method that can convert an exception into a translatable message but the exception message should not be translatable.

I find it ready to go except 2 nits, sorry(

1. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -31,78 +31,104 @@ class ErrorUtils {
   -  public static function throwException(string $original_file_name, int $errorCode) {
   +  public static function throwException(string $filename, int $errorCode) {
   ...
   +  public static function getTranslatableMessage(int $errorCode, string $filename): TranslatableMarkup {
   

   ubernit - why not fileName)

2. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -31,78 +31,104 @@ class ErrorUtils {
   +    return sprintf('The file %s could not be saved because it exceeds %s, the maximum allowed size for uploads.',
   ...
   +    return sprintf('The file %s could not be saved because the upload did not complete.', $filename);
   ...
   +    return sprintf('The file %s could not be saved. An unknown error has occurred.', $filename);
   

   as it's a file name would be great to have " or ' around it

Thank you, gtg

+++ b/core/modules/file/src/Upload/FileUploadHandler.php
@@ -0,0 +1,299 @@
+   * Constructs FileUploader object.

nit: this should also be FileUploadHandler

I keep meaning to review this (sorry!). This at least fixes nit #44. ;) I hope to dive in more closely tomorrow...

Time's up for today, only a partial review. Mostly looks great so far. 1 question about consistency in user-facing messages, another ubernit, and a note-to-self about something I noticed while reviewing. ;)

1. +++ b/core/modules/file/file.module
   @@ -892,9 +898,69 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +          $message = t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);
   ...
   +          $message = t('Your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);
   ...
   +          '#markup' => t('The specified file %name could not be uploaded.', ['%name' => $e->getFilename()]),
   ...
   +      \Drupal::logger('file')->notice('Upload error. Could not move uploaded file %file to destination %destination.', ['%file' => $uploaded_file->getClientOriginalName(), '%destination' => $destination . '/' . $uploaded_file->getClientOriginalName()]);
   ...
   +      \Drupal::messenger()->addError(t('The file %filename could not be uploaded because the name is invalid.', ['%filename' => $uploaded_file->getClientOriginalName()]));
   

   Do we want to be consistent about wrapping filenames in " in these messages? Or do we want to keep the strings as they were since in many cases the translations already exist from identical t() strings in _file_save_upload_single()?

2. +++ b/core/modules/file/file.module
   @@ -892,9 +898,69 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +      \Drupal::messenger()->addError(ErrorUtils::getTranslatableMessage($e->getCode(), $uploaded_file->getFilename()));
   

   Slick. This is why I didn't initially see some of the other possible error messages in the new method that _file_save_upload_single() can generate. Cool.

3. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -0,0 +1,134 @@
   +   * Throw the appropriate Symfony exception for the given upload error code.
   

   Nit: "Throws".

I'll plan to continue tomorrow...

Thanks/sorry, -Derek

Re #47: 👍 makes perfect sense. Thanks!

Got through the rest of the patch. This is all great work!

A ton of fiddly docs nitpicks follow. I'll include a patch that addresses all those (and #46.3). The un-addressed points in this list are:

4. Minor possible doc improvement, but wanted to make sure I understood before I changed anything.
7. This is the only real point of substance that I'd love @kim.pepper to address.
9. Follow-up? Or do it here? Or?
15. Follow-up needed?

Full dreditor review follows. Again, attached patch addresses all of this except the 4 points above.

1. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -0,0 +1,134 @@
   +   *   The original filename.
   

   More nit: If we're fileName'ing the param, it's because we consider "file" and "name" separate words. So this should be "The original file name". ;)

2. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +   * The mime type guesser.
   ...
   +   *   The mime type guesser.
   

   Technically, "mime" is an acronym and should be all caps.

3. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +   * @param array $validators
   +   *   The uploaded file.
   

   Doc doesn't match the param.

4. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +   *   The file destination name.
   

   Do we want to mention this is expected to be a directory, not a final filename?

5. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +    if ($replace == FileSystemInterface::EXISTS_REPLACE) {
   

   Might as well use ===, right?

6. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +      throw new FileValidationException("File validation failed", $filename, $errors);
   ...
   +      throw new FileValidationException("File validation failed", $filename, $errors);
   

   Why " vs ' for this?

7. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +    // Update the filename with any changes as a result of the renaming due to an
   +    // existing file.
   +    $file->setFilename($this->fileSystem->basename($file->getFileUri()));
   ...
   +    // Update the filename with any changes as a result of security or renaming
   +    // due to an existing file.
   +    $file->setFilename($this->fileSystem->basename($destinationFilename));
   

   I don't understand why:

   a) We do this twice.
   b) With different logic of the source of truth of the new filename.

   $destinationFilename doesn't change between these two calls. I think one of them needs to be removed.

8. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +   * @param array $validators
   +   *
   +   * @return string
   

   Missing docs.

9. +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,299 @@
   +      $extensions = 'jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp';
   

   Should this list live in a class constant somewhere? I see it's now duplicated both here and in the deprecated method. Perhaps in other spots, too.

10. +++ b/core/modules/file/src/Upload/FileUploadResult.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * If the file was renamed from the original.
    +   *
    +   * @var bool
    +   */
    +  protected $rename = FALSE;
    

    This is dead code, and now handled computationally via isRenamed().

11. +++ b/core/modules/file/src/Upload/FileUploadResult.php
    @@ -0,0 +1,124 @@
    +   * @return FileUploadResult
    ...
    +   * @return FileUploadResult
    ...
    +   * @return FileUploadResult
    ...
    +   * @return FileUploadResult
    

    I thought `return $this` was better for such docs.

12. +++ b/core/modules/file/src/Upload/FileUploadResult.php
    @@ -0,0 +1,124 @@
    +  /**
    +   * @param string $sanitizedFilename
    ...
    +  /**
    +   * @return string
    +   */
    ...
    +  /**
    +   * @param string $originalFilename
    ...
    +  /**
    +   * @param \Drupal\file\FileInterface $file
    ...
    +  /**
    +   * @return bool
    ...
    +  /**
    +   * @return string
    ...
    +  /**
    +   * @return \Drupal\file\FileInterface
    

    Missing doc 1-liners.

13. +++ b/core/modules/file/src/Upload/FileUploadResult.php
    @@ -0,0 +1,124 @@
    +   * If there was a file rename.
    

    Should start with a verb.

14. +++ b/core/modules/file/src/Upload/FileValidationException.php
    @@ -0,0 +1,60 @@
    +  /**
    +   * The filename.
    +   *
    +   * @var string
    +   */
    +  protected $filename;
    ...
    +   * @param string $filename
    +   *   The filename.
    ...
    +  public function __construct(string $message, string $filename, array $errors) {
    ...
    +    $this->filename = $filename;
    ...
    +   * Gets the filename.
    ...
    +   *   The filename.
    ...
    +    return $this->filename;
    

    For consistency with the rest of this patch, these should all be file name, fileName, etc.

15. +++ b/core/modules/file/src/Upload/FileValidationException.php
    @@ -0,0 +1,60 @@
    +  public function getFilename(): string {
    

    Ugh, except we have a bunch of `getFilename()` methods everywhere else in core, so it's probably not worth changing this to getFileName().

I promised I'd look into #49.7. Here's what I found:

Looking at git blame for _file_save_upload_single(), the first occurrence of setFilename() is from here:

commit 69a0a8354628e2567120ff4985620029059ec191
Author: catch <catch@35733.no-reply.drupal.org>
Date:   Wed Feb 24 16:02:18 2021 +0000

    Issue #3032390 by alexpott, dww, Pancho, 3CWebDev, kim.pepper, larowlan, Berdir, catch, andypost, chr.fritsch, Wim Leers: Add an event to sanitize filenames during upload

While the 2nd is from here:

commit 9431a1cb8ed28d086d93ee7c0973a34eaa9d7010
Author: Lee Rowlands <lee.rowlands@previousnext.com.au>
Date:   Mon Feb 25 20:45:21 2019 +1000

    Issue #3032376 by alexpott, dww, Berdir: Files renamed by \_file_save_upload_single() do not have the correct filename on the File entity

It seems plausible that since 3032390 took so long to land, we didn't notice that 3032376 had already landed and already got this point right. ;) I think it's safe to remove the duplicate 2nd call now, but I'd love to get @alexpott to confirm if he agrees.

+++ b/core/modules/file/src/Upload/FileUploadHandler.php
@@ -0,0 +1,306 @@
+    // Update the filename with any changes as a result of the renaming due to an
+    // existing file.
+    $file->setFilename($this->fileSystem->basename($file->getFileUri()));
...
+    // Update the filename with any changes as a result of security or renaming
+    // due to an existing file.
+    $file->setFilename($this->fileSystem->basename($destinationFilename));

I think we want the comment from the second with the code of the first :)

I'm not a huge fan of the ErrorUtils class. I think we can get rid of it and have easier to follow code with less indirection.

1. +++ b/core/modules/file/file.module
   @@ -892,9 +898,69 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +      \Drupal::messenger()->addError(ErrorUtils::getTranslatableMessage($e->getCode(), $uploaded_file->getFilename()));
   +      $files[$i] = FALSE;
   

   I think the messages should be inlined here. I think the indirection doesn't really buy us anything and adds complexity. If you want to handle file uploads with messages - use file_save_upload() and wait until we've provided that as a service.

2. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -0,0 +1,134 @@
   +  /**
   +   * Throws the appropriate Symfony exception for the given upload error code.
   +   *
   +   * @param string $fileName
   +   *   The original file name.
   +   * @param int $errorCode
   +   *   The error code.
   +   *
   +   * @throws \Symfony\Component\HttpFoundation\File\Exception\FileException
   +   *   Throws FileException or a sub-class for each kind of error.
   +   *
   +   * @see https://www.php.net/manual/en/features.file-upload.errors.php
   +   */
   +  public static function throwException(string $fileName, int $errorCode) {
   
   +++ b/core/modules/file/src/Upload/FileUploadHandler.php
   @@ -0,0 +1,306 @@
   +    if (!$uploadedFile->isValid()) {
   +      ErrorUtils::throwException($originalName, $uploadedFile->getError());
   +    }
   

   I think we can change all of this so that we don't have ErrorUtils::throwException()...

   I think this could be:

           switch ($uploadedFile->getError()) {
               case \UPLOAD_ERR_INI_SIZE:
                   throw new IniSizeFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_FORM_SIZE:
                   throw new FormSizeFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_PARTIAL:
                   throw new PartialFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_NO_FILE:
                   throw new NoFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_CANT_WRITE:
                   throw new CannotWriteFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_NO_TMP_DIR:
                   throw new NoTmpDirFileException($uploadedFile->getErrorMessage());
               case \UPLOAD_ERR_EXTENSION:
                   throw new ExtensionFileException($uploadedFile->getErrorMessage());
           }
   
           throw new FileException($uploadedFile->getErrorMessage());
   

   I think indirection when throwing exceptions makes it harder to rationalise about what is going on.

All my reviews are picking at the edges of this change. Just to make it clear - I think this change is fantastic and will help maintain form, jsonapi, rest file uploads and not have to duplicate code and have separate bugs. Thanks @kim.pepper for all the effort here.

+++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
@@ -21,23 +20,22 @@ class FileModuleTest extends KernelTestBase {
-    $file_info->expects($this->once())->method('getError')->willReturn(UPLOAD_ERR_FORM_SIZE);
-    $file_info->expects($this->once())->method('getClientOriginalName')->willReturn($file_name);
-    $this->assertFalse(\_file_save_upload_single($file_info, 'name'));
-    $expected_message = new TranslatableMarkup('The file %file could not be saved because it exceeds %maxsize, the maximum allowed size for uploads.', ['%file' => $file_name, '%maxsize' => format_size(Environment::getUploadMaxSize())]);
-    $this->assertEquals($expected_message, \Drupal::messenger()->all()['error'][0]);
+    $file_info->expects($this->once())
+      ->method('getError')
+      ->willReturn(UPLOAD_ERR_FORM_SIZE);
+    $file_info->expects($this->once())
+      ->method('getClientOriginalName')
+      ->willReturn($file_name);
+    $this->expectDeprecation('_file_save_upload_single() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\file\Upload\FileUploadHandler::handleFileUpload() instead. See https://www.drupal.org/node/3239547');
+    $this->assertFalse(_file_save_upload_single($file_info, 'name'));

I think the message assertion should still be in the test. And there's quite a bit of unrelated change that shouldn't be done here either.

#54 would be even simpler using match, perhaps we could add a D10 followup to move from switch to match in D10 when php8 is required

1. +++ b/core/modules/file/src/Upload/ErrorUtils.php
   @@ -0,0 +1,134 @@
   +  public static function throwException(string $fileName, int $errorCode) {
   ...
   @@ -0,0 +1,134 @@
   +    switch ($errorCode) {
   +      case \UPLOAD_ERR_INI_SIZE:
   ...
   +      case \UPLOAD_ERR_FORM_SIZE:
   ...
   +      case \UPLOAD_ERR_PARTIAL:
   ...
   +      case \UPLOAD_ERR_NO_FILE:
   ...
   +      case \UPLOAD_ERR_NO_TMP_DIR:
   ...
   +      case \UPLOAD_ERR_CANT_WRITE:
   ...
   +      case \UPLOAD_ERR_EXTENSION:
   ...
   +    throw new FileException(static::getDefaultMessage($fileName), $errorCode);
   

   filed #3243450: Replace FileUploadHandler:: handleFileUpload() switch with match when core requires PHP 8

I reviewed the interdiffs from #59 and #61. Definitely looks good to me and that all of #53, #54 and #55 are addressed. #53 takes care of #49.7, so that's the last point from #49, too.

I also updated the title/summary for #3243450: Replace FileUploadHandler:: handleFileUpload() switch with match when core requires PHP 8 since the switch moved in #59. ;) Thanks for opening that follow-up, @andypost.

Although I uploaded a few patches for this, they were only to do trivial doc fixes and the like, so I think I'm still qualified to RTBC this. I'll wait for the bot results to come back green, but I think we're there. ;)

Thanks, everyone!
-Derek

One minor question:

+++ b/core/modules/file/file.module
@@ -892,9 +901,82 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
+      $files[$i] = $result->getFile();

Would it be better to set this after the try/catch block to avoid setting it to FALSE for every caught exception?

+++ b/core/modules/file/file.module
@@ -892,9 +901,82 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
+    try {
+      $result = $file_upload_handler->handleFileUpload($uploaded_file, $validators, $destination, $replace);
+      $file = $result->getFile();
+      // If the filename has been modified, let the user know.
+      if ($result->isRenamed()) {
+        if ($result->isSecurityRename()) {
+          $message = t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);
+        }
+        else {
+          $message = t('Your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]);
+        }
+        \Drupal::messenger()->addStatus($message);
+      }
+      $files[$i] = $result->getFile();
+    }

$result->getFile() is called twice here. It's the second one I was referring to, which FWIW should just be adding $file to the array.

EDIT: OK that might be intentional, ignore me :)

I figured out the hooks fail. We are doing a file load to see if the file exists before create, so we need to add 'load' to the expected hooks.

This strikes me as odd. How come we're doing an extra load. Why is this refactoring changing the logic?

@kim.pepper I think the new way will result in additional queries to the database. Also I think in general it's not great to change behaviour in a refactor as it increases the chance of unintended impacts. If we do decide to change the behaviour then it needs to be clearly documented in a change record and in the issue summary. My preference would be to change the behaviour in a follow-up.

@kim.pepper++ looks much nicer now there is no SaveUploadFormTest SaveUploadTest changes.

+++ b/core/modules/file/src/Upload/FileUploadHandler.php
@@ -0,0 +1,336 @@
+    // Build a list of allowed extensions.
+    $extensions = '';
+    if (isset($validators['file_validate_extensions'])) {
+      if (isset($validators['file_validate_extensions'][0])) {
+        // Build the list of non-munged extensions if the caller provided them.
+        $extensions = $validators['file_validate_extensions'][0];
+      }
+      else {
+        // If 'file_validate_extensions' is set and the list is empty then the
+        // caller wants to allow any extension. In this case we have to remove the
+        // validator or else it will reject all extensions.
+        unset($validators['file_validate_extensions']);
+      }
+    }
+    else {
+      // No validator was provided, so add one using the default list.
+      // Build a default non-munged safe list for
+      // \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName().
+      $extensions = self::DEFAULT_EXTENSIONS;
+      $validators['file_validate_extensions'] = [];
+      $validators['file_validate_extensions'][0] = $extensions;
+    }
+    return $extensions;

This can be written with way less logic....

  protected function getAllowedExtensions(array &$validators): string {
    if (isset($validators['file_validate_extensions'])) {
      if (!isset($validators['file_validate_extensions'][0])) {
        // If 'file_validate_extensions' is set and the list is empty then the
        // caller wants to allow any extension. In this case we have to remove the
        // validator or else it will reject all extensions.
        unset($validators['file_validate_extensions']);
      }
    }
    else {
      // No validator was provided, so add one using the default list.
      // Build a default non-munged safe list for
      // \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::sanitizeName().
      $validators['file_validate_extensions'] = [self::DEFAULT_EXTENSIONS];
    }
    return $validators['file_validate_extensions'][0] ?? '';
  }

Also I think the docblock for this should note the side effect of adding a validator if there is not one and removing the validator if it is set but is empty. Because it is very confusing and awkward. Plus I think the method name should not be getExtensions() but that implies it is a simpler getter when it is anything but. How about handleExtensionValidation() - I'm not sure.

Thanks for all the effort in here, folks!

#65 was a good point, fixed in #66.

#71 addresses @alexpott's good points in #67 and #69.

#73 is a great suggested improvement, implemented and documented nicely in #74.

I opened #3243712: Change behavior of FileUploadHandler::handleFileUpload() to update existing entities instead of always creating new ones as a child issue for the possible follow-up to actually change the behavior and document it appropriately. No "Needs followup" needed. 😉 Might not even be worth doing, but we can discuss there.

Bot is happy. I believe everything has been addressed. I don't see room for improvement (although others still might). The draft CR looks good. Per the end of #62, I think it's okay if I RTBC this, so I will. 😀

Thanks everyone! 🙏🎉
-Derek

Committed bfe5274 and pushed to 9.3.x. Thanks!

Time to get on with #2940383: Unify file upload logic of REST and JSON:API at long last!

Also we can open a follow-up to call it directly in _file_save_upload_from_form() instead of file_save_upload and then handle all the messages there.

Time to get on with #2940383: Unify file upload logic of REST and JSON:API at long last!

💯

Also: wow, thanks everyone for the epic progress here!

Sorry folks.

While working on #92944: Display generic message and log detailed message when file upload fails due to PHP error I read the change record for this issue and noticed that it refers to method createFromUpload which does not exist. Also, should the change record mention the various file exceptions that might be thrown?

I've fixed the CR and expanded on why custom/contrib should not actually use the new service.

@quietone tbh I'm not sure that we really should have change records for changing a method that is marked as internal. Also I don't think we need to document the exceptions on the CR. They are documented on the method and a CR that repeats the documentation is not that useful. A CR is to tell people how to update their code to use the new functionality. People should not have been calling _file_save_upload_single() directly and the are not really many use-cases for calling the new service outside of core. 99.9% of the time contrib/custom should be using the managed file api and form elements and not this.

Removing tag per #81.