Move the unmanaged file APIs to the file_system service (file.inc)

Replace drupal_set_message with throw new Exception.

Fixed tests that generated exceptions in #1.

This is my first patch review, so pardon if I do anything wrong! It looks like putting the Exception throw before the call(s) to watchdog is going to prevent the logging of this error that should happen, right? I think it would be better to put the Exception throw above return FALSE;

That also brings up a minor issue that the returning of FALSE there doesn't make sense anyway, if an exception will be thrown before returning FALSE can even happen.

Actually, looking further into this, there are multiple places in this function where drupal_set_message would still need to be replaced with an Exception being thrown (basically, everywhere FALSE is currently returned). The patch only addresses the first error case.

Also, Exception messages should not be translated, so the use of t() here should be removed. See this issue: #2055851: Remove translation of exception messages

just re-wording a bit.

Replace drupal_set_message() with exceptions in file_unmanaged_copy()
Replace t() with String::format() in message texts.
Alter tests.

9: 2244513-9.patch queued for re-testing.

Let's use explicit Exception classes per condition + a base class that can be caught; e.g.:

Drupal\Core\File\Exception\FileException          (generic base)
Drupal\Core\File\Exception\FileNotFoundException  (extends FileException)

+++ b/core/includes/file.inc
@@ -699,15 +699,13 @@ function file_unmanaged_copy($source, $destination = NULL, $replace = FILE_EXIST
+    throw new Exception(String::format('The specified file %file could not be copied because no file by that name exists. Please check that you supplied the correct filename.', array('%file' => $original_source)));

I might be wrong about this, but I don't think we're using format() in exceptions anymore -- IIRC, we just use sprintf(). (Also, the String class is long gone to support PHP 7. :)

Marking for re-roll due to usages of the String class.

This patch totally refactors file_unmanaged_copy(), turning it into a backwards-compatible wrapper around a new file_handler.unmanaged service. This service provides a copy() method which will throw exceptions (based on a new FileException class) when various error conditions occur.

Still needs doc comments and tests.

This issue wants to be its own parent?! Kinky, but no.

+++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
@@ -0,0 +1,80 @@
+      if (($realpath = drupal_realpath($original_source)) !== FALSE) {
...
+      $dirname = drupal_dirname($destination);

Remember, methods like drupal_realpath() and drupal_dirname() are already deprecated wrappers around the file_system service, so this class should have that injected and use it instead.

Good point. Fixed in this patch.

Reroll.

Rerolled again. Still needs tests...but I'm determined to get this into 8.3.x. It's long overdue.

I make test fun times!

D'oh. Like an idiot, caught the wrong exception in modified file_unmanaged_copy(). THIS patch oughta pass everything.

1. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,92 @@
   +class UnmanagedFileHandler implements UnmanagedFileHandlerInterface {
   

   nit: missing docblock

2. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,92 @@
   +      $destination = file_build_uri($this->fileSystem->basename($source));
   ...
   +    if (file_prepare_directory($destination)) {
   ...
   +      $destination = file_stream_wrapper_uri_normalize($destination . '/' . $this->fileSystem->basename($source));
   ...
   +    $destination = file_destination($destination, $replace);
   ...
   +    file_ensure_htaccess();
   

   Can we replace these with injected services? If not, a follow up to to do so would be good.

3. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,92 @@
   +    if (!@copy($source, $destination)) {
   

   if we're silently swallowing errors, shouldn't we have an else here?

4. +++ b/core/tests/Drupal/Tests/Core/File/UnmanagedFileHandlerTest.php
   @@ -0,0 +1,95 @@
   + * @coversDefaultClass \Drupal\Core\File\UnmanagedFileHandler
   

   nit: i think there's supposed to be a space here. And needs a docblock

file_unmanaged_copy() will throw an Exception on error, instead of returning FALSE.

That wouldn't be acceptable but it does no such thing:

  try {
    return \Drupal::service('file_handler.unmanaged')->copy($source, $destination, $replace);
  }
  catch (FileException $e) {
    \Drupal::logger('file')->error($e->getMessage());
    return FALSE;
  }
}

Point. IS updated.

I have tried to address @larowlan's feedback from #30. Uploading patch for testing.

• Reused values of FILE_EXISTS_* constants from interface to make sure that they do not end up having different values by accident.
• Updated documentation to more closely follow coding standards and added missing docblock to UnmanagedFileHandler.
• Renamed NonExistentFileException into easier to read FileNotExistsException.
• Used sprintf() instead of variable interpolation to format exception messages.

The file_unmanaged_* API consists of 6 functions, as well as several other helpers that are shared between managed and unmanaged API. It does not make sense to refactor only one of them into a service and keep the rest of the file.inc untouched.

In #2229865: [meta] Modernize File/StreamWrapper API @larowlan suggested to use League\Flysystem to replace Drupal file management code but this suggestion did not receive any support. Personally, I think that this change would be too large for a minor release, but D8 would still benefit from a File API based on services.

First of all, deprecate internal file_unmanaged_prepare() in the similar manner.

Thanks for the great work continuing this patch. I've wanted this to get in since Drupal 8.0.0, but I keep getting sidetracked with other things.

+++ b/core/lib/Drupal/Core/File/UnmanagedFileHandlerInterface.php
@@ -50,10 +45,10 @@
+  public function copy($source, $destination, $replace = FileHandlerInterface::FILE_EXISTS_RENAME);

I think we should stick with self::FILE_EXISTS_RENAME, since UnmanagedFileHandlerInterface extends FileHandlerInterface by design.

1. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -32,7 +33,63 @@ public function __construct(FileSystemInterface $file_system) {
   +   * Internal function that prepares the destination for a file_unmanaged_copy or
   +   * file_unmanaged_move operation.
   

   Should read "Prepares the destination for a file copy or move operation."

2. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -32,7 +33,63 @@ public function __construct(FileSystemInterface $file_system) {
   +   *   This public method is marked internal in 8.3.x and will be
   +   *   converted into private method before 9.0.0.
   

   Drupal core rarely if ever uses private anything, so this should read "will be converted into a protected method".

3. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -32,7 +33,63 @@ public function __construct(FileSystemInterface $file_system) {
   +   * @throws \Drupal\Core\File\Exception\DirectoryNotReadyException
   +   * @throws \Drupal\Core\File\Exception\FileException
   +   * @throws \Drupal\Core\File\Exception\FileExistsException
   +   * @throws \Drupal\Core\File\Exception\FileNotExistsException
   

   These should all document the circumstances under which each exception type is thrown.

@phenaproxima
I will try to work on this patch as much as I can and your feedback is very appreciated!!

I agree with your comments in #37, uploading updated patch for testing. I will move file_unmanaged_move next.

With this version of patch, all 6 file_unmanaged_* function are moved into UnmanagedFileHandler service.

Changes from previous patch:

• New methods in UnmanagedFileHandler: delete(), deleteRecursive(), move(), saveData().
• Extend FileException from RuntimeException because that is what SPL file classes throw.
• New FileWriteException, NotRegularFileException.

Old unmanaged and managed file APIs have very parallel structure file_delete() vs. file_unmanaged_delete(), file_copy() vs. file_unmanaged_copy(), etc. I think it would be nice to have two services in core file_handler.unmanaged and file_handler.managed that have the same set of public methods and can be used interchangeably.

I think we also need to consider file_save_upload(), file_save_data() and file_save_htaccess(). It'd be great if the IS could list all the methods to be refactored into a service and what the new service and method name is. Also it'd be great if the initial refactor concentrated on the interface and service names rather than any internal refactoring. As I said in #2482783-62: File upload errors not set or shown correctly

We should ensure the interface reflects the need to do future work to separate UI from API functionality. All of the existing methods mix drupal_set_message() with API functionality making it really hard to disentangle everything.

So let's start with a list of the non deprecated methods in file.inc and file.module that are related to this.

file.inc

Unmanaged file service?

• file_unmanaged_copy()
• file_unmanaged_prepare()
• file_unmanaged_move()
• file_unmanaged_delete()
• file_unmanaged_delete_recursive()
• file_unmanaged_save_data()

Separate issue to move to 'file_system' service or a 'file_url/i' service

• file_uri_target()
• file_stream_wrapper_uri_normalize()
• file_create_url() ?
• file_url_transform_relative() ?
• file_valid_uri()
• file_build_uri()

Separate issue to move to 'file_system'

• file_prepare_directory()
• file_destination()
• file_create_filename()
• file_scan_directory()
• file_directory_temp()

Not sure

• file_ensure_htaccess()
• file_save_htaccess()
• file_htaccess_lines() is weirdly deprecated to \Drupal\Component\PhpStorage\FileStorage::htaccessLines() not sure that this is right
• file_default_scheme() not sure this is just a config get - probably should be deprecated just to read from config.
• file_munge_filename()
• file_unmunge_filename()
• file_upload_max_size()

file.module

Managed file service - or stuff on the entity?

• file_copy()
• file_move()
• file_validate()
• file_save_data()
• file_save_upload()
• file_managed_file_submit()
• file_managed_file_save_upload()

Weird hooks - the hook is file_validate but these have no module identifier - I would have expected file_file_validate_name_length() for example...

• file_validate_name_length()
• file_validate_extensions()
• file_validate_size()
• file_validate_is_image()
• file_validate_image_resolution()

Not sure

• file_get_content_headers()

Separate issue to deprecate in favour of entity methods

• file_delete()
• file_delete_multiple()

file_get_content_headers()

Glancing at this function, I think this should be made into a method of file entities. FileInterface::getContentHeaders().

I'm not sure if this would constitute a BC break, though. Maybe if we added it to the File entity class, but not FileInterface, and updated the function to accept only a File, rather than FileInterface, we could have a @todo to move File::getContentHeaders() to FileInterface in D9.

Some random thoughts:

* I think we should avoid putting a lot of logic on the file entity class, so better make a service. But we could have methods that call that service, just like $entity->save() calls the storage handler?

* validate stuff isn't a hook, it's just callbacks that you specify, with arguments/configuration. Maybe we could use validation constraints for this?

* file_create_url() has an issue already: #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it

* Partially unrelated but I'm not sure why we have file_upload_max_size() and use it to validate the filesize. If it's really bigger than the allowed size ,then it possibly doesn't even get to that point. Due to generic entity validation, this can also fail on files that were uploaded to S3 or so which is super weird. Will open a dedicated issue.

* file_managed_file_submit() should be a static method on \Drupal\file\Element\ManagedFile, but \Drupal\image\Plugin\Field\FieldWidget\ImageWidget::validateRequiredFields() is a fun example where switching could possibly break something if someone else relies on that too.

* file_get_content_headers() sounds like a good match for a method on File, yes.

I think we should avoid putting a lot of logic on the file entity class, so better make a service

Personally, I think that File entity class is a perfect place to put refactored version of the current "managed file API".

// This static method already exists and this is the official way to construct
// new entity objects.
$file = File::create([
  'uri' => 'public://file.png',
]);

// A similarly named static method can replace file_save_data()
$file = File::createData($data, [
  'uri' => 'public://file.png',
]);

// And to replace file_copy()
$file = File::createCopy($original, [
  'uri' => 'public://file.png',
]);

If you think about it, all these three methods do a similar thing: create a record in the DB to reflect state of a file on disk, the difference is in how they process input arguments.

As of the other functions of the "managed file API"

file_move($file, 'public://destination.txt');
// becomes
$file->move('public://destination.txt');

file_delete($file);
// becomes
$file->delete();

Behind the scenes, they all will be using a couple of services but a public-facing API can be added to File entity directly.

I don't know if it will help the current discussion, but I've described the problems I see with the file API in the parent meta-issue #2229865: [meta] Modernize File/StreamWrapper API in #12.

Behind the scenes, they all will be using a couple of services but a public-facing API can be added to File entity directly.

That's *exactly* what I said?

Also, $file->delete() already exists, so we can immediately deprecate those two methods.

Pretty sure we want to make this a meta/plan issue, so those delete functions would be an easy standalone API.

@Berdir
I wasn't trying to argue with what you said. On the contrary, just adding my two cents.

@alexpott
Thanks for the suggestion on where to move next. I really needed some feedback on this patch. Perhaps, #2229865: [meta] Modernize File/StreamWrapper API would be a better place to create a list of new services. There is one there already but it is almost three years out of date.

A lot of work has already been started. As @Berdir said, there is #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it for file_create_url(), there is also #2620304: htaccess functions should be a service opened by @phenaproxima, and other issues as well.

No updates from @phenaproxima, removing assignment.

Coming here from #2800267: Turn simpletest_clean*() functions into a helper class where we still have file_unmanaged_delete_recursive() as a dependency.

Adding file name to the issue title to help with issue searching

Reroll of #41

A lot of the files point to core/includes/file.inc:846 which calls \Drupal::logger('file')->error($e->getMessage());

Do we need to initialise watchdog or something?

Given #2821423: Dealing with unexpected file deletion due to incorrect file usage and #2835840: Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.) I think we should be completely rewriting this functionality (i.e. not using a counter) rather than converting it to OOP. Going to move this to 'needs more info'.

@catch: This is about the (unmanaged) low-level file_unmanaged_* API functions mostly, which have nothing to do with file usage which sets several API levels above it.

I don't think it needs to be postponed on that discussion.

Doh, probably I should have read the issue properly..

@kim.pepper it points that a lot of exceptions trown, maybe better to rework usage somehow?

As example we have a lot of external url generation for files #2669074-37: Convert file_create_url() & file_url_transform_relative() to service, deprecate it and usage of (un)managed api still high in core as well, so both changes require followups to replace usage and rework, then we can discover why exception rate is so high.

not sure that all failed tests require dblog module enabled

Reroll of #58

Looks like throwing an exception if the file could not deleted was something introduced here, which is throwing lots of exceptions in tests.

I've removed it to see what other fails we are getting.

Heads-up that #2987538: Notice: Constant FILE_EXISTS_RENAME already defined might require changing a test or two.

Or we could change FileCopyTest to use the new FILE_* constants here.

Thanks Mile23. I'll try fixing some of these tests first.

after change record it should be added to all deprecated functions

Added draft change record https://www.drupal.org/node/3006851

Looks good, now only use of CR left

Tagging for framework manager sign-off, since we're modifying Drupal core's API here.

Added @see for CR and fixed codesniff issues in \Drupal\Core\File\UnmanagedFileHandlerInterface

Also \Drupal\Core\File\UnmanagedFileHandler::prepare() is not in \Drupal\Core\File\UnmanagedFileHandlerInterface

This was marked as @internal on file_unmanaged_prepare() and its calling \Drupal::service('file_handler.unmanaged')->prepare() internally. Is this intentional?

Coding standards still reports 7 additions

1. +++ b/core/includes/file.inc
   @@ -452,116 +464,40 @@ function file_valid_uri($uri) {
   - * - Checks if $source and $destination are valid and readable/writable.
   - * - Checks that $source is not equal to $destination; if they are an error
   - *   is reported.
   - * - If file already exists in $destination either the call will error out,
   - *   replace the file or rename the file based on the $replace parameter.
   ...
   - * @param $source
   - *   A string specifying the filepath or URI of the source file.
   

   I think this doc block should not change especially about params because this is API still

2. +++ b/core/includes/file.inc
   @@ -452,116 +464,40 @@ function file_valid_uri($uri) {
   -  // Make sure the .htaccess files are present.
   -  file_ensure_htaccess();
   
   +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,247 @@
   +    // Make sure the .htaccess files are present.
   +    file_ensure_htaccess();
   

   Should point to #2620304: htaccess functions should be a service

Addresses feedback in #79

Also found an issue with it not checking the realpath.

This looks good, a couple of observations,

We also need deprecation tests and trigger_error calls?

1. +++ b/core/includes/file.inc
   @@ -452,116 +464,40 @@ function file_valid_uri($uri) {
   + * @deprecated in Drupal 8.7.0, will be removed before Drupal 9.0.0.
   + *   Use \Drupal::service('file_handler.unmanaged')->copy().
   

   this needs to have a trigger_error with a link to a CR in the deprecation message, applies for all of these?

2. +++ b/core/includes/file.inc
   @@ -452,116 +464,40 @@ function file_valid_uri($uri) {
   +    return \Drupal::service('file_handler.unmanaged')
   +      ->copy($source, $destination, $replace);
   ...
   +    \Drupal::logger('file')->error($e->getMessage());
   

   shouldn't UnmanagedFileHandler::copy handle the logging for us so there is parity with the existing function?
   I think this is better (leave it up to the caller) but I think we have to give consumers like for like.

3. +++ b/core/includes/file.inc
   @@ -452,116 +464,40 @@ function file_valid_uri($uri) {
   - * @param $source
   - *   A string specifying the filepath or URI of the source file.
   - * @param $destination
   - *   A URI containing the destination that $source should be moved/copied to.
   - *   The URI may be a bare filepath (without a scheme) and in that case the
   - *   default scheme (file://) will be used. If this value is omitted, Drupal's
   - *   default files scheme will be used, usually "public://".
   - * @param $replace
   - *   Replace behavior when the destination file already exists:
   - *   - FILE_EXISTS_REPLACE - Replace the existing file.
   

   i think we retain these, even though its deprecated

4. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,248 @@
   + * Implements service for low-level unhandled file operations.
   

   unmanaged?

5. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,248 @@
   +  public function prepare($source, &$destination = NULL, $replace = self::FILE_EXISTS_RENAME) {
   

   should we call this prepareDestination for clarity?

@larowlan thanks for the review!

1. Yep. I think there is going to be a lot of usage that needs updating.
2. Does this mean we catch the exceptions in UnmanagedFileHandler, log the exception, then re-throw the exception, and re-catch in the deprecated functions? If not, then we don't really need to throw exceptions inside UnmanagedFileHandler at all.
3. ok
4. ok
5. ok

Re #82.2 nevermind. I think I'll just move the logging as you suggested.

Re #82

1. Haven't removed any usage yet. Will do next patch
2. Fixed. Moved logging to UnmanagedFileHandler
3. After looking, not entirely sure what you mean here?
4. Fixed
5. Fixed

This replaces all usages of the deprecated unmanaged file functions. Lets see what testbot thinks.

Fixed an unusual function name concatenation occurrence, as well as uncovered a bug in 2 tests which thought they were loading translation files but weren't.

We're throwing exceptions now, so we need to expect them in tests. I also updated the changelog example to show you need to handle exceptions.

Added some more expectExceptions to tests.

1. +++ b/core/lib/Drupal/Core/Asset/AssetDumper.php
   @@ -25,7 +25,9 @@ public function dump($data, $file_extension) {
   +    $file_handler = \Drupal::service('file_handler.unmanaged');
   

   can we inject this?

2. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -178,7 +178,7 @@ public function deleteAll() {
   +        \Drupal::service('file_handler.unmanaged')->delete($uri);
   
   +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
   @@ -180,7 +180,7 @@ public function deleteAll() {
   +        \Drupal::service('file_handler.unmanaged')->delete($uri);
   

   same here?

3. +++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
   @@ -0,0 +1,281 @@
   +   * @internal
   +   *   This public method is marked internal in 8.3.x and will be
   +   *   converted into protected method before 9.0.0.
   

   c/p? should be 8.7.x

   We should deprecate it straight away if that's the case?

Re #94

1. Fixed
2. Fixed
3. Fixed

1 and 2 from #94 addressed.

Tiny nit from 3:

+++ b/core/lib/Drupal/Core/File/UnmanagedFileHandler.php
@@ -191,8 +191,7 @@ public function move($source, $destination = NULL, $replace = self::FILE_EXISTS_
+   * @deprecated This public method is marked internal in 8.7.0 and will be

Comment still talks about being marked internal but has been set as deprecated.

Thanks @acbramley! Fixed.

Thanks @kim.pepper! All feedback from #94 has been addressed.

+++ b/core/lib/Drupal/Core/Asset/AssetDumper.php
@@ -25,7 +43,7 @@ public function dump($data, $file_extension) {
-    if (!file_exists($uri) && !file_unmanaged_save_data($data, $uri, FILE_EXISTS_REPLACE)) {
+    if (!file_exists($uri) && !$this->fileHandler->saveData($data, $uri, FILE_EXISTS_REPLACE)) {

@@ -37,7 +55,7 @@ public function dump($data, $file_extension) {
-      if (!file_exists($uri . '.gz') && !file_unmanaged_save_data(gzencode($data, 9, FORCE_GZIP), $uri . '.gz', FILE_EXISTS_REPLACE)) {
+      if (!file_exists($uri . '.gz') && !$this->fileHandler->saveData(gzencode($data, 9, FORCE_GZIP), $uri . '.gz', FILE_EXISTS_REPLACE)) {

This code now doesn't make much sense because saveData is never going to return FALSE. It's going to throw an exception. The change to make these utility functions throw exception instead of return FALSE is always tricky to implement because these functions are so low level. Essentially every usage now needs to be wrapped with a try / catch to be sure of BC. For me that means we shouldn't do that in this issue because it makes a lot of work and is change on top of a refactor. That said this is one of the times when you get to do this so it is a good time to consider it. With these though because they are so low level and used by so many other utility like places I'm not sure it is worth it and I think we need more discussion on issue as to the advantages of throwing exceptions.

I do feel bad about my comment after so much work was put into this issue, the first part might realtively easy to adress, not so sure about the follow-up thought.

+++ b/core/core.services.yml
@@ -369,6 +369,9 @@ services:
     class: Drupal\Core\File\FileSystem
     arguments: ['@stream_wrapper_manager', '@settings', '@logger.channel.file']
+  file_handler.unmanaged:
+    class: Drupal\Core\File\UnmanagedFileHandler
+    arguments: ['@file_system', '@logger.channel.file']
   form_builder:

Surprised to see that the name hasn't been discussed more, at least not as far as I see. I think there are 3 different things that I don't like about it :p

* . as a separator seems strange here, if we use that then usually as a separator for a subsystem or so, e.g. entity. or language. (although we're also very inconsistent with this stuff, e.g. entity_type.manager. But file_handler isn't a component or anything. So if we have a dot, then file.something? But we also have file_system above, so we could just as well do file_something.

* The concept of an "unmanaged" API was introduced when we added file entities in 7.x and needed API functions that were lower-level than that. E.g. compared to file_move(), file_copy(). Without namespaces or anything, we had to come up with something to deal with that overlap. I think we can do better today and figure out a name for these low level file API that's better than "unmanaged".

* handler: What exactly is this handling? This doesn't tell me anything about what this service does, especially compared to the already existing file_system service. Would it maybe be an option to add these file_system to the existing FileSystem service?

--

Looking at this made me think again about the sad state of our low-level file API. I had a look at the git history and the only real change in there in the last 2+ years is a bugfix to the fairly new and actually not file system related file_url_transform_relative().

Is this really a part of Drupal that we want to and can maintain ourself?

Is there a good reason we can't use https://symfony.com/doc/3.4/components/filesystem.html, for example? Or http://flysystem.thephpleague.com/, which seems to be very actively maintained and might allow us to also remove/simplify a ton of custom code around stream wrappers as well.

Thanks @Berdir! I think this is very useful feedback and worthy of discussion.

I'd be happy to consider leveraging existing filesystem components such as symfony filesystem or flysystem. I'm not sure how much work, or framework manager buy in there would be for this kind of change.

I think we'd need to be sure of that before doing any more work.

I think @Berdir raises an excellent point. It would be good to have some exploration of leveraging others work. One thing I think that will cause problems with adopting another framework is where the Drupal application bleeds into the low-level system. For example logging and configuration.

Another thing that @Berdir's comment makes me think is that we already have a file system service - why are we not adding these methods to that and then sometime in the future try and refactor that to leverage Symfony or PHPLeague's code? Hah, It looks like I suggested this split in #46. I think that that is what we are still missing - a discussion about the overall API and where everything belongs. Doing things piecemeal looks like we're ending up with a hodgepodge of services. I think I've over-egged the importance of the word unmanaged - this made more sense when the functions are all global but once they're on a file system service is more obvious they are low level and do not concern entities. If we consolidate these method on the file system service then it'll be easier to potentially move to another framework's code because we'll have a better idea of all the functionality we need.

#103 Makes sense to me. I can give this a go later this week. Happy if anyone else wants to jump in in the meantime.

Yeah, or as I said in #101: "Would it maybe be an option to add these file_system to the existing FileSystem service?" :)

+1 to moving these methods there and then later on evaluate if we can re-use symfony or something else there internally. Needs work for that, then.

I also added a comment to #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it about all the file_url related functions mentioned in #46.

OK. Moved everything over to FileSystem service, and deleted the UnmanagedFile service.

Discussed whether we should be adding exceptions here and agreed "I think now is the only time when we *can* add exceptions".

I expect there to be a few fails with this patch, but sharing anyway. :-)

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -301,4 +307,239 @@ public function validScheme($scheme) {
   +    // Prepare the destination directory.
   +    if (file_prepare_directory($destination)) {
   +      // The destination is already a directory, so append the source basename.
   +      $destination = file_stream_wrapper_uri_normalize($destination . '/' . $this->basename($source));
   

   So this function is categorized as a file url related thing in #46 but I think the name is misleading. it looks like it's just a wrapper around several other functions. one is already a method on this service, another is a deprecated function on this service and the third is a pretty simple preg_replace function that is used in quite a lot of places.

   Considering that, I would move this , including the functions it calls, into the file system service as well and deprecate the leftovers too.

   I guess that makes sense as a separate issue, together with file_prepare_directory(), file_destination and that group, which #46 already suggests to do so in a separate issue.

   While it will be a large service with a lot of methods, I think these things belong together.

2. +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php
   @@ -237,4 +252,153 @@ public function uriScheme($uri);
   +   *
   +   * @return bool
   +   *   TRUE for success; FALSE if path does not exist.
   +   *
   +   * @throws \Drupal\Core\File\Exception\FileException
   +   *   Implementation may throw FileException or its subtype on failure.
   

   no need for both then, I suppose we can remove the @return, will need quite a lot of test updates that are currently doing assertTrue()

3. +++ b/core/modules/file/file.module
   @@ -226,7 +226,7 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FILE_E
     *
   - * @see file_unmanaged_move()
   + * @see \Drupal::service('file_system')->move()
     * @see hook_file_move()
   

   search & replace thing, @see should refer to the interface, not this :)

4. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -239,8 +239,11 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
    
   +    /** @var \Drupal\Core\File\UnmanagedFileHandler $file_handler */
   +    $file_handler = \Drupal::service('file_system');
   

   leftover @var class + variable name

5. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -130,6 +131,7 @@ public function testExistingError() {
        // Clone the object so we don't have to worry about the function changing
        // our reference copy.
   +    $this->expectException(FileExistsException::class);
        $result = file_copy(clone $source, $target->getFileUri(), FILE_EXISTS_ERROR);
    
        // Check the return status and that the contents were not changed.
   

   I guess we should confirm that this direction makes sense before going further but I like how this will clean this up, the code below is now not needed anymore.

6. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -176,8 +176,12 @@ protected function writeFile($source, $destination, $replace = FILE_EXISTS_REPLA
   -    $function = 'file_unmanaged_' . ($this->configuration['move'] ? 'move' : 'copy');
   -    return $function($source, $destination, $replace);
   +    /** @var \Drupal\Core\File\FileSystemInterface $file_handler */
   +    $file_handler = \Drupal::service('file_system');
   +    if ($this->configuration['move']) {
   +      return $file_handler->move($source, $destination, $replace);
   

   I guess a $fille_handler => $file_system search & replace should take care of a lot of these.

I haven't been able to work out that test fail yet, but took care of the rest of the feedback in #107

1. Moved and deprecated file_prepare_directory()
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed

Fix some test fails.

FileSystemInterface::prepareDirectory() needs $directory to be passed by reference.

+++ b/core/modules/file/tests/src/Kernel/CopyTest.php
@@ -132,17 +132,7 @@ public function testExistingError() {
-    // Check that the correct hooks were called.
-    $this->assertFileHooksCalled([]);
-
-    $this->assertFileUnchanged($source, File::load($source->id()));
-    $this->assertFileUnchanged($target, File::load($target->id()));

Hm, this part was actually useful and might make sense to keep? Do we need a try/catch with manual fail here then?

+++ b/core/includes/file.inc
@@ -44,7 +44,7 @@
 /**
- * Flag used by \Drupal::service('file_system')->prepareDirectory() -- create directory if not present.
+ * Flag used by \Drupal\Core\File\FileSystemInterface::prepareDirectory() -- create directory if not present.
  *

these are now > 80 characters, given that it is deprecated, we can maybe remove the method reference? Technically, these new methods are not actually using this constants anymore anyway ;)

Looks like I missed importing FileSystemInterface for some of the find/replace of constants.

Re: #114

1. Fixed
2. Fixed

More missing imports 🤦‍♂️

..and more

...and more.

This might do the trick. The kernel test was failing because it defaulted to rename, so the self-copy didn't actually self-copy, added a separate method to test both versions.

And the warnings happened due to a missing @ inside prepareDestination() that is suppressing the warning. Somehow that got lost in the conversion. Kinda weird to do that like that, but it's been like thatbefore.

The level of nesting that's going on in those file system calls is pretty mindblowing and "fun" to debug as we call into the LocalStream implementation, which then goes through the deprecated functions back into the file_system service where the real action finally happens. We should create an issue to add @trigger_error()'s to the already deprecated functions in file.inc and convert them.

Cross-posting my comment from the file_create_url() issue in regards to those uri functions that we also need to move somewhere.

Hm, on closer investigation, I don't think any of these additional methods belong into this new service.

* file_uri_target() is the counter-part to file_uri_scheme(), which is already on file_system
* file_stream_wrapper_uri_normalize() is also very related to these, they're all about (stream wrapper) URI's, not file URLs.
* file_valid_uri() is again about stream wrapper URI's and as a small wrapper around two functions/methods that were already moved to file_system.
* file_build_uri() also is about creating stream wrapper URIs and is tightly connected to these other functions.

FileSystem is getting quite big with that other issue, so maybe these methods could be on the stream wrapper manager, but then we'd also need to move some existing methods from FileSystem to that to avoid a cross-dependency.

This is definitely already big enough, so lets do a follow-up issue for these, with these we are getting close to having the whole file deprecated. Looking at what's left now..

* Another is file_destination(), that is actually small enough to possibly handle in this issue, I'm only seeing ~4 non-test calls to that. Could be challenging one to name, though. getDestinationFilename()?
* file_create_filename() is except in tests only used inside file_destination() in core at least. could also be a target for making protected in 9.x as you could always call it through the other method?
* file_munge_filename() is a pretty weird one, maybe we should have something like a file upload service, with functions related to handling uploads, which this clearly is. (That said, drupal_move_uploaded_file() was already moved to the file_system service, so we could also just move those to that as well)
* Same for file_unmunge_filename(), which is actually used only once in a test.
* file_upload_max_size(), another upload related thingy.
* the file status constant, that should be a separate issue to move it to FileInterface.
* Then we have file_delete() and file_delete_multiple(), which should be trivial deprecations, both are unused in core, same issue as th one above?
* file_directory_temp() is kinda weird and scary as it also updates configuration on the fly. not quite sure why that's even an "API", as temporary:// should have the same effect, except maybe in super-early installer or something?
* file_scan_directory() I think should be separate, there's an issue to use the symfony finder component, or it should be something in the extension subsystem?

Thanks @Berdir. I created a number of follow up issues:

Re: #125 created #3021651: Add trigger_error to deprecated functions in file.inc in convert them

Re #126 created

• #3021652: Deprecate upload-related functions file.inc and move to a file upload service
• #3021653: Move file status constant to FileSystemInterface
• #3021654: Deprecate file_delete() and file_delete_multiple() in file.inc

Re #126:

* Another is file_destination(), that is actually small enough to possibly handle in this issue, I'm only seeing ~4 non-test calls to that. Could be challenging one to name, though. getDestinationFilename()?
* file_create_filename() is except in tests only used inside file_destination() in core at least. could also be a target for making protected in 9.x as you could always call it through the other method?

Moved to file_system service and converted usages.

Updated IS and change record.

Missed a usage.

I'm a bit concerned file_system will become a dumping ground for anything related to files. I'd like to keep it simple and restrict it to low-level file operations. Anything higher level, like file upload functions, uri functions etc. should be separate.

+++ b/core/includes/file.inc
@@ -41,29 +44,44 @@
+const FILE_CREATE_DIRECTORY = FileSystemInterface::FILE_CREATE_DIRECTORY;
...
+const FILE_MODIFY_PERMISSIONS = FileSystemInterface::FILE_MODIFY_PERMISSIONS;
...
+const FILE_EXISTS_RENAME = FileSystemInterface::FILE_EXISTS_RENAME;
...
+const FILE_EXISTS_REPLACE = FileSystemInterface::FILE_EXISTS_REPLACE;
...
+const FILE_EXISTS_ERROR = FileSystemInterface::FILE_EXISTS_ERROR;

I think `FILE_` prefix should be removed from new constants because they already in FileSystemInsteface

Fixes for #132

Fixes deprecation notices and converts a number of deprecated constants to the new ones.

1. +++ b/core/includes/file.inc
   @@ -65,7 +65,7 @@
     */
   -const FILE_EXISTS_RENAME = FileSystemInterface::FILE_EXISTS_RENAME;
   +const FILE_EXISTS_RENAME = FileSystemInterface::EXISTS_RENAME;
    
   

   Unsure about that. for create directory and modify permissions, it works I guess, but the exists constants seem a bit strange now IMHO.

   Afaik all constants are about directories, so *maybe* we could include that in the name somehow?

   DIRECTORY_CREATE
   DIRECTORY_MODIFY_PERMISSIONS
   DIRECTORY_EXITsS_...?

   Alternatively RENAME_EXISTING? But then are no longer grouped together.

   Just thinking out loud, see also comments below.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -575,17 +575,17 @@ public function prepareDirectory(&$directory, $options = FileSystemInterface::FI
    
   -        case FileSystemInterface::FILE_EXISTS_ERROR:
   +        case FileSystemInterface::EXISTS_ERROR:
              // Error reporting handled by calling function.
              return FALSE;
   

   so what exactly does happen now with ERROR? Is it really still an error, isn't it an exception, and therefore should be called _EXCEPTION?

3. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -134,7 +134,7 @@ public function testExistingError() {
        try {
   -      $result = file_copy(clone $source, $target->getFileUri(), FileSystemInterface::FILE_EXISTS_ERROR);
   +      $result = file_copy(clone $source, $target->getFileUri(), FileSystemInterface::EXISTS_ERROR);
        }
        catch (FileExistsException $e) {
   

   ok, so the exists constants are not only for directories, so directory definitely doesn't make sense here.

   Also, this one seems double the fun:

   a) as mentioned, ERROR actually does an exception now.

   b) except this function is actually an existing one and we should not suddenly throw exceptions in it, which means we need to catch it and do what it it did before.

   on error vs exception, one option might be to use separate values for error and exception and deprecate error without a direct replacement, then we could also support that in file_copy() if someone does want an exception. But hopefully these functions will also move somewhere.

Extensive review of the whole patch, focused specifically on error handling, consistent DI as well as the test coverage, which showed some inconsistencies with return values.

I believe we can do the change from return value to exception but there are various calls to update still. Hopefully once this review is addressed, it should be RTBC-able.

1. +++ b/core/lib/Drupal/Core/Asset/AssetDumper.php
   @@ -24,8 +43,13 @@ public function dump($data, $file_extension) {
   -    file_prepare_directory($path, FILE_CREATE_DIRECTORY);
   -    if (!file_exists($uri) && !file_unmanaged_save_data($data, $uri, FILE_EXISTS_REPLACE)) {
   ...
   +    try {
   +      if (!file_exists($uri) && !$this->fileSystem->saveData($data, $uri, FileSystemInterface::EXISTS_REPLACE)) {
   +        return FALSE;
   +      }
   +    }
   +    catch (FileException $e) {
   

   one call to \Drupal::service().

   also prepareDirectory() does not yet throw an exceptions but does use return true/false. Should we change that too? Counting 50 usages to it, so that would be a ton of changes :-/

2. +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
   @@ -48,12 +56,15 @@ class JsCollectionOptimizer implements AssetCollectionOptimizerInterface {
       *   The state key/value store.
   +   * @param \Drupal\Core\File\FileSystemInterface $fileHandler
   +   *   The unmanaged file handler.
       */
   -  public function __construct(AssetCollectionGrouperInterface $grouper, AssetOptimizerInterface $optimizer, AssetDumperInterface $dumper, StateInterface $state) {
   +  public function __construct(AssetCollectionGrouperInterface $grouper, AssetOptimizerInterface $optimizer, AssetDumperInterface $dumper, StateInterface $state, FileSystemInterface $fileHandler) {
        $this->grouper = $grouper;
        $this->optimizer = $optimizer;
        $this->dumper = $dumper;
        $this->state = $state;
   +    $this->fileHandler = $fileHandler;
   

   still called fileHandler here.

3. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -76,7 +77,7 @@ public static function getFileExtension() {
      protected function ensureStorage() {
        $dir = $this->getCollectionDirectory();
   -    $success = file_prepare_directory($dir, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
   +    $success = \Drupal::service('file_system')->prepareDirectory($dir, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
        // Only create .htaccess file in root directory.
   

   Not injected yet.

4. +++ b/core/modules/color/color.module
   @@ -582,7 +583,7 @@ function _color_rewrite_stylesheet($theme, &$info, &$paths, $palette, $style) {
    function _color_save_stylesheet($file, $style, &$paths) {
   -  $filepath = file_unmanaged_save_data($style, $file, FILE_EXISTS_REPLACE);
   +  $filepath = \Drupal::service('file_system')->saveData($style, $file, FILE_EXISTS_REPLACE);
      $paths['files'][] = $filepath;
    
   

   still uses the old constant.

5. +++ b/core/modules/file/file.module
   @@ -166,7 +167,7 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FILE_E
        return FALSE;
      }
    
   -  if ($uri = file_unmanaged_copy($source->getFileUri(), $destination, $replace)) {
   +  if ($uri = \Drupal::service('file_system')->copy($source->getFileUri(), $destination, $replace)) {
        $file = $source->createDuplicate();
   

   repeating from previous comment, for completeness, I think file_copy() and file_move() should do a try/catch and return FALSE.

6. +++ b/core/modules/file/file.module
   @@ -1022,9 +1023,10 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  $file->destination = \Drupal::service('file_system')->getDestinationFilename($destination . $file->getFilename(), $replace);
   +  // If \Drupal::service('file_system')->getDestinationFilename() returns FALSE
   +  // then $replace === FILE_EXISTS_ERROR and there's an existing file so we need
   +  // to bail.
   

   Pretty awkward comment already and getting worse, half code/half english and also references the deprecated constant (looks like these were not replaced yet, seeing 50 cases just for REPLACE. separate issue maybe as this is huge already?).

   Maybe just something like this:

   // If the destination is FALSE then there is an existing file and $replace is set to return an error, so we need to bail.

   Or something like that.

7. +++ b/core/modules/file/src/Entity/File.php
   @@ -206,7 +206,7 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
          // were already deleted are logged to watchdog but ignored, the
          // corresponding file entity will be deleted.
   -      file_unmanaged_delete($entity->getFileUri());
   +      \Drupal::service('file_system')->delete($entity->getFileUri());
   

   the comment is a lie now, this will fail with an exception, so we need a try/catch. Maybe we can then also simplify the comment.

8. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -226,7 +226,7 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
    
        // Check the destination file path is writable.
   -    if (!file_prepare_directory($destination, FILE_CREATE_DIRECTORY)) {
   +    if (!\Drupal::service('file_system')->prepareDirectory($destination, FileSystemInterface::CREATE_DIRECTORY)) {
          throw new HttpException(500, 'Destination file path is not writable');
        }
    
   @@ -239,8 +239,11 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
   
   @@ -239,8 +239,11 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
    
        $temp_file_path = $this->streamUploadData();
    
   +    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
   +    $file_system = \Drupal::service('file_system');
   

   this should already have $this->fileSystem injecteed.

9. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -265,8 +268,8 @@ public function post(Request $request, $entity_type_id, $bundle, $field_name) {
        // Move the file to the correct location after validation. Use
        // FILE_EXISTS_ERROR as the file location has already been determined above
   -    // in file_unmanaged_prepare().
   -    if (!file_unmanaged_move($temp_file_path, $file_uri, FILE_EXISTS_ERROR)) {
   +    // in $file_system->prepareDestination().
   +    if (!$file_system->move($temp_file_path, $file_uri, FileSystemInterface::EXISTS_ERROR)) {
          throw new HttpException(500, 'Temporary file could not be moved to file location');
        }
    
   

   constant updated only in code but not comment, but as mentioned, I would suggest a follow-up to update them all.

   Also this is checking for a boolean return value still, we need to try/catch and throw the http exception.

10. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
    @@ -130,17 +132,21 @@ public function testExistingError() {
    +    $result = NULL;
    +    try {
    +      $result = file_copy(clone $source, $target->getFileUri(), FileSystemInterface::EXISTS_ERROR);
    +    }
    +    catch (FileExistsException $e) {
    

    restoring the behavior of file_copy() means we can then also completely revert the changes to this test and likely others, making the patch a tiny bit smaller again ;)

11. +++ b/core/modules/file/tests/src/Kernel/SaveDataTest.php
    @@ -121,6 +123,7 @@ public function testExistingError() {
     
         // Check the overwrite error.
    +    $this->expectException(FileExistsException::class);
         $result = file_save_data('asdf', $existing->getFileUri(), FILE_EXISTS_ERROR);
         $this->assertFalse($result, 'Overwriting a file fails when FILE_EXISTS_ERROR is specified.');
         $this->assertEqual($contents, file_get_contents($existing->getFileUri()), 'Contents of existing file were unchanged.');
    

    file_save_data() is another candidate for a try/catch...

12. +++ b/core/modules/image/src/Controller/QuickEditImageController.php
    @@ -95,7 +96,7 @@ public function upload(EntityInterface $entity, $field_name, $langcode, $view_mo
     
         // Create the destination directory if it does not already exist.
    -    if (isset($destination) && !file_prepare_directory($destination, FILE_CREATE_DIRECTORY)) {
    +    if (isset($destination) && !\Drupal::service('file_system')->prepareDirectory($destination, FileSystemInterface::CREATE_DIRECTORY)) {
    

    should we inject this?

13. +++ b/core/modules/locale/locale.bulk.inc
    @@ -506,7 +506,7 @@ function locale_translate_delete_translation_files(array $projects = [], array $
         foreach ($files as $file) {
    -      $success = file_unmanaged_delete($file->uri);
    +      $success = \Drupal::service('file_system')->delete($file->uri);
           if (!$success) {
             $fail = TRUE;
    

    try/catch

14. +++ b/core/modules/locale/locale.install
    @@ -32,7 +33,7 @@ function locale_uninstall() {
         $locale_javascripts = \Drupal::state()->get('locale.translation.javascript') ?: [];
         foreach ($locale_javascripts as $langcode => $file_suffix) {
           if (!empty($file_suffix)) {
    -        file_unmanaged_delete($locale_js_directory . '/' . $langcode . '_' . $file_suffix . '.js');
    +        \Drupal::service('file_system')->delete($locale_js_directory . '/' . $langcode . '_' . $file_suffix . '.js');
           }
    

    try/catch to make sure this can't somehow fail and break the uninstall?

15. +++ b/core/modules/locale/locale.module
    @@ -1298,8 +1300,12 @@ function _locale_rebuild_js($langcode = NULL) {
       if (!empty($locale_javascripts[$language->getId()]) && (!$data || $changed_hash)) {
    -    file_unmanaged_delete($dir . '/' . $language->getId() . '_' . $locale_javascripts[$language->getId()] . '.js');
    +    $file_system->delete($dir . '/' . $language->getId() . '_' . $locale_javascripts[$language->getId()] . '.js');
         $locale_javascripts[$language->getId()] = '';
    

    same.

16. +++ b/core/modules/media/media.install
    @@ -28,7 +29,7 @@ function media_install() {
         if (!file_exists($destination . DIRECTORY_SEPARATOR . $file->filename)) {
    -      file_unmanaged_copy($file->uri, $destination, FILE_EXISTS_ERROR);
    +      \Drupal::service('file_system')->copy($file->uri, $destination, FILE_EXISTS_ERROR);
         }
    

    same.

17. +++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
    @@ -381,7 +382,7 @@ protected function getLocalThumbnailUri(Resource $resource) {
         // log an error and bail out.
    -    if (!file_prepare_directory($directory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
    +    if (!\Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) {
           $this->logger->warning('Could not prepare thumbnail destination directory @dir for oEmbed media.', [
    

    inject?

18. +++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
    @@ -395,7 +396,7 @@ protected function getLocalThumbnailUri(Resource $resource) {
         try {
           $response = $this->httpClient->get($remote_thumbnail_url);
           if ($response->getStatusCode() === 200) {
    -        $success = file_unmanaged_save_data((string) $response->getBody(), $local_thumbnail_uri, FILE_EXISTS_REPLACE);
    +        $success = \Drupal::service('file_system')->saveData((string) $response->getBody(), $local_thumbnail_uri, FILE_EXISTS_REPLACE);
     
             if ($success) {
    

    try/catch?

19. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
    @@ -120,21 +120,23 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
    -    $final_destination = file_destination($destination, $this->configuration['file_exists']);
    +    /** @var \Drupal\Core\File\FileSystemInterface $file_system */
    +    $file_system = \Drupal::service('file_system');
    +    $final_destination = $file_system->getDestinationFilename($destination, $this->configuration['file_exists']);
     
    

    this has it injected already.

20. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -141,10 +141,11 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
    -    // call and write the file to destination.
    +    // If the directory exists and is writable, avoid
    +    // \Drupal\Core\File\FileSystemInterface::prepareDirectory() call and write
    +    // the file to destination.
         if (!is_dir($dir) || !is_writable($dir)) {
    -      if (!file_prepare_directory($dir, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS)) {
    +      if (!\Drupal::service('file_system')->prepareDirectory($dir, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) {
    

    FileCopy too (there are 3+ calls further down)

21. +++ b/core/modules/simpletest/simpletest.install
    @@ -189,5 +189,5 @@ function simpletest_uninstall() {
       }
       // Delete verbose test output and any other testing framework files.
    -  file_unmanaged_delete_recursive('public://simpletest');
    +  \Drupal::service('file_system')->deleteRecursive('public://simpletest');
    

    try/catch.

22. +++ b/core/modules/simpletest/simpletest.module
    @@ -143,7 +143,7 @@ function simpletest_run_tests($test_list) {
       // Clear out the previous verbose files.
    -  file_unmanaged_delete_recursive('public://simpletest/verbose');
    +  \Drupal::service('file_system')->deleteRecursive('public://simpletest/verbose');
    

    I'd say actually running tests can fail with an exception, likely won't get this far it wouldn't be writable anyway.

23. +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -287,7 +288,7 @@ protected function setUp() {
         // StreamWrapper APIs.
         // @todo Move StreamWrapper management into DrupalKernel.
         // @see https://www.drupal.org/node/2028109
    -    file_prepare_directory($this->publicFilesDirectory, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
    +    \Drupal::service('file_system')->prepareDirectory($this->publicFilesDirectory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS);
    

    unrelated: the @todo points to an issue that was fixed in 2015 :)

24. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1362,7 +1363,7 @@ protected function settingsSet($name, $value) {
       /**
    -   * Ensures test files are deletable within file_unmanaged_delete_recursive().
    +   * Ensures test files are deletable within \Drupal::service('file_system')->deleteRecursive().
        *
    

    should reference the interface instead of \Drupal::service()

    Also > 80 characters, maybe rewrite to avoid the exact function call and put a @see or so?

25. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
    @@ -442,13 +442,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
         if (!empty($values['logo_upload'])) {
    -      $filename = file_unmanaged_copy($values['logo_upload']->getFileUri());
    +      $filename = $file_system->copy($values['logo_upload']->getFileUri());
           $values['default_logo'] = 0;
           $values['logo_path'] = $filename;
         }
         if (!empty($values['favicon_upload'])) {
    -      $filename = file_unmanaged_copy($values['favicon_upload']->getFileUri());
    +      $filename = $file_system->copy($values['favicon_upload']->getFileUri());
           $values['default_favicon'] = 0;
    

    try/catch?

26. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
    @@ -243,7 +243,7 @@ public function save($destination) {
         // Move temporary local file to remote destination.
         if (isset($permanent_destination) && $success) {
    -      return (bool) file_unmanaged_move($destination, $permanent_destination, FILE_EXISTS_REPLACE);
    +      return (bool) \Drupal::service('file_system')->move($destination, $permanent_destination, FILE_EXISTS_REPLACE);
         }
    

    inject? Was surprised it wasn't already, there are some calls to \Drupal::service() already as well as other deprecated file system methods.

27. +++ b/core/modules/system/system.module
    @@ -1348,7 +1349,7 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
           ->getBody();
    -    $local = $managed ? file_save_data($data, $path, $replace) : file_unmanaged_save_data($data, $path, $replace);
    +    $local = $managed ? file_save_data($data, $path, $replace) : \Drupal::service('file_system')->saveData($data, $path, $replace);
    

    another catch? this is already in a try/catch, so should be easy to add.

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

    interesting that this still passes, looks like delete() actually still returns TRUE, despite not having a documented return value.

    another such case above. and the deleteRecursive() tests will also be affected when changing this.

29. +++ b/core/tests/Drupal/KernelTests/Core/File/FileDeleteTest.php
    @@ -37,7 +39,8 @@ public function testDirectory() {
         // Try to delete a directory
    -    $this->assertFalse(file_unmanaged_delete($directory), 'Could not delete the delete directory.');
    +    $this->expectException(NotRegularFileException::class);
    +    $this->assertFalse(\Drupal::service('file_system')->delete($directory), 'Could not delete the delete directory.');
         $this->assertTrue(file_exists($directory), 'Directory has not been deleted.');
    

    the assertFalse() is bogus now, the asserTrue() means lost test coverage, we make sure that we still don't delete the directory.

30. +++ b/core/tests/Drupal/KernelTests/Core/File/FileMoveTest.php
    @@ -48,7 +53,8 @@ public function testNormal() {
         // Move non-existent file.
    -    $new_filepath = file_unmanaged_move($this->randomMachineName(), $this->randomMachineName());
    +    $this->expectException(FileNotExistsException::class);
    +    $new_filepath = \Drupal::service('file_system')->move($this->randomMachineName(), $this->randomMachineName());
         $this->assertFalse($new_filepath, 'Moving a missing file fails.');
    

    the assertFalse() below is dead code.

31. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -415,7 +415,7 @@ protected function setUp() {
       /**
    -   * Ensures test files are deletable within file_unmanaged_delete_recursive().
    +   * Ensures test files are deletable within \Drupal::service('file_system')->deleteRecursive().
    

    another reference to \Drupal::service() and > 80 characters.

Thanks @Berdir. I agree with the general approach:

• Methods on FileSystemInterface throw exceptions
• Wrapper methods in file.module etc try/catch and return booleans

I've addressed most of your feedback, although there were a few hairy ones which I've outlined below.

1. Fixed. I've held off changing prepareDirectory() to throw exceptions in this patch, as its a big change, and I wanted to isolate it.
2. Fixed
3. This is a big change, as it's very low level and used in a number of config and bootstrap related places. I held off on making this change.
4. Fixed
5. Fixed
6. Fixed
7. Fixed, but I left the comment in place as it does actually describe what we are doing (I think?).
8. Fixed
9. Fixed. Created a follow up #3023015: Replace usages of deprecated constants in file.inc
10. Fixed. I reverted previous CopyTest and MoveTest changes.
11. Fixed. Added try/catch to file_save_data()
12. Fixed
13. Fixed.
14. Fixed
15. Fixed
16. Fixed
17. Fixed
18. Fixed
19. Fixed
20. Fixed
21. Fixed
22. Left it as is, as it's throwing an exception.
23. Left it as is.
24. Fixed
25. Fixed
26. Fixed
27. Fixed
28. Fixed. I wrapped in a try/catch and we now do the assert.
29. Fixed as above
30. Fixed. Removed
31. Fixed

Fixed a leftover issue playing around with FileStorage.

Looks like I accidentally reverted changes in install.core.inc

Thanks for all the work.

Not sure if something needs to be done about #135 now, the error vs exception constant for example is a fun one. The constants names might be OK otherwise, feedback from someone else would be great.

3. Maybe add it behind a method or so to limit the size of this issue? could still be mocked then if we'd want to.
7. I see, you added a try/catch now, then the comment remains the same, yes.
23. Maybe (yet another) separate issue about this? I'm not sure if we can simply remove the @todo or if there's still something that should be improved and didn't happen in that other issue.

1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -73,7 +74,7 @@ class FileUploadResource extends ResourceBase {
       *
   -   * @var \Drupal\Core\File\FileSystemInterface
   +   * @var \Drupal\Core\File\FileSystem
       */
      protected $fileSystem;
   

   strange change.

2. +++ b/core/modules/media/src/Plugin/media/Source/OEmbed.php
   @@ -382,27 +394,24 @@ protected function getLocalThumbnailUri(Resource $resource) {
    
   -    $error_message = 'Could not download remote thumbnail from {url}.';
   -    $error_context = [
   -      'url' => $remote_thumbnail_url,
   -    ];
        try {
          $response = $this->httpClient->get($remote_thumbnail_url);
          if ($response->getStatusCode() === 200) {
   -        $success = \Drupal::service('file_system')->saveData((string) $response->getBody(), $local_thumbnail_uri, FILE_EXISTS_REPLACE);
   -
   -        if ($success) {
   +        try {
   +          $this->fileSystem->saveData((string) $response->getBody(), $local_thumbnail_uri, FileSystemInterface::EXISTS_REPLACE);
              return $local_thumbnail_uri;
            }
   -        else {
   -          $this->logger->warning($error_message, $error_context);
   +        catch (FileException $e) {
   +          $this->logger->warning('Could not download remote thumbnail from {url}.', [
   +            'url' => $remote_thumbnail_url,
   +          ]);
            }
   

   looks like we have nested try/catch here now. could be the existing with an additional catch statement.

   A bit unrelated but in general +1 on the logging change, the existing code can't be parsed by potx.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -174,15 +175,18 @@ protected function writeFile($source, $destination, $replace = FileSystemInterfa
   +        return $this->fileSystem->move($source, $destination, $replace);
   +      }
   +      return $this->fileSystem->copy($source, $destination, $replace);
   +    }
   

   do these methods really still have a return value?

4. +++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
   @@ -243,7 +256,13 @@ public function save($destination) {
   -      return (bool) \Drupal::service('file_system')->move($destination, $permanent_destination, FILE_EXISTS_REPLACE);
   +      try {
   +        $this->fileSystem->move($destination, $permanent_destination, FileSystemInterface::EXISTS_REPLACE);
   +        return TRUE;
   +      }
   

   this does a separate return, like I expected above.

5. +++ b/core/tests/Drupal/KernelTests/Core/File/FileDeleteTest.php
   @@ -38,9 +38,13 @@ public function testDirectory() {
   -    // Try to delete a directory
   -    $this->expectException(NotRegularFileException::class);
   -    $this->assertFalse(\Drupal::service('file_system')->delete($directory), 'Could not delete the delete directory.');
   +    // Try to delete a directory.
   +    try {
   +      \Drupal::service('file_system')->delete($directory);
   +    }
   +    catch (NotRegularFileException $e) {
   +      // Ignore.
   +    }
        $this->assertTrue(file_exists($directory), 'Directory has not been deleted.');
   

   now we're not actually asserting the exception. maybe do a fail after the call to delete(), which shouldn't be executed?

   I was also wondering about the new test class you added, seems like quite a few fail-cases are covered in existing tests, so not quite sure how much we really need those new tests?

I can't seem to find the change that introduced these test fails!

Re #136

• 3. Done
• 23. Removed the comment

Re: #143

1. The method is using the public method on the FileSystem class, which doesn't exist on FileSystemInterface. Not sure if there is a better way to do this?
2. Fixed
3. Fixed
4. OK. Nothing to change
5. Added a fail

Re the test class, I think it was added waaaaay back. Happy to move these somewhere else, or remove completely?

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -259,8 +260,7 @@ public function deleteAll($prefix = '') {
   public function createCollection($collection) {
     return new static(
-      $this->directory,
-      $collection
+      $collection, $this->directory
     );

Looks like the argument order here changed? I guess some left-over from trying to inject the file system service..

Also not yet sure about the approach we use there right now for using the using the file system service ( I know I suggested that). But it doesn't seem to put us in a a good way to move forward. Maybe just assign to a property in the constructor, with a follow-up to allow to inject it?