Use file_system service to \Drupal\Core\Config\FileStorage

Looks great! Only those deprecated methods are used in that file, so that means this is RTBC.

This is not injecting the service. If we want to do that then we need to do quite a bit more work to add it to the constructor of FileStorage and InstallStorage and ExtensionInstallStorage.

As these classes are used everywhere the default should be NULL but we should automatically deprecate this and mandate it in Drupal 9.

Yes, injecting through the constructor is not an option. What if we just add a setter method so if someone really wants to do unit tests, he can, similar to string translation trait, messenger and so on. That said, not quite sure how you'd do unit tests of this anyway, because it clearly is very file/storage related, you can't mock the remaining low-level php functions and so on anyway.

There are almost 40 variants of new FileStorage() in core, so updating them all would be a ton of work, which is why I didn't deprecate not passing one in.

Interdiff is against #2.

blocker for #3021434: Properly deprecate drupal_unlink()

+1 for rtbc

+++ b/core/core.services.yml
@@ -321,6 +321,8 @@ services:
     factory: Drupal\Core\Config\FileStorageFactory::getSync
+    calls:
+      - [setFileSystem, ['@file_system']]

How about making the factory container aware, using \Drupal\Core\DependencyInjection\ContainerInjectionInterface, and then injecting the service via constructor injection? Yes we'd then have to do more work with the other storages but we'd achieve the aims of having the file system service injected everywhere.

I discussed this a bit with alexpott in slack:

berdir: there are 2 problems
alexpott: Only 2?
berdir: I'm grouping them together
berdir: a) FileStorage has 4 subclasses, 30+ versions of new FileStorage() all over the place and optional constructor arguments (the collection)
berdir: that's all solvable, but it is a *lot* of work
berdir: \Drupal\Core\Config\ConfigInstaller alone creates file storage objects in 5 places
berdir: which means we'd also need to inject it through those places, so inject into ConfigInstaller, so that it i can inject it further. And the optional constructor argument makes it all a bit awkward IMHO
alexpott: Yeah well I guess this makes me ponder why we’re borthering (edited)
berdir: and ExtensionInstallStorage has 5 optional arguments
berdir: b) is that we end up in a circular reference if we try to do that. See test fails in #10: Circular reference detected for service "config.factory", path: "language_request_subscriber -> language_manager -> config.factory -> config.typed -> config.storage.schema -> file_system -> logger.channel.file -> logger.factory -> logger.syslog".
alexpott: Nice
alexpott: We really should never be injecting loggers… they should be listening to events - but that’s a separate issue.
alexpott: Is there a burning reason why we don’t convert the calls to \Drupal::service() - yes it is ugly - but in reality this is what is happening anyway
berdir: yeah, in case of FileSystem, we also discussed if it shouldn't log bug only throw exceptions, leaving the logging to the parent.. which would allow more useful log messages than the ones we currently have _if_ those implementations would bother to do that, and I'm sure most wouldn't.. :wink:
berdir: I don't think there is a burning reason, FileStorage isn't very unit-testable anyway, as it has a lot of direct IO calls itself too
berdir: if you're OK with that then I'm too. I modeled this after things like StringTranslationTrait, which allows optional injection. but as I said, we're most likely not going to write unit tests for this thing any time soon
alexpott: I think we should admit the awfulness of \Drupal::service() and move on.
berdir: not even hidden behind a getFileSystem() method? there are 4 or 5 calls to its methods
alexpott: It could be - but it should be private so if we ever get to injection we can remove it without thinking too hard
berdir: sounds good

So..
* Change the method to private, remove setFileSystem(), just do return \Drupal::service()
* update the issue title/summary to reflect that we don't inject but just use the serice

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -33,6 +33,13 @@ class FileStorage implements StorageInterface {
+  /**
+   * The file system.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  private $fileSystem;

I'm not sure that the property is worth it.

Lets do this so we can finalize the deprecation of those functions.

I pushed the deprecation patch first because it's doing something more important - this needs a reroll.

And back to RTBC.

dispatcher/testbot issue. back to rtbc

Committed and pushed 2348afa46a to 8.8.x and 321bbf9658 to 8.7.x. Thanks!