Move file_scan_directory() to file_system service

As it turned out in #1451320: Evaluate Symfony2's Finder Component to simplify file handling, file_scan_directory() is far more better in terms of performance. That we should preserve. But lacks DX. What if we create a component that is using file_scan_directory() implementation but on Finder interface so we’re compatible with the Symfony2 component and at some point, if they are solving their perfomance debt, we can easily drop our code but still keep the contract?

+1 to trying #2.

As 8.8 branch is open - not sure what we should have in deprecation message

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
@@ -196,7 +196,7 @@ public function deleteAll() {
         $this->fileSystem->delete($uri);
...
-    file_scan_directory('public://css', '/.*/', ['callback' => $delete_stale]);
+    \Drupal::service('file_system')->scanDirectory('public://css', '/.*/', ['callback' => $delete_stale]);

+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
@@ -198,7 +198,7 @@ public function deleteAll() {
         $this->fileSystem->delete($uri);
...
-    file_scan_directory('public://js', '/.*/', ['callback' => $delete_stale]);
+    \Drupal::service('file_system')->scanDirectory('public://js', '/.*/', ['callback' => $delete_stale]);

both services has fileSystem already injected

Looks it should be 8.7 for a while!

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -634,4 +634,82 @@ public function createFilename($basename, $directory) {
+      $dir = file_stream_wrapper_uri_normalize($dir);

I bet we should use the service methods instead, maybe convert it same time?

Related to 10, the problem here is that service should not use function from include which using the service and calls already deprecated stuff

Other then a minor question patch looks great. Thanks for working on it.

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -117,4 +128,17 @@ public static function filesToArray($langcode, array $files) {
+  private function getFileSystem() {
+    if (!isset($this->fileSystem)) {
+      $this->fileSystem = \Drupal::service('file_system');
+    }
+    return $this->fileSystem;

Why are we doing this here and not in constructor?

1. +++ b/core/includes/file.inc
   @@ -968,80 +967,15 @@ function file_unmanaged_save_data($data, $destination = NULL, $replace = FILE_EX
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.
   

   It's 8.8.x now. :-)

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -634,4 +634,99 @@ public function createFilename($basename, $directory) {
   +   * @todo Remove when https://www.drupal.org/node/3034072 is in.
   

   #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager still waiting...

3. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
   @@ -117,4 +128,17 @@ public static function filesToArray($langcode, array $files) {
   +      $this->fileSystem = \Drupal::service('file_system');
   
   +++ b/core/tests/Drupal/KernelTests/Core/File/ScanDirectoryTest.php
   @@ -3,7 +3,7 @@
   + * Tests \Drupal\Core\File\FileSystemInterface::scanDirectory().
   

   Use @coversDefaultClass and/or @covers. Also, it doesn't test the interface.

Diggit.

Fixing the test

Mostly looks great, thanks. Don't want to NW over this, but:

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
@@ -22,15 +23,25 @@ class FileTranslation extends StaticTranslation {
-  public function __construct($directory) {
+  public function __construct($directory, FileSystemInterface $file_system = NULL) {
     parent::__construct();
     $this->directory = $directory;
+    $this->fileSystem = $file_system;
   }

@@ -117,4 +128,17 @@ public static function filesToArray($langcode, array $files) {
+    if (!isset($this->fileSystem)) {
+      $this->fileSystem = \Drupal::service('file_system');
+    }

Generally don't we trigger a deprecation warning if the new arg isn't passed and we have to fall back to a default of NULL triggering \Drupal::service()?

I usually see that directly in the constructor, instead of needing the whole private getFileSystem() method.

I don’t know what we gain in general by writing “deprecated in Drupal 8.7.0“ with a specific version (8.7.0). It will be deprecated whenever the deprecation is released, right?

Don’t change this patch because of this! But if anyone agrees with me we should open an issue to discuss.

The version number is crucial for contrib, because when you get deprecation messages, you need to know if you can actually act on them. For example, right now, it is too early to change code so it works only on 8.7, but in 6 month, you can add a version dependency on that and update your calls.

Also, it's a coding standard, see #3024461: Adopt consistent deprecation format for core and contrib deprecation messages.

@kim.pepper re: #31: Ok, makes sense. Thanks for the explanation. However, it seems we still want a deprecation warning, no?

Re #33: Thanks and ouch: 1,567 fails... yikes. ;) I haven't tried to look into why this is failing so much. But, seems like a good thing we added the warning, since it's showing us that the patch isn't complete and something getting called frequently hasn't been updated as needed.

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +636,99 @@ public function createFilename($basename, $directory) {
   +   * @see https://www.drupal.org/node/3034072
   +   */
   +  private function normalizeUri($uri) {
   +    return file_stream_wrapper_uri_normalize($uri);
   +  }
   

   Why do we need this? Can't we just call the function until it becomes deprecated?

2. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
   @@ -117,4 +128,18 @@ public static function filesToArray($langcode, array $files) {
   +   */
   +  private function getFileSystem() {
   +    if (!isset($this->fileSystem)) {
   +      @trigger_error('Calling FileTranslation::__construct() without the $file_system argument is deprecated in drupal:8.8.0. The $file_system argument will be required in drupal:9.0.0. See https://www.drupal.org/node/3038437', E_USER_DEPRECATED);
   +      $this->fileSystem = \Drupal::service('file_system');
   +    }
   +    return $this->fileSystem;
   +  }
   

   I don't understand the pattern here. We are we kinda injecting it but then still have a private method and deprecate it in there?

   It has an existing constructor, so why don't we just put the deprecation message in there? Or we say we don't require it, but then we shouldn't deprecate it.

1. +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
   @@ -462,4 +462,43 @@ public function createFilename($basename, $directory);
   +   * @param int $depth
   +   *   The current depth of recursion. This parameter is only used internally
   +   *   and should not be passed in.
   ...
   +  public function scanDirectory($dir, $mask, array $options = [], $depth = 0);
   

   The interface is a public contract. Parameters that are revealing an implementation detail ($depth) should not be on the public contract. We should remove it and rework the implementation, probably by wrapping a protected helper method: doScanDirectory().

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +636,82 @@ public function createFilename($basename, $directory) {
   +        $this->logger->error('@dir can not be opened', ['@dir' => $dir]);
   

   Log or throw an exception? Have no idea if we can touch this but exception sounds better for me. Then we provide the BC in file_scan_directory() by catching the exception and logging. I think this would needs sub-system maintainer advice.

3. +++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php
   @@ -22,15 +23,25 @@ class FileTranslation extends StaticTranslation {
   +  public function __construct($directory, FileSystemInterface $file_system = NULL) {
   ...
   +    $this->fileSystem = $file_system;
   
   @@ -117,4 +128,17 @@ public static function filesToArray($langcode, array $files) {
   +  /**
   +   * Returns file system service.
   +   *
   +   * @return \Drupal\Core\File\FileSystemInterface
   +   *   The file system service.
   +   */
   +  private function getFileSystem() {
   +    if (!isset($this->fileSystem)) {
   +      $this->fileSystem = \Drupal::service('file_system');
   +    }
   +    return $this->fileSystem;
   +  }
   

   In see the BC is supported in signature but we have also to raise the red flag, i.e @trigger_error(). #37.2 was correct. We should drop the method and add the deprecation in the constructor, as usual.

4. The deprecated function is still mentioned in a docblock, in default.settings.php
5. The CR branch needs to be updated to 8.8.x.

More...

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -636,4 +636,82 @@ public function createFilename($basename, $directory) {
+    if (is_dir($dir)) {

So... this is also weird and for sure it's a bug. If the directory doesn't exist or $dir points to a file, rather than a directory, the method will still return an empty array ([]), same as in the case when is scanning an existing, empty directory. This is misleading. I think we have to throw an exception. Some could argue that this is off-topic and should be moved to a followup. But as this is a new service method, I think we should fix it here.

Re: #39.3 and #37.2: I raised this is in #28. See @kim.pepper's answer in #31:

The reason for the getFileSystem() is to lazily load the file system service. If we do it in the constructor, Symfony dependency injection detects a circular dependency and errors out.

Not sure I totally buy it, but that's the story.

> FileTranslation is a special case where it is a value object that gets instantiated for a specific directory, but also calls a service internally.

I don't think it's really a value object, it *is* service, but it's only dynamically registered in install_begin_request() for example. I think it would be possible to inject the file_system there. But I'm also open to try that in a follow-up issue.

After a visual scan of the patch ... this looks like a good improvement.

a) The drive behind the issue is sound.
b) The implementation is good.

I have only a minor thing

The Drupal standard as encoded in phpcs now places a tight restraint on the text format of deprecation text.

drupal:8.7.0 (without capitalization ) is the main change needed. Here is a single example of the warnings generated by the patch.

507 | ERROR | [ ] The deprecation text '@deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0. Use
| | \Drupal\Core\File\FileSystemInterface::getDestinationFilename() instead.' does not match the standard
| | format: @deprecated in %in-version% and will be removed from %removal-version%. %extra-info%.

drupal:version is IMHO fine, I think the problem with the message is before => from.

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,120 @@ public function createFilename($basename, $directory) {
   +    // Merge in defaults.
   +    $options += [
   +      'callback' => 0,
   +      'recurse' => TRUE,
   +      'key' => 'uri',
   +      'min_depth' => 0,
   +    ];
   ...
   +    $options['key'] = in_array($options['key'], ['uri', 'filename', 'name']) ? $options['key'] : 'uri';
   

   It doesn't make sense to merge defaults and normalize $options['key'] on each recursion. We can move them in the main public method so that they run only once.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,120 @@ public function createFilename($basename, $directory) {
   +    if ($depth == 0) {
   +      $dir = file_stream_wrapper_uri_normalize($dir);
   +      $dir_has_slash = (substr($dir, -1) === '/');
   +    }
   

   This also can be moved in the public method to run only once. I'm also confused about the logic related to $dir_has_slash. Why do we need that? Wouldn't it be more simple to remove the trailing slash once (also in the main method) and drop the if(), few lines below: if ($depth == 0 && $dir_has_slash) {...? Or I'm missing something.

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,120 @@ public function createFilename($basename, $directory) {
   +    // Allow directories specified in settings.php to be ignored. You can use
   +    // this to not check for files in common special-purpose directories. For
   +    // example, node_modules and bower_components. Ignoring irrelevant
   +    // directories is a performance boost.
   +    if (!isset($options['nomask'])) {
   +      $ignore_directories = Settings::get('file_scan_ignore_directories', []);
   +      array_walk($ignore_directories, function (&$value) {
   +        $value = preg_quote($value, '/');
   +      });
   +      $default_nomask = '/^' . implode('|', $ignore_directories) . '$/';
   +    }
   

   This also looks like a part that should run only once. It seems that on every recursion the result of $default_nomaks is the same. It doesn't depend on the recursion depth. We can move it in the main method and pass it to the protected method, or add it as a new entry in $options, as 'default_nomaks'.

4. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,120 @@ public function createFilename($basename, $directory) {
   +    // Avoid warnings when opendir does not have the permissions to open a
   +    // directory.
   +    if (!is_dir($dir)) {
   +      throw new NotRegularDirectoryException("$dir is not a directory.");
   +    }
   

   This check is also needed only for the top directory because the subsequent directories are checked again with is_dir()few lines below: if ($options['recurse'] && is_dir($uri)) {. That means every sub-dir is checked twice. We can move it in the main method.

5. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,120 @@ public function createFilename($basename, $directory) {
   +          if ($depth == 0 && $dir_has_slash) {
   +            $uri = "$dir$filename";
   +          }
   

   If the trim of trailing slash works (see above), we can drop this.

Re: #50 @martin107 the deprecation messages on file_destination() and file_unmanaged_prepare() aren't in this patch??

@kim.pepper - Sorry I did not mean to give you grief.

As I visually scanned the patch, my brain flagged the imperfections that phpcs would spot and then I blindly reported the stuff incorrectly.

by way of getting right with the universe .. here is the change.

   987 | ERROR   | [ ] The deprecation text '@deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use
       |         |     \Drupal\Core\File\FileSystemInterface::scanDirectory() instead.' does not match the standard format:
       |         |     @deprecated in %in-version% and will be removed from %removal-version%. %extra-info%.
   993 | WARNING | [ ] The deprecation version 'Drupal 8.8.0 and will be removed before Drupal 9.0.0' does not match the
       |         |     standard: drupal:n.n.n or project:n.x-n.n
   993 | WARNING | [ ] The 'See' url 'https://www.drupal.org/node/3038437.' should not end with a period. 

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,110 @@ public function createFilename($basename, $directory) {
   +      throw new NotRegularDirectoryException("$dir is not a directory.");
   
   +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
   @@ -462,4 +462,43 @@ public function createFilename($basename, $directory);
   +   * @throws \Drupal\Core\File\Exception\FileException
   +   *   Implementation may throw FileException or its subtype on failure.
   

   These are not the same. I don't know we can document parent exception like that.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -636,4 +637,110 @@ public function createFilename($basename, $directory) {
   +   * @throws \Drupal\Core\File\Exception\FileException
   +   *   Implementation may throw FileException or its subtype on failure.
   

   This not thrown anywhere in this function. Am I missing something?

Here is a a rerolled-patch

Hiding old patches

+++ b/core/includes/install.inc
@@ -166,9 +166,11 @@ function drupal_get_database_types() {
diff --git a/core/includes/theme.inc b/core/includes/theme.inc

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
--- a/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php

+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
@@ -198,7 +198,9 @@ public function deleteAll() {
+      $this->fileSystem->scanDirectory('public://js', '/.*/', ['callback' => $delete_stale]);

This method call (scanDirectory) throws an exception, should it be caught here and in all other cases where it's called?

Resolved one coding standard issue & added an interdiff as well.

1. +++ b/core/modules/image/tests/src/Functional/ImageAdminStylesTest.php
   @@ -44,7 +44,11 @@ public function createSampleImage(ImageStyleInterface $style) {
   +    $count = 0;
   +    if (is_dir('public://styles/' . $style->id())) {
   +      $count = count(\Drupal::service('file_system')->scanDirectory('public://styles/' . $style->id(), '/.*/'));
   +    }
   

   I'm not *completely* convinced that requiring a valid directory is a good thing, results in a ton of extra checks.

2. +++ b/core/modules/system/tests/src/Kernel/Installer/InstallTranslationFilePatternTest.php
   @@ -1,16 +1,18 @@
   +
   +  protected static $modules = ['system'];
    
   

   this needs a docblock.

3. +++ b/core/scripts/run-tests.sh
   --- a/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   
   +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   @@ -252,4 +252,11 @@ public function providerTestUriScheme() {
   +  public function testDepreactedScanDirectory() {
   +    $this->assertNotNull(file_scan_directory('temporary://', '/^NONEXISTINGFILENAME/'));
   +  }
   

   Depreacated :)

1. Yes, lets see if we can get feedback on that, RTBC otherwise IMHO, splitting the method into a public API and the internal one for the recursion looks much cleaner.

Reroll and fix message according current core sniffers

One minor point, otherwise looks good:

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -625,4 +626,97 @@ public function createFilename($basename, $directory) {
+    // directories is a performance boost.
+    if (!isset($options['nomask'])) {
+      $ignore_directories = $this->settings->get('file_scan_ignore_directories', []);
+      array_walk($ignore_directories, function (&$value) {
+        $value = preg_quote($value, '/');
+      });
+      $options['nomask'] = '/^' . implode('|', $ignore_directories) . '$/';
+    }
+    if (!is_dir($dir)) {
+      throw new NotRegularDirectoryException("$dir is not a directory.");
+    }

The is_dir() check seems like it should come before the nomask, otherwise we're building up $options['nomask'] for no reason.

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -625,4 +626,97 @@ public function createFilename($basename, $directory) {
+      throw new NotRegularDirectoryException("$dir is not a directory.");

Should this be a InvalidArgumentException?

https://www.php.net/manual/en/class.invalidargumentexception.php

Committed dd90789 and pushed to 8.8.x. Thanks!