Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager

Some DI injections left and some unit tests are doing fun things but this deals with most things. I think this makes a lot of sense as an API, multiple places that used these functions already had the stream wrapper manager injected, so that shows IMHO that it makes sense to have them there.

Two are left that I'm not sure: file_default_scheme(), which is a simple wrapper around \Drupal::config() and file_build_uri(), which uses that. StreamWrapperManager is too deep for config, so we can't move that there. But FileSystem uses them in prepareDestination, which only works because nothing that's low-level enough calls that.

+++ b/core/tests/Drupal/KernelTests/Core/StreamWrapper/StreamWrapperManagerTest.php
@@ -0,0 +1,62 @@
+  protected function setUp() {
+    parent::setUp();
+    $this->streamWrapperManager = $this->container->get('stream_wrapper_manager');

still not 100% official, but I prefer using \Drupal::service(), I just don't see a benefit of using $this->container :)

in this case it's also a bit overkill to have that property and setup method for one method, but I guess the goal is to have more test methods here.

This should fix most of these fails. Was pondering about using isValidUri($destination) there but we need the scheme later on, so I ended up doing a mix of both :)

Some more fixes and test updates.

Thanks for the RTBC, I think it's a bit early for that though because the CR has not been actually written yet, and there is still the second paragraph of #3 to think about.

We could deal with those two functions elsewhere, but might make sense to have a plan at least.

Yes, I'd be happy to drop file_default_scheme() but it doesn't actually solve anything as file_build_uri() uses it.

I'd really prefer that the file_system service doesn't depend on configuration, that just seems like a bad idea. Config\DatabaseStorage is full of fileSystem calls, and then that again depending on config and even writing config seems too dangerous, even i it's not the actually the same methods.

There's also file_directory_temp(), see #931794: 'file_temporary_path' variable and file_directory_temp() function should match, maybe a new FileSystemConfiguration service for those, but file_build_uri() is still weird, especially since FileSystem actually calls it already. The thing is that it is only called if $destination is *empty* for copy() or move() which, to be honest, seems extremely weird. I'm considering that we should just deprecate support for that entirely, copying/moving implicitly into the default scheme seems like a *suuper* rare case. Question is whether we could push that still in 8.7, then we wouldn't need a dedicated deprecation for that, we'd just make the argument required from the start.

If we drop that, we can remove the hidden config dependency in FileSystem, same for system_retrieve_file(), and then the only remaining non-test use case of that function is \Drupal\image\Entity\ImageStyle::buildUrl(), meaning, we can possibly deprecate that without a replacement too?

I actually really like that idea, so created #3038988: Remove support for empty destination in FileSystem::copy(), FileSystem::move() and FileSystem::saveData()

The answer to #3 is #3039026: Deprecate file_directory_temp() and move to FileSystem service and #3038988: Remove support for empty destination in FileSystem::copy(), FileSystem::move() and FileSystem::saveData(), we move those two into a separate service and remove the dependency on file_build_uri() in FileSystem.

Did a reroll, which git apply -3 resolved automatically, lets see if that worked.

Also started working on an issue summary and change record update. Shows that we still have some work to do in regards to making the new methods more consistent. I kept the names from FileSystem, but I think it would make more sense to prefix them with is/get or another verb (isValidScheme()? validateScheme()? getUriScheme()? getScheme()?)

+++ b/core/includes/file.inc
@@ -98,25 +98,27 @@
 function file_uri_scheme($uri) {
-  return \Drupal::service('file_system')->uriScheme($uri);
+  @trigger_error('file_uri_scheme() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface::getScheme(). See https://www.drupal.org/node/3035273.', E_USER_DEPRECATED);
+  return \Drupal::service('stream_wrapper_manager')->getScheme($uri);

The deprecation message here is a bit challenging. file_uri_scheme() *is* deprecated since 8.0.0, but the replacement is changing in 8.8, so we should probably say that? Easiest "fix" might be to just update it to say deprecated in 8.8, so that people know they can't use the stream wrapper manager replacement before that?

Either way, most messages will need to be updated to 8.8 and I think the message needs some updates for the new standard, e.g. it should AFAIK be "drupal:8.8.0" now.

+++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
@@ -146,4 +146,39 @@ public function testDeprecatedDrupalTempnam() {
+  /**
+   * @expectedDeprecation file_uri_scheme() is deprecated in drupal:8.0.0 and will be removed before drupal:9.0.0. Use \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface::getScheme() instead. See https://www.drupal.org/node/3035273.
+   */
+  public function testDeprecatedFileUriScheme() {
+    $this->assertNotNull(file_uri_scheme(''));
+  }

We might need to be a bit more specific with those kernel tests.

The problem is that this would still pass if the function would return an incorrect but non-null result. A legacy test has two goals:

a) Assert that the expected deprecation message is thrown.
b) Assert that it still works correctly.

b is only partially tested right now. See for example the copy/move test methods that I updated in that test.

+++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
@@ -147,38 +147,38 @@ public function testDeprecatedDrupalTempnam() {
   /**
-   * @expectedDeprecation file_stream_wrapper_uri_normalize() is deprecated in drupal:8.8.0 and will be removed before drupal:9.0.0. Use \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface::normalizeUri() instead. See https://www.drupal.org/node/3035273.
+   * @expectedDeprecation file_stream_wrapper_uri_normalize() is deprecated in Drupal:8.8.0 and will be removed before Drupal:9.0.0. Use \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface::normalizeUri() instead. See https://www.drupal.org/node/3035273.

why the case change in those messages?

It is "drupal", according to the examples in https://www.drupal.org/project/coding_standards/issues/3024461, also for @deprecated.

4. We usually don't test constructor deprecations unless it's something special, I did dozens of these part of my entity manager issues.

Conflicted with #2802803: Temporary files whose files are missing on the disk result in never-ending error log messages

Good question, I'm not sure. Happy to make some of these static, I assume they would still live on StreamWrapperManager? Note that it doens't really change that much. most calls that we have to one of these is then followed by another method call that is not static, typically something like this:

$scheme = $this->streamWrapperManager->getScheme($directory);
if ($this->streamWrapperManager->isValidScheme($scheme)) {

And leaving uriScheme() on FileSystem also doesn't make sense to me when all the other related functions are on StreamWrapperManager, including its companion getUriTarget().

1. and 3. is the same function I think, just old and new location.
2. getUriTarget(), yup, that could be static.
4. This one actually calls $this->getClass(), so no candidate.

So that means we would have 2 methods that could be static, getScheme() and getUriTarget().

I'm not sure how a FileUri value object would work. These are URI's that we're passing around to file_system and php functions, they need a string. And several things, like isValidScheme() need access to the registered stream wrappers and with that the manager.

Maybe a helper class FileUrl (..Validator|Parser|...) with static methods but the only thing we can IMHO move are getScheme() and getUriTarget() and not sure that's worth it.

Setting back to RTBC to get feedback from @alexpott/@larowlan.

As discussed with @alexpott in slack, we'll heave the methods on the interface/class and just make it static.

1. +++ b/core/includes/file.inc
   @@ -369,7 +369,7 @@ function file_save_htaccess($directory, $private = TRUE, $force_overwrite = FALS
    
   -  if ($stream_wrapper_manager->getScheme($directory)) {
   +  if ($stream_wrapper_manager::getScheme($directory)) {
        $htaccess_path = $stream_wrapper_manager->normalizeUri($directory . '/.htaccess');
      }
   

   hm, that's an interesting pattern. Not sure, but I'd either keep it as -> or with the classname.
   Rest of the patch is a mix of both. I'd wait for some more feedback on that.

   Discussed a bit on slack and thought about it a bit more. There's a good amount of existing examples in core and it helps to avoid an extra use statement in most cases, so $stream_wrapper_manager:: if it's needed as an object, the classname otherwise seems fine.

2. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -125,7 +126,7 @@ public function loadFile($file, $optimize = NULL, $reset_basepath = TRUE) {
        // Stylesheets are relative one to each other. Start by adding a base path
        // prefix provided by the parent stylesheet (if necessary).
   -    if ($basepath && !$this->streamWrapperManager->getScheme($file)) {
   +    if ($basepath && !StreamWrapperManager::getScheme($file)) {
          $file = $basepath . '/' . $file;
   

   Looks like this was the only usage of this in CssOptimizer, so we can drop the constructor again, yay.

Great, back to RTBC as #69 has been addressed.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManager.php
   @@ -100,7 +100,7 @@ public function getViaScheme($scheme) {
       */
      public function getViaUri($uri) {
   -    $scheme = file_uri_scheme($uri);
   +    $scheme = $this->getScheme($uri);
        return $this->getWrapper($scheme, $uri);
      }
   

   this could be a static::getScheme(), not sure if it makes a difference.

2. +++ b/core/lib/Drupal/Core/StreamWrapper/StreamWrapperManagerInterface.php
   @@ -169,4 +169,81 @@ public function getClass($scheme);
   +   *
   +   * @param $uri
   +   *   The URI to be tested.
   +   *
   +   * @return bool
   +   *   TRUE if the URI is allowed.
   

   nitpick: missing type and description of return seems off, "allowed" for what? should be valid I'd say

3. +++ b/core/modules/image/image.module
   @@ -471,8 +473,11 @@ function image_field_config_update(FieldConfigInterface $field) {
    
   +  /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
   +  $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
   +
      // If the upload destination changed, then move the file.
   -  if ($file_new && (file_uri_scheme($file_new->getFileUri()) != $field_storage->getSetting('uri_scheme'))) {
   +  if ($file_new && ($stream_wrapper_manager::getScheme($file_new->getFileUri()) != $field_storage->getSetting('uri_scheme'))) {
        $directory = $field_storage->getSetting('uri_scheme') . '://default_images/';
   

   another one where the static call is the only call, so no service needed?

4. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -48,8 +49,11 @@ class ImageStyleDownloadController extends FileDownloadController {
       */
   -  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory) {
   +  public function __construct(LockBackendInterface $lock, ImageFactory $image_factory, StreamWrapperManagerInterface $stream_wrapper_manager) {
   +    parent::__construct($stream_wrapper_manager);
        $this->lock = $lock;
   

   needs to be = NULL here too, so that subclasses of ImageStyleDownloadController still work as well.

5. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -91,6 +96,9 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
    
   +    /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
   +    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
   +
        // Check that the style is defined, the scheme is valid, and the image
        // derivative token is valid. Sites which require image derivatives to be
        // generated without a token can set the
   @@ -102,7 +110,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   
   @@ -102,7 +110,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
        // The $target variable for a derivative of a style has
        // styles/<style_name>/... as structure, so we check if the $target variable
        // starts with styles/.
   -    $valid = !empty($image_style) && file_stream_wrapper_valid_scheme($scheme);
   +    $valid = !empty($image_style) && $stream_wrapper_manager->isValidScheme($scheme);
   

   we are injecting the stream wrapper manager here, so can use $this->streamWrapperManager

6. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -505,16 +510,12 @@ protected function addExtension($path) {
      protected function fileUriScheme($uri) {
   -    return file_uri_scheme($uri);
   +    return StreamWrapperManager::getScheme($uri);
      }
   
   @@ -525,17 +526,13 @@ protected function fileUriScheme($uri) {
       */
      protected function fileUriTarget($uri) {
   -    return file_uri_target($uri);
   +    return StreamWrapperManager::getUriTarget($uri);
      }
   

   now that these two methods are static, can we just call them directly in the unit test? looks like that duplicates the real implementation? Then we can add a @trigger_error() to these too.

7. +++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
   @@ -451,7 +451,11 @@ public function testImageFieldDefaultImage() {
   +
   +    /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
   +    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
   +
   +    $this->assertEqual('private', $stream_wrapper_manager->getScheme($file->getFileUri()), 'Default image uses private:// scheme.');
        $this->assertTrue($file->isPermanent(), 'The default image status is permanent.');
   

   another static call?

8. +++ b/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php
   @@ -184,12 +184,15 @@ public function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_s
    
   +    /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
   +    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
   +
        // Check that the generated URL is the same when we pass in a relative path
        // rather than a URI. We need to temporarily switch the default scheme to
        // match the desired scheme before testing this, then switch it back to the
        // "temporary" scheme used throughout this test afterwards.
        $this->config('system.file')->set('default_scheme', $scheme)->save();
   -    $relative_path = file_uri_target($original_uri);
   +    $relative_path = $stream_wrapper_manager::getUriTarget($original_uri);
   

   doesn't seem to use the service elsewhere?

9. +++ b/core/modules/image/tests/src/Kernel/ImageStyleCustomStreamWrappersTest.php
   @@ -75,7 +75,7 @@ public function register(ContainerBuilder $container) {
      public function testCustomStreamWrappers($source_scheme, $expected_scheme) {
        $derivative_uri = $this->imageStyle->buildUri("$source_scheme://some/path/image.png");
   -    $derivative_scheme = $this->fileSystem->uriScheme($derivative_uri);
   +    $derivative_scheme = \Drupal::service('stream_wrapper_manager')->getScheme($derivative_uri);
    
   

   same.

10. +++ b/core/modules/locale/locale.translation.inc
    @@ -376,7 +376,9 @@ function locale_cron_fill_queue() {
     function _locale_translation_file_is_remote($uri) {
    -  $scheme = file_uri_scheme($uri);
    +  /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
    +  $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
    +  $scheme = $stream_wrapper_manager->getScheme($uri);
    

    same.

11. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -295,8 +296,9 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
    +
    +        if (StreamWrapperManager::getScheme($original_path) == 'public') {
    +          $friendly_path = StreamWrapperManager::getUriTarget($original_path);
               $element['#default_value'] = $friendly_path;
             }
    

    seeing them next to each other made me realize that one has Uri and the other doesn't, seems a bit inconsistent but not sure if that's an issue and which way we should make them consistent.

12. +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
    @@ -63,9 +63,12 @@ public function testCreateDeletePicture() {
     
    +    /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
    +    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
    +
         // Verify that the image is displayed on the user account page.
         $this->drupalGet('user');
    -    $this->assertRaw(file_uri_target($file->getFileUri()), 'User picture found on user account page.');
    +    $this->assertRaw($stream_wrapper_manager::getUriTarget($file->getFileUri()), 'User picture found on user account page.');
     
    
    @@ -135,8 +138,11 @@ public function testPictureOnNodeComment() {
           ->save();
     
    +    /** @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface $stream_wrapper_manager */
    +    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
    +
         $this->drupalGet('node/' . $node->id());
    -    $this->assertNoRaw(file_uri_target($file->getFileUri()), 'User picture not found on node and comment.');
    +    $this->assertNoRaw($stream_wrapper_manager::getUriTarget($file->getFileUri()), 'User picture not found on node and comment.');
       }
    

    more static only calls.

Looks good to me now, nice cleanup in that interdiff.

@alexpottt might have some thoughts on that method name in #79.11, I think otherwise it is ready.

Works for me, thanks.

Fun :-/ I'd suggest you open a follow-up, we can do the same thing we did before in other places, add a private getFileSystem() method that does another @trigger_error() and falls back to Drupal::service() if it's not set.