deprecate system_retrieve_file() without replacement

1. +++ b/core/lib/Drupal/Core/File/FileFetcher.php
   @@ -0,0 +1,103 @@
   +    if (!isset($destination)) {
   +      $path = $this->configFactory->get('system.file')->get('default_scheme') . '://' . $filename;
   +    }
   

   I'm unsure if we really want to support destination being optional.

   This puts files in the root public:// folder, which I think is a bad practice. You should at least bother to create a directory for your module somewhere. It's also the only case we use the default scheme, so if we require destination, we could drop that.

   There are no non-test calls in core that don't pass a destination and I also can't see any examples on the first page of http://grep.xnddx.ru/search?text=system_retrieve_file.

   I'd vote to drop that feature.

2. +++ b/core/lib/Drupal/Core/File/FileFetcher.php
   @@ -0,0 +1,103 @@
   +    else {
   +      if (is_dir($this->fileSystem->realpath($destination))) {
   +        // Prevent URIs with triple slashes when glueing parts together.
   +        $path = str_replace('///', '//', "$destination/") . $filename;
   +      }
   

   I've been wondering about this line before, why it's specifically in this function and nowhere else. Tracked it down to #573300: system_retrieve_file() fails in file_unmanaged_copy() (invocation with path instead of URI), which was 11 years ago, but can't see the reason it was added.

   I do like that Dries already pointed this out back then:

   > I find it odd that an API function uses drupal_set_message() but that is probably best left for another issue.

   11 years later, we finally have that issue ;)

3. +++ b/core/lib/Drupal/Core/File/FileFetcher.php
   @@ -0,0 +1,103 @@
   +  protected function saveData(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME) {
   +    return $this->fileSystem->saveData($data, $destination, $replace);
   

   what's the reason for wrapping fileSystem->saveData()? Seems like that requires to copy a lot of documentation for a single line of code?

4. +++ b/core/modules/file/src/ManagedFileFetcher.php
   @@ -0,0 +1,20 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function saveData(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME) {
   +    return file_save_data($data, $destination, $replace);
   +  }
   

   Ah, that's why.

   Wondering something similar as with the empty destination. How often is this used and is there a better way.

   Having two services do something different with the same interface does seem a bit unusual too. And the interface forces a string return, but file_save_data() would return an entity, is that code path untested now?

   the deduplication logic in file_save_data() is definitely useful. Maybe the replacement should just be about creating a file entity from an already existing local file? then we only need one service, you can use retrieve file and create the file entity yourself with a single extra call.

   FWIW, not sure if there's much left here that's actually worth putting in a service then, with no error handling, you do need to duplicate that then.

1. +++ b/core/core.services.yml
   @@ -395,6 +395,9 @@ services:
   +  file.fetcher.unmanaged:
   +    class: Drupal\Core\File\FileFetcher
   

   I get why the services are named: file.fetcher.unmanaged and file.fetcher.managed. Only I am not happy with the new core service name. The first part "file." implies that it is part of the file module. Only it is not. It is part of core. My second objection is the ".unmanaged" part. The class name is FileFetcher, not FileFetcherUnmanaged. Or the class name is not correct.

2. +++ b/core/lib/Drupal/Core/File/FileFetcherInterface.php
   @@ -0,0 +1,39 @@
   +   * @param string|null $destination
   

   Missing the part that the parameter is optional and what its default value is.

3. +++ b/core/lib/Drupal/Core/File/FileFetcherInterface.php
   @@ -0,0 +1,39 @@
   +   * @param int $replace
   

   Missing the part that the parameter is optional and what its default value is.

4. +++ b/core/modules/system/system.module
   @@ -604,7 +604,7 @@ function system_page_attachments(array &$page) {
   -  list($version,) = explode('.', \Drupal::VERSION);
   +  [$version] = explode('.', \Drupal::VERSION);
   

   Out of scope change.

5. +++ b/core/lib/Drupal/Core/File/FileFetcher.php
   @@ -0,0 +1,103 @@
   +   * @return string
   +   *   A string with the path of the resulting file, or FALSE on error.
   

   Should it not be "@return string|false"

6. +++ b/core/lib/Drupal/Core/File/FileFetcher.php
   @@ -0,0 +1,103 @@
   +  public function fetch(string $url, ?string $destination = NULL, int $replace = FileSystemInterface::EXISTS_RENAME): string {
   ...
   +    return $this->saveData($data, $path, $replace);
   
   +++ b/core/lib/Drupal/Core/File/FileFetcherInterface.php
   @@ -0,0 +1,39 @@
   +   * @return string
   +   *   The path to the local file.
   ...
   +  public function fetch(string $url, ?string $destination = NULL, int $replace = FileSystemInterface::EXISTS_RENAME): string;
   

   The method saveData() can return false, therfor the method fetch can also return false.

7. +++ b/core/modules/file/src/ManagedFileFetcher.php
   @@ -0,0 +1,20 @@
   +class ManagedFileFetcher extends FileFetcher {
   

   Can we change the name to FileFetcherManaged.

I have worked in this issue and I am submitting my patch. Please review it.

Patch is failing testbot.

Adding a patch from #10 fixing the phpcs errors reported.

Agree with postponing on #3223209: deprecate file_save_data, file_copy and file_move and replace with a service, we should work bottom up here.

Also, the last two patches seem to just move the function around, something went wrong there. Ignore those.

+1 to postpone, meantime the fetcher service is http(s):// stream it just need to read data

+++ b/core/modules/system/system.module
@@ -1169,30 +1169,20 @@ function system_time_zones($blank = NULL, $grouped = FALSE) {
+ * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use
...
+  @trigger_error(__FUNCTION__ . ' is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\Core\File\FileFetcherInterface::fetch() instead. See https://www.drupal.org/node/3223362', E_USER_DEPRECATED);

+++ b/core/modules/system/tests/src/Kernel/SystemDeprecationTest.php
@@ -0,0 +1,42 @@
+    $this->expectDeprecation('system_retrieve_file is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\Core\File\FileFetcherInterface::fetch() instead. See https://www.drupal.org/node/3223362');

needs re-roll

Do we need to take care of error handling in these replacements? system_retrieve_file() converted any exceptions to messages.

Sure, just messages is strange, but not catching them means we'll break update and translation import processes, I've definitely seen error messages around translation imports in the past. As an Api function, adding messages was weird, but in this specific cases, that might be fine?

And #2177545: Change system_retrieve_file function to return error instead of generating Drupal messages will be a won't fix once the function is deprecated but would result in exactly the same as we have to do here. Returning an error means callers need to make the decision on how to handle the error, just like here.

Can we add test coverage for the new errors being logged here?

+    catch (TransferException $exception) {
+      \Drupal::messenger()->addError(t('Failed to fetch file due to error "%error"', ['%error' => $exception->getMessage()]));
+    }
+    catch (FileException | InvalidStreamWrapperException $e) {
+      \Drupal::messenger()->addError(t('Failed to save file due to error "%error"', ['%error' => $e->getMessage()]));
+    }

Ah in that case think this is good to go then!

random

Are we sure the test fail here is random .. its to do with a locale file it seems?

Re queued tests

I reviewed the patch with @kim.pepper at DrupalSouth the changes look good so setting it back to RTBC.

No longer applies, can we get a reroll please 🙏

Just a reroll.

Issue credit

Committed 8219cb1 and pushed to 11.x. Thanks!

Published the change record