Deprecate file_default_scheme() and just use config

+++ b/core/modules/editor/tests/src/Functional/EditorLoadingTest.php
@@ -114,7 +114,7 @@ public function testLoading() {
       'image_upload' => [
         'status' => FALSE,
-        'scheme' => file_default_scheme(),
+        'scheme' => $this->config('system.file')->get('default_scheme'),
         'directory' => 'inline-images',
         'max_size' => '',

As discussed in slack, I would actually suggest to update all usages in tests that are not specifically testing the configurable default to just hardcode public, we *know* that's the value it has and we have dozens if not hundreds of hardcoded public:// references already in tests. That will also better show how common it really is to use that configuration (and hopefully that it is indeed OK to just access the configuration).

And we also need a legacy test I think.

1. +++ b/core/modules/file/src/Tests/FileManagedTestBase.php
   @@ -189,7 +189,7 @@ public function createUri($filepath = NULL, $contents = NULL, $scheme = NULL) {
        if (!isset($scheme)) {
   -      $scheme = file_default_scheme();
   +      $scheme = 'public';
        }
   

   this is the old deprecated base test, not sure if we should touch that one at all..

2. +++ b/core/modules/file/tests/src/Functional/FileManagedTestBase.php
   @@ -184,7 +184,7 @@ public function createUri($filepath = NULL, $contents = NULL, $scheme = NULL) {
        }
        if (!isset($scheme)) {
   -      $scheme = file_default_scheme();
   +      $scheme = 'public';
        }
   

   this *could* maybe be a problem as it is a base class, so subclasses might rely on it, we'll see.

3. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -549,7 +549,7 @@ protected function fileUriTarget($uri) {
       */
      protected function fileDefaultScheme() {
   -    return file_default_scheme();
   +    return \Drupal::config('system.file')->get('default_scheme');
      }
   

   the docblock here still mentiones the function, with todos to convert it to a service.

   Also, we could actually deprecate this function instead of updating it, call \Drupal::config() directly and mock it through the container?

4. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -92,12 +93,12 @@ public function testFileCheckDirectoryHandling() {
        // Remove .htaccess file to then test that it gets re-created.
   -    @drupal_unlink(file_default_scheme() . '://.htaccess');
   -    $this->assertFalse(is_file(file_default_scheme() . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File');
   +    @drupal_unlink($default_scheme . '://.htaccess');
   +    $this->assertFalse(is_file($default_scheme . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File');
   

   aww, this will conflict with the RTBC drupal_unlink() issue, so will hold back on RTBC until that one is in. Not sure if updating that already now will merge it easier to rebase or not.

5. +++ b/core/tests/Drupal/KernelTests/Core/File/FileDeleteTest.php
   @@ -28,7 +28,7 @@ public function testNormal() {
        // Try to delete a non-existing file
   -    $this->assertTrue(\Drupal::service('file_system')->delete(file_default_scheme() . '/' . $this->randomMachineName()), 'Returns true when deleting a non-existent file.');
   +    $this->assertTrue(\Drupal::service('file_system')->delete('public/' . $this->randomMachineName()), 'Returns true when deleting a non-existent file.');
      }
   

   this looks completely bogus, which it was before as well, but I don't think this is what it should do.

> I think this is pretty tricky to try and mock \Drupal::config() here.

Should be possible by providing a mock through a container like other unit tests, but yeah, this is out of scope here I think.

> Not sure what else to do here except replace 'public/' with 'public://'

Yeah, that's what I meant. It does test a non-existing file, but I think that's what it should have been and feels like a more realistic test than trying to delete DRUPAL_ROOT/public/...

Looks good to me I think, lets see what the committers think about this suggestion.

Maybe the issue summary could be improved a bit and e.g. point out that we removed the configurability from a lot of tests where we just know that it is public://. I quick search shows around 450 existing usages of "'public://" in tests, so these are already the exception.

Random fail maybe, saw a few random build successful problems recently.

issue credits

Committed 31a5cba and pushed to 8.8.x. Thanks!

published change record

Think we need a followup here:

  @trigger_error('file_default_scheme() is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Use \Drupal::config(\'system.file\')->get(\'default_scheme\') instead. See https://www.drupal.org/project/paragraphs/issues/3049030', E_USER_DEPRECATED);

It's supposed to be: https://www.drupal.org/node/3049030 instead.