deprecate system_retrieve_file() without replacement

The issue this was postponed on was committed in Drupal 9.3.x.

+++ 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

As mentioned in #16 I think we should deprecate without replacement.

I can only see 2 places in core where this gets used, and both of them just need to use the unmanaged file system to write the file.

Format deprecation message correctly.

The last submitted patch, 23: 3223205-23.patch, failed testing. View results →

locale_translation_download_source expects the file to be saved with the same name as the fetched URL file.

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

There is 📌 Change system_retrieve_file function to return error instead of generating Drupal messages Closed: outdated linked in the IS. Do we want to document them, and just let the exceptions bubble up? Personally, I think it's strange to set messages at this level.

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 📌 Change system_retrieve_file function to return error instead of generating Drupal messages Closed: outdated 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.

> Returning an error means callers need to make the decision on how to handle the error, just like here.

But it should be handled further up that call stack. That's where you can potentially do something more useful with more context.

Copied over setting messages if exceptions are caught in the two locations where this function has been replaced.

Updated the title and IS to reflect the current solution.

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()]));
+    }

These are not new errors being logged. We're copying the behaviour of system_retrieve_file() which is now deprecated.

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

The last submitted patch, 31: 3223205-31.patch, failed testing. View results →

random

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

Re queued tests

Rebase on 11.x and updated deprecation message with 10.2.0 version.

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

Automatically closed - issue fixed for 2 weeks with no activity.