deprecate file_save_data, file_copy and file_move and replace with a service

1. +++ b/core/modules/file/file.module
   @@ -448,7 +449,7 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
   -      list($width, $height) = explode('x', $maximum_dimensions);
   +      [$width, $height] = explode('x', $maximum_dimensions);
   
   @@ -488,7 +489,7 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
   -      list($width, $height) = explode('x', $minimum_dimensions);
   +      [$width, $height] = explode('x', $minimum_dimensions);
   

   Out of scope.

2. +++ b/core/modules/file/file.services.yml
   @@ -4,3 +4,11 @@ services:
   +    arguments:
   +      - '@file_system'
   +      - '@stream_wrapper_manager'
   +      - '@entity_type.manager'
   +      - '@config.factory'
   +      - '@current_user'
   

   Why is this not done on a single line?

3. +++ b/core/modules/system/system.module
   @@ -604,7 +605,7 @@ function system_page_attachments(array &$page) {
   -  list($version,) = explode('.', \Drupal::VERSION);
   +  [$version] = explode('.', \Drupal::VERSION);
   

   Out of scope.

4. +++ b/core/modules/file/src/FileCreator.php
   @@ -0,0 +1,118 @@
   +    $uri = $this->fileSystem->saveData($data, $destination, $replace);
   

   This method might throw an \Drupal\Core\File\Exception\FileException only the new interface does not mention this.

1. +++ b/core/modules/file/src/FileCreator.php
   @@ -0,0 +1,118 @@
   +   * {@inheritdoc}
   +   */
   +  public function createFile(string $data, ?string $destination = NULL, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface {
   ...
   +      $destination = $this->configFactory->get('system.file')->get('default_scheme') . '://';
   +    }
   

   Same as in the system_retrieve_file() issue. We removed support for $destination = NULl when we deprecated lots of other unmanaged file API functions and should do it here too, this is an anti-pattern.

2. +++ b/core/modules/file/src/FileCreator.php
   @@ -0,0 +1,118 @@
   +    if (!$this->streamWrapperManager->isValidUri($destination)) {
   ...
   +    }
   +
   

   Interesting, was wondering why FileSystem::saveData() doesn't contain this check, but I think it's because we want to limit file entities to stream wrapper uris, while the lower-level APIs do support copying to any path.

3. +++ b/core/modules/file/src/FileCreator.php
   @@ -0,0 +1,118 @@
   +    $uri = $this->fileSystem->saveData($data, $destination, $replace);
   +    // Create a file entity.
   +    $file = File::create([
   +      'uri' => $uri,
   +      'uid' => $this->currentUser->id(),
   +      'status' => FILE_STATUS_PERMANENT,
   +    ]);
   +    // If we are replacing an existing file re-use its database record.
   +    // @todo Do not create a new entity in order to update it. See
   +    //   https://www.drupal.org/node/2241865.
   +    if ($replace == FileSystemInterface::EXISTS_REPLACE) {
   +      /** @var \Drupal\file\FileStorageInterface $fileStorage */
   +      $fileStorage = $this->entityTypeManager->getStorage('file');
   

   See #2241865: Do not create a new file entity in order to overwrite an existing entity as a related issue that we haven't managed to get fixed in years. Not sure what to do. Updating the code to use that implementation might make it more complicated to land this, not doing it means we'll have to redo that patch.

   That said, that also reminds me that this is not the only managed file API function. Should we really limit this service/issue to just file_save_data() or should we make a more generic FileSystemManaged or whatever we want to call it?

   There's file_move(), file_copy() and... actually, that's it. The others are different scopes.. validate or upload related or hooks and small helper functions that aren't actually doing any IO.

   Not sure how to name it, but I think it would be easier to do them all at once, then we don't need to juggle patches and constructor changes between different issues. And risk only getting some changes into the same minor version and need BC and stuff.

   Thinking more about the name, FileEntityWriter or something in that direction might work, copy/move/save is all about saving files. We managed to almost entirely remove the term "unmanaged files" from core, just a few comment leftovers from what I see, so I think we should do the same with the term "managed files", aka _not_ use ManagedFileWriter or so.

4. +++ b/core/modules/file/tests/src/Kernel/FileCreatorTest.php
   @@ -125,15 +141,21 @@ public function testExistingReplace() {
    
        // Check the overwrite error.
   -    $result = file_save_data('asdf', $existing->getFileUri(), FileSystemInterface::EXISTS_ERROR);
   -    $this->assertFalse($result, 'Overwriting a file fails when FileSystemInterface::EXISTS_ERROR is specified.');
   +    try {
   +      $this->fileCreator->createFile('asdf', $existing->getFileUri(), FileSystemInterface::EXISTS_ERROR);
   +      $this->fail('expected FileExistsException');
   +    }
   +    catch (FileExistsException $e) {
   +      // expected exception.
   +      $this->assertStringContainsString("could not be copied because a file by that name already exists in the destination directory", $e->getMessage());
   +    }
        $this->assertEquals($contents, file_get_contents($existing->getFileUri()), 'Contents of existing file were unchanged.');
    
   

   Hm, I guess the hook stuff below prevents us from using expectsException()? Makes me wonder if that's really worth testing when we have an exception, can we assume that this will abort early? Not sure.

5. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
   @@ -0,0 +1,23 @@
   +  public function testSaveData() {
   +    $this->expectDeprecation();
   

   I guess you didn't quite finish here ;)

6. +++ b/core/modules/system/system.module
   @@ -1174,6 +1175,8 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
      $file_system = \Drupal::service('file_system');
   +  /** @var \Drupal\file\FileCreatorInterface $file_creator */
   +  $file_creator = \Drupal::service('file.creator');
      if (!isset($destination)) {
        $path = file_build_uri(\Drupal::service('file_system')->basename($parsed_url['path']));
   

   As you can see in the test fail, we have a hidden dependency on file.module here, so you need to put the \Drupal::service() call inline into the condition or make it an if/else.

The patch looks good to me now.

The only problem that is left for me it naming of the new class:

+++ b/core/modules/file/src/FileEntityWriter.php
@@ -0,0 +1,103 @@
+namespace Drupal\file;
...
+class FileEntityWriter implements FileEntityWriterInterface {

I am not really happy with the name Drupal\file\FileEntityWriter. Not sure if just Drupal\file\FileEntity is better, then we are getting very close to Drupal\file\Entity\File. I am guessing that naming things in programming is hard. :-)

Could we do an inventory of what the new class is going to do. Is it only going to do what it is now doing or are we going to add more methods to it?

I recommend to have FileEntity in there to separate from Drupal\Core\File, but not feeling too strongly about it. I would like to get rid of the "Managed" term as mentioned above.

+++ b/core/modules/file/src/FileEntityWriter.php
@@ -0,0 +1,103 @@
+  public function writeFile(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface {

Could we maybe change the method name to "write"? Or are we going to write something else in this class beside files?

Was wondering about that too. maybe writeData()? We're not going to write something else else than files, but we might write/save from different sources. the managed system_retrieve_file() _could_ be a writeFromUrl method on this service too?

+1 for "writeData()"

All code changes look good to me.
The function has been deprecated and testing for it has also been added.
For me it is RTBC.

I'm not sure if we should move the move and copy functions in this issue as well. Doing it later might be more complicated for BC if we need to expand the interface, assuming we don't manage to do it before 9.3.

I'm not sure if we should move the move and copy functions in this issue as well. Doing it later might be more complicated for BC if we need to expand the interface, assuming we don't manage to do it before 9.3.

Can we add the issues for that to the related issues?

I don't think we have issues yet for that. I guess I'm kind of asking whether we should do it here or create those issues :)

It would be good to get a rough idea of what the follow-up will look like before committing here, for the same reasons as @Berdir lists in #21

Moving back to needs review. If it makes the overall change simpler to review, then doing all three functions at once seems sensible, we can always split again later.

1. +++ b/core/modules/file/src/FileEntityInterface.php
   @@ -0,0 +1,138 @@
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   *   Thrown when an error occurs updating the file storage.
   ...
   +  public function copy(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   

   I am not sure if this can happen. If so, can you explain from were? If so, the function file_copy() does not throw this exception and it also does not catch it. Therefor something does not add up.

2. +++ b/core/modules/file/src/FileEntityInterface.php
   @@ -0,0 +1,138 @@
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   *   Thrown when an error occurs updating the file storage.
   ...
   +  public function move(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   

   Same problem as with copy().

3. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -130,10 +146,15 @@ public function testExistingError() {
   +      $result = $this->fileEntity->copy(clone $source, $target->getFileUri(), FileSystemInterface::EXISTS_ERROR);
   ...
   +    catch (FileExistsException $e) {
   

   The docblock of the method copy() does not say anything about a FileExistsException.

4. +++ b/core/modules/file/tests/src/Kernel/FileEntityTest.php
   @@ -125,15 +118,21 @@ public function testExistingReplace() {
   +      $this->fileEntity->writeData('asdf', $existing->getFileUri(), FileSystemInterface::EXISTS_ERROR);
   ...
   +    catch (FileExistsException $e) {
   

   Same problem as with copy().

5. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
   @@ -0,0 +1,61 @@
   +  public function testCopy() {
   ...
   +    // Check the return status and that the contents changed.
   ...
   +    $this->assertEquals($contents, file_get_contents($result->getFileUri()));
   

   You say that the contents changed and you are asserting that they are equal?

6. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
   @@ -0,0 +1,61 @@
   +  public function testMove() {
   ...
   +    // Check the return status and that the contents changed.
   ...
   +    $this->assertEquals($contents, file_get_contents($result->getFileUri()));
   

   Same as with testCopy().

For #28.1: So they are missing from the docblock of file_copy(). Maybe we should add them.

For #28.2: So they are missing from the docblock of file_move(). Maybe we should add them.

For #28.3 and #28.4: Could we add a comment with that the exception FileExistsException is also a FileException.

I have just 1 point left:

+++ b/core/modules/file/src/FileEntityInterface.php
@@ -0,0 +1,138 @@
+   * @throws \Drupal\Core\File\Exception\InvalidStreamWrapperException
+   *   If the destination is an invalid stream wrapper.
+   * @throws \Drupal\Core\Entity\EntityStorageException
+   *   If the file entity could not be saved.
+   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
+   *   Thrown if the entity type doesn't exist.
+   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
+   *   Thrown if the storage handler couldn't be loaded.
...
+  public function writeData(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
...
+   * @throws \Drupal\Core\Entity\EntityStorageException
+   *   Thrown when an error occurs updating the file storage.
+   * @throws \Drupal\Core\File\Exception\InvalidStreamWrapperException
+   *   Thrown when the destination is an invalid stream wrapper.
...
+  public function copy(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
...
+   * @throws \Drupal\Core\Entity\EntityStorageException
+   *   Thrown when an error occurs updating the file storage.
+   * @throws \Drupal\Core\File\Exception\InvalidStreamWrapperException
+   *   Thrown when the destination is an invalid stream wrapper.
...
+  public function move(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;

I am missing testing for all those exceptions.

Patch looks good. Just one nitpick left for me. After that is it RTBC for me.

+++ b/core/modules/file/src/FileEntity.php
@@ -0,0 +1,229 @@
+  public function move(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface {
...
+      $existing = $this->loadFirstByUri($uri);
+      if ($existing) {
...
+  protected function createOrUpdate(FileInterface $file, string $destination, int $replace, string $uri): void {
...
+      $existing = $this->loadFirstByUri($uri);
+      if ($existing) {

For both: can we do this on a single line.

By the way: adding the new 2 helper methods loadFirstByUri() and createOrUpdate() is a great idea!

All code changes look good to me.
The functions are deprecated and have deprecation message testing.
The IS and the CR are in order.
The new service has testing added.
For me it is RTBC.

@kim.pepper: Great work!

A bit of feedback on the new API, didn't look at the full patch. Crosspost with #38, didn't see the RTBC before I posted it, but I think we can improve things a bit more.

1. +++ b/core/modules/file/src/FileEntity.php
   @@ -0,0 +1,227 @@
   +   */
   +  protected function loadFirstByUri(string $uri): ?FileInterface {
   +    try {
   +      $fileStorage = $this->entityTypeManager->getStorage('file');
   +    }
   +    catch (InvalidPluginDefinitionException | PluginNotFoundException $e) {
   +      // Simplify the API by wrapping exceptions.
   +      throw new EntityStorageException("Failed to load file storage.", 0, $e);
   +    }
   +    /** @var \Drupal\file\FileInterface[] $files */
   

   I know we have those exceptions, but they're all pretty bogus here and are just inherited from generic plugin managers. They would be thrown if the file entity type does not exist or is invalid, but it's almost impossible to get here without a valid file entity type definition, at least for copy/move since you are passing in a file entity already :)

2. +++ b/core/modules/file/src/FileEntity.php
   @@ -0,0 +1,227 @@
   +
   +  /**
   +   * Create or update a file entity according to replace behaviour.
   +   *
   +   * @param \Drupal\file\FileInterface $file
   +   *   The file entity.
   +   * @param string $destination
   +   *   The destination.
   +   * @param int $replace
   +   *   The replace behavior when the destination file already exists.
   +   * @param string $uri
   +   *   The file URI.
   +   *
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   *   Thrown when there is a file entity storage error.
   +   */
   +  protected function createOrUpdate(FileInterface $file, string $destination, int $replace, string $uri): void {
   +    // If we are replacing an existing file re-use its database record.
   +    // @todo Do not create a new entity in order to update it. See
   +    //   https://www.drupal.org/node/2241865.
   +    if ($replace == FileSystemInterface::EXISTS_REPLACE) {
   +      if ($existing = $this->loadFirstByUri($uri)) {
   

   With all that refactoring, I'd really prefer if we'd just fix that problem. Because this current architecture makes it in fact much harder to do so because you require that create or update receives a file entity already.

   What if we pass in a URI instead and create the file entity only if no existing one was found?

   And I'm thinking about making this method public and part of the API. Because then we can IMHO just document that for the system_retrieve_file() with a managed file use case (which is quite rare) that you just let it create an unmanaged file and then call createOrUpdate($uri).

3. +++ b/core/modules/file/src/FileEntityInterface.php
   @@ -0,0 +1,134 @@
   +use Drupal\Core\File\FileSystemInterface;
   +
   +/**
   + * Writes new file entities from data.
   + */
   +interface FileEntityInterface {
   +
   

   I'm confused about the new interface/class name, initially I thought you moved those methods to the file entity class.

   Why switch to FileEntity/FileEntityInterface? move/copy is IMHO still about writing/copying data, so the Writer part isn't wrong?

   That said, this service is actually not really writing anything, we leave that to the underlying file system API. This really is about file storage operations.

   We've been discussion about repository services for entitys for years, so why not go with that? FileRepositoryInterface.

4. +++ b/core/modules/file/src/FileEntityInterface.php
   @@ -0,0 +1,134 @@
   +   * @throws \Drupal\Core\Entity\EntityStorageException
   +   *   Thrown when there is an error updating the file storage.
   

   "updating the file storage" sounds strange to me. IMHO "file storage" is the handler that is responsible for saving, we don't update that. Maybe just "when there is an error saving the file" (doesn't really sound like proper english to me just like now, but not sure)

1. +++ b/core/modules/file/src/FileRepositoryInterface.php
   @@ -0,0 +1,157 @@
   +  public function createOrUpdate(string $uri, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   

   As we make this method part of the public API, we should also add some testing for the method.

2. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,212 @@
   +    // Inform modules that the file has been copied.
   +    $this->moduleHandler->invokeAll('file_copy', [$file, $source]);
   +    return $file;
   

   Nitpick: Could we add an empty line before the return statement.

For #41: To me it is a helper function, but feel free to disagree.

> For #41: To me it is a helper function, but feel free to disagree.

FWIW, the idea of those repository services is exactly this: providing commonly used helpers (like user load by mail and so on for users) around entity crud operations. nothing that we do here is rocket science or impossible without this service.

Given that, I'd say why not? loading by uri is a pretty common use case for files. There are more such calls in file.module and other places, file_file_download() (although that is actually a loop, I think that's due to case sensitivity problems) and _file_save_upload_single().

Also just found #685818: Provide an API function to convert a URI to a file object, which we could close as a duplicate of the new public createOrUpdate() method. this exists in file_entity.module as file_uri_to_object().

If you both think loadFirstByUri() should be part of the public API, then lets do that!

Maybe? I'm actually unsure about the current state of the case sensitive behavior with our different database backends and if that still is an issue or not.

Including the loop in that method would be a minimal overhead even if it's not strictly necessary anymore and we could use it in those methods too without having to spend a lot of time to investigate whether or not it's required?

I also know that the URI isn't actually enforced to be unique and people have actual duplicate managed file entries too, but that's a separate topic.

Patch looks good. Just a couple of nitpicks:

1. +++ b/core/modules/file/file.module
   @@ -643,19 +579,10 @@ function file_theme() {
   -  $files = \Drupal::entityTypeManager()->getStorage('file')->loadByProperties(['uri' => $uri]);
   
   +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -345,7 +345,9 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    $fileRepository = \Drupal::service('file.repository');
   +    $file = $fileRepository->writeData($data, $destination, FileSystemInterface::EXISTS_ERROR);
   
   +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -55,7 +55,9 @@ protected function setUp(): void {
   +    $fileRepository = \Drupal::service('file.repository');
   +    $file = $fileRepository->writeData($data, "public://");
   
   +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
   @@ -298,7 +298,9 @@ public function testHandleFileMunge() {
   +    $fileRepository = \Drupal::service('file.repository');
   +    $this->image = $fileRepository->move($this->image, $original_uri . '.foo.' . $this->imageExtension);
   
   @@ -324,7 +326,7 @@ public function testHandleFileMunge() {
   +    $this->image = $fileRepository->move($this->image, $original_uri . '.foo2.' . $this->imageExtension);
   
   +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
   @@ -430,7 +430,9 @@ public function testHandleFileMunge() {
   +    $fileRepository = \Drupal::service('file.repository');
   +    $this->image = $fileRepository->move($this->image, $original_image_uri . '.foo.' . $this->imageExtension);
   
   @@ -477,7 +479,7 @@ public function testHandleFileMunge() {
   +    $this->image = $fileRepository->move($this->image, $original_image_uri . '.foo.txt.' . $this->imageExtension);
   
   @@ -498,7 +500,7 @@ public function testHandleFileMunge() {
   +    $this->image = $fileRepository->move($this->image, $original_image_uri . '.php.' . $this->imageExtension);
   
   @@ -538,7 +540,7 @@ public function testHandleFileMunge() {
   +    $this->image = $fileRepository->move($this->image, $original_image_uri . '.cgi.' . $this->imageExtension . '.txt');
   

   Should the variable name not be: $file_repository.

2. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -2,43 +2,48 @@
   +   * Tests the writeFile() function.
   ...
      public function testWithFilename() {
   

   Could we add or replace with: "@covers ::writeFile". Same for the other test methods in this class. It is better for the coverage report generator.

3. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -2,8 +2,13 @@
     * Tests the file copy function.
   
   @@ -12,6 +17,21 @@
    class CopyTest extends FileManagedUnitTestBase {
   

   Could we add or replace with: "@coversDefaultClass \Drupal\file\FileRepository". Same for the test method with "@covers ::copy".

4. +++ b/core/modules/file/tests/src/Kernel/MoveTest.php
   @@ -3,8 +3,13 @@
     * Tests the file move function.
   
   @@ -13,6 +18,21 @@
    class MoveTest extends FileManagedUnitTestBase {
   

   Could we add or replace with: "@coversDefaultClass \Drupal\file\FileRepository". Same for the test method with "@covers ::move".

All code changes look good to me.
The 3 functions are deprecated with deprecation message testing.
The new methods have testing.
The IS and the CR are i order.
For me it is RTBC.

@kim.pepper: Great work.

Adding review credits for daffie and berdir

The momentum on this issue has been amazing, awesome work folks

Will try to review next week

This is looking great, just a couple of minor questions/observations from first quick pass

1. +++ b/core/modules/file/file.module
   @@ -110,17 +111,29 @@ function file_field_widget_info_alter(array &$info) {
   + * @throws \Drupal\Core\Entity\EntityStorageException
   + *   Thrown when there is an error updating the file storage.
   
   @@ -194,60 +176,37 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FileSy
   + * @throws \Drupal\Core\Entity\EntityStorageException
   + *   Thrown when there is an error updating the file storage.
   
   @@ -537,56 +496,33 @@ function file_validate_image_resolution(FileInterface $file, $maximum_dimensions
   + * @throws \Drupal\Core\Entity\EntityStorageException
   + *   Thrown when there is an error updating the file storage.
   

   Did we always throw this and are now just documenting it - or is this an API change?

2. +++ b/core/modules/file/file.module
   @@ -110,17 +111,29 @@ function file_field_widget_info_alter(array &$info) {
   +  $file_repository = \Drupal::service('file.repository');
   

   The change notices says file.entity, but it's file.repository now, so we need to update the change record

3. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,235 @@
   +      $file = $source->createDuplicate();
   ...
   +    $file = clone $source;
   

   In once case we use clone, but in the other we use ::createDuplicate

   Should we be using ::createDuplicate everywhere?

4. +++ b/core/modules/file/tests/src/Kernel/CopyTest.php
   @@ -2,18 +2,41 @@
   +   * The file entity service under test.
   
   +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -0,0 +1,258 @@
   +   * The file entity service under test.
   
   +++ b/core/modules/file/tests/src/Kernel/MoveTest.php
   @@ -3,18 +3,41 @@
   +   * The file entity service under test.
   

   still using the old name here in the comment too

5. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -59,6 +60,13 @@ class FileUploadForm extends AddFormBase {
   +   * The file entity service.
   

   here too

1. It's not new. We would have to add throws EntityStorageException to 50% of our API, because anything that saves an entity could theoretically do that. And we do that a lot ;)

3. One is a copy, so a new file entity, the other is move. I think the clone on move is done to not change $source, in case you want to compare against that (did filename change or something like that), so I think that makes sense.

Need work for the comment updates :)

Also, in regards to credits, not sure what you want to do about the related issues that this is resolving:

* #685818: Provide an API function to convert a URI to a file object
* #2241865: Do not create a new file entity in order to overwrite an existing entity

Both have been open for many years and had plenty of people contributing over that time.

Also, in regards to credits, not sure what you want to do about the related issues that this is resolving:

Yeah we should add those folks here if they're likely to be closed as duplicates.

All points of @larowlan have been addressed.
Back to RTBC.

cross-post - added credit for those issues and marked them as duplicates

Are you able to transfer the credit over here and mark that as duplicated? Thanks

Larowlan - looks like you mistakenly credited me for this one instead of Berdir. I'm not sure how to change the credit system, but want to make sure credit goes where credit is due!

Transferring credit from #1048114: file_copy(), $destination cannot be NULL per #77.

Removing @brianV credit per #79.

@brianV: That was not a mistake. You've been credited because you've worked on the now-duplicate #685818: Provide an API function to convert a URI to a file object. Almost 10 years ago :)

While you didn't directly contribute to this issue, we try to take over credits in such a case as you would have been created if that issue would have been committed/fixed on its own.

Went over this more thoroughly, a few minor nits are all that remain, and one question about PHP version support for the tests

1. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -0,0 +1,258 @@
   +   * The file entity service under test.
   

   old name here

2. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -0,0 +1,258 @@
   +    $contents = $this->randomMachineName(8);
   ...
   +    $contents = $this->randomMachineName(8);
   ...
   +    $contents = $this->randomMachineName(8);
   ...
   +    $contents = $this->randomMachineName(8);
   

   the 8 here isn't needed, its the default

3. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -0,0 +1,258 @@
   +    $result = $this->fileRepository->writeData($contents, $existing->getFileUri(), FileSystemInterface::EXISTS_RENAME);
   

   rename is the default, so not needed here

4. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
   @@ -0,0 +1,61 @@
   +    $this->assertFileDoesNotExist($source->getFileUri());
   

   assertFileDoesNotExist seems to be PHPUnit 9+ - does that cause issues for getting this passing on lower versions of PHP? We should be able to queue up a lower version of PHP to check.

Looks all points addressed, back to RTBC (hope CI will be green)

Green and ready

Need a reroll of the patch - there have been a couple of changes to file.module since the patch was made.

+++ b/core/modules/file/src/FileRepository.php
@@ -0,0 +1,235 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function loadOrCreateByUri(string $uri): FileInterface {
+    if ($file = $this->loadByUri($uri)) {
+      return $file;
+    }
+    $file = File::create(['uri' => $uri]);
+    $file->setOwnerId($this->currentUser->id());
+    return $file;
+  }

The only usage of this is in the very similarly named and protected createOrUpdate on this class. Why are we adding this here and how is this additional API justified? Oh I see we're trying solve a twelve year old issue in a refactor #685818: Provide an API function to convert a URI to a file object. I'm not sure this is a great idea. Firstly, because we adding new API in a refactor so the new API never gets the discussion it deserves. Secondly, because while functions that load or create always seem like a good idea at the start they often seem to back us into complex corners. For example, with Config objects you need to check the isNew() more often than you'd like. With the function added here - if the file doesn't exist then the caller is responsible for calling ->save() but if it does exist then you shouldn't call save because that'd result in unnecessary writes to the database.

The method loadOrCreateByUri() has been removed from the patch.
I have updated the CR and the IS.
Back to RTBC.

+++ b/core/modules/file/src/FileRepository.php
@@ -116,7 +116,14 @@ public function writeData(string $data, string $destination, int $replace = File
   protected function createOrUpdate(string $uri, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface {
-    $file = $this->loadOrCreateByUri($uri);
+    if ($file = $this->loadByUri($uri)) {
+      $file->setPermanent();
+      $file->save();
+      return $file;
+    }
+    $file = File::create(['uri' => $uri]);
+    $file->setOwnerId($this->currentUser->id());
+
     if ($replace == FileSystemInterface::EXISTS_RENAME && is_file($destination)) {

Hm.

I'd suggest that if we don't do it here and now then we just won't fix the other issue. we have loadByUri() now, and it's fairly simple then. Adding it later to this repository isn't work the BC dance that it would require.

That said, I'm wondering where the permanent call here is coming from, loadOrCreate didn't do that? If you move a temporary file that shouldn't automatically become permanent?

Also, not sure if the setOwner is necessary. We have the default value callback now on file entities, that should automatically happen.

+1 to those names - but lets get a second +1 before reworking it

1. +++ b/core/modules/file/src/FileRepositoryInterface.php
   @@ -0,0 +1,145 @@
   +  public function writeData(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   ...
   +  public function copy(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   ...
   +  public function move(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   ...
   +  public function loadByUri(string $uri): ?FileInterface;
   

   I think it needs @Berdir as current names works for me

2. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -364,7 +364,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   -        $file = file_move($image, $destination);
   +        $file = \Drupal::service('file.repository')->move($image, $destination);
   

   I bet it will need DI

I think it's ready for commiter's eyes

Thanks @larowlan who pointed that static method cant use injections - re #105.2

I'm reviewing this now. Found some problems. I'll post the full comment and a patch shortly.

This mostly looks really great. Thanks to everyone who worked on it so far, especially @kim.pepper! Most of what I could find are docs nitpicks, fixed with the attached patch. The unaddressed points are:

2. Point A is fixed, but B + C are open questions
4. Minor, probably unneeded.
5. Minor, asking it to see if I'm missing something.
6. This is the only concern of any substance.

Everything else is now fixed or informational. But I'll ping @kim.pepper in Slack to sort out 6.

Thanks!
-Derek

1. +++ b/core/modules/file/file.module
   @@ -194,60 +176,37 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FileSy
   +      \Drupal::logger('file')->notice('File %file (%realpath) could not be moved because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', ['%file' => $source->getFileUri(), '%realpath' => $realpath, '%destination' => $destination]);
   

   Why is this talking about file_copy() when we're inside file_move()?

2. +++ b/core/modules/file/file.module
   @@ -194,60 +176,37 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FileSy
   -      \Drupal::logger('file')->notice('File %file could not be moved because the destination %destination is invalid. This may be caused by improper use of file_move() or a missing stream wrapper.', ['%file' => $source->getFileUri(), '%destination' => $destination]);
   +      \Drupal::logger('file')->notice('File %file could not be moved because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', ['%file' => $source->getFileUri(), '%destination' => $destination]);
   

   A. Why is the text of this message changing? In particular, why is the log message from file_move() now talking about file_copy()?

   B. Is there anything other than the lack of "(%realpath)" we can/should say differently in this case?

   C. If we're changing the text of this for valid reasons, do we want to start wrapping the filename in double quotes (like we're tending to do with new messages in other issues)?

3. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,228 @@
   +   * FileCreator constructor.
   

   Whoops. Now "FileRepository".

4. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,228 @@
   +      throw new InvalidStreamWrapperException(sprintf('Invalid stream wrapper: %destination', ['%destination' => $destination]));
   ...
   +      throw new InvalidStreamWrapperException(sprintf('Invalid stream wrapper: %destination', ['%destination' => $destination]));
   

   Should these say "Invalid destination stream wrapper" or something? Mention "destination"? We already know it's an InvalidStreamWrapperException, seems the text could be more helpful than repeating that?

5. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,228 @@
   +   * @return \Drupal\file\Entity\File|\Drupal\file\FileInterface
   

   Why the | here? Why not just FileInterface and be done?

6. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,228 @@
   +   *   - FileSystemInterface::EXISTS_ERROR: Do nothing and return FALSE.
   

   If EXISTS_ERROR causes this to return FALSE, we don't say that's allowed by docs or return type.

   Doesn't look like we actually handle EXISTS_ERROR in createOrUpdate() at all. Maybe it doesn't matter, since we currently only call it in a method that would have already thrown the exception. But it seems a little fishy to document that as a valid choice but then not handle it. Should we throw the exception ourselves here?

7. +++ b/core/modules/file/src/FileRepository.php
   @@ -0,0 +1,228 @@
   +    if ($replace == FileSystemInterface::EXISTS_RENAME && is_file($destination)) {
   ...
   +    if ($replace == FileSystemInterface::EXISTS_REPLACE && $existing = $this->loadByUri($uri)) {
   

   === ?

8. +++ b/core/modules/file/src/FileRepositoryInterface.php
   @@ -0,0 +1,145 @@
   + * Writes new file entities from data.
   

   This interface does more than that, now. Can we do better? How about:

   "Performs file system operations and updates database records accordingly."

   ?

9. +++ b/core/modules/file/src/FileRepositoryInterface.php
   @@ -0,0 +1,145 @@
   +   *   - FileSystemInterface::EXISTS_ERROR: Do nothing and return FALSE.
   +   *
   +   * @return \Drupal\file\FileInterface
   +   *   The file entity.
   ...
   +  public function writeData(string $data, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
   

   If EXISTS_ERROR causes this to return FALSE, we don't say that's allowed by docs or return type.

   It looks like the end result will be a FileExistsException thrown by FileSystem::prepareDestination():

       // Determine whether we can perform this operation based on overwrite rules.
       $destination = $this->getDestinationFilename($destination, $replace);
       if ($destination === FALSE) {
         $this->logger->error("File '%original_source' could not be copied because a file by that name already exists in the destination directory ('%destination').", [
           '%original_source' => $original_source,
           '%destination' => $destination,
         ]);
         throw new FileExistsException("File '$original_source' could not be copied because a file by that name already exists in the destination directory ('$destination').");
       }
   

10. +++ b/core/modules/file/src/FileRepositoryInterface.php
    @@ -0,0 +1,145 @@
    +   * @throws \Drupal\Core\File\Exception\FileException
    +   *   Thrown when there is an error writing to the file system.
    

    We should probably also mention FileExistsException in this list for each of the methods in this interface...

11. +++ b/core/modules/file/src/FileRepositoryInterface.php
    @@ -0,0 +1,145 @@
    +   *   - FileSystemInterface::EXISTS_ERROR: Do nothing and return FALSE.
    +   *
    +   * @return \Drupal\file\FileInterface
    +   *   The file entity.
    ...
    +  public function copy(FileInterface $source, string $destination, int $replace = FileSystemInterface::EXISTS_RENAME): FileInterface;
    

    Same here. Docs are off. FileSystem::copy() calls prepareDirectory() which will throw the FileExistsException for us in this case, so nothing else to fix other than the docs.

12. +++ b/core/modules/file/src/FileRepositoryInterface.php
    @@ -0,0 +1,145 @@
    +   *   - FileSystemInterface::EXISTS_ERROR: Do nothing and return FALSE.
    +   *
    +   * @return \Drupal\file\FileInterface
    +   *   The file entity.
    

    Same here. Docs are off. In this case, FileSystem::move() calls prepareDirectory() which will throw the FileExistsException for us...

13. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
    @@ -2,58 +2,66 @@
    -  public function testWithoutFilename() {
    -    $contents = $this->randomMachineName(8);
    -
    -    $result = file_save_data($contents);
    -    $this->assertNotFalse($result, 'Unnamed file saved correctly.');
    -
    -    $stream_wrapper_manager = \Drupal::service('stream_wrapper_manager');
    -    assert($stream_wrapper_manager instanceof StreamWrapperManagerInterface);
    -    $this->assertEquals(\Drupal::config('system.file')->get('default_scheme'), $stream_wrapper_manager::getScheme($result->getFileUri()), "File was placed in Drupal's files directory.");
    -    $this->assertEquals(\Drupal::service('file_system')->basename($result->getFileUri()), $result->getFilename(), "Filename was set to the file's basename.");
    -    $this->assertEquals($contents, file_get_contents($result->getFileUri()), 'Contents of the file are correct.');
    -    $this->assertEquals('application/octet-stream', $result->getMimeType(), 'A MIME type was set.');
    -    $this->assertTrue($result->isPermanent(), "The file's status was set to permanent.");
    +  protected $fileRepository;
     
    -    // Check that the correct hooks were called.
    -    $this->assertFileHooksCalled(['insert']);
    

    Did we lose this test coverage? I don't see what happened to it.

    I suppose since it's testing an edge case of a now deprecated API, better to remove the coverage than complicate the new API to support this feature. I suppose we can live without testing file_save_data() between 9.3.0 and 10.0.0...

14. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
    @@ -125,15 +137,24 @@ public function testExistingReplace() {
    +    catch (FileExistsException $e) {
    

    🎉 We've even got test coverage for this exception being thrown. So all we need are the doc updates mentioned above. 😉

15. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
    @@ -0,0 +1,61 @@
    +   * Tests file_save_data deprecation.
    

    Maybe mention this is also testing file_save_data() with no $destination set?

16. +++ b/core/modules/file/tests/src/Kernel/LegacyFileTest.php
    @@ -0,0 +1,61 @@
    +  public function testSaveData() {
    +    $this->expectDeprecation('file_save_data is deprecated in drupal:9.3.0 and will be removed in drupal:10.0.0. Use \Drupal\file\FileRepositoryInterface::writeData() instead. See https://www.drupal.org/node/3223520');
    +    $contents = $this->randomMachineName(8);
    +    $result = file_save_data($contents);
    +    $this->assertNotFalse($result, 'Unnamed file saved correctly.');
    

    Oh slick, there it went! Never mind. 😉

just nit while skimed

+++ b/core/modules/file/file.module
@@ -199,10 +199,10 @@
+      \Drupal::logger('file')->notice('File %file could not be moved because the destination %destination is invalid. This may be caused by improper use of file_move() or a missing stream wrapper.', ['%file' => $source->getFileUri(), '%realpath' => $realpath, '%destination' => $destination]);

The %realpath is missing in message test

re: #110: Good point. Fixed here.

Cool, thanks. I think option 1 is more clear and explicit, and less weird. Even if it's slightly more hassle at the call site. But only a little. ;)

However:

+++ b/core/modules/file/src/FileRepository.php
@@ -101,13 +101,8 @@ public function writeData(string $data, string $destination, int $replace = File
+   * @param bool $rename
+   *   (optional) Whether to rename the file.

@@ -115,14 +110,14 @@ public function writeData(string $data, string $destination, int $replace = File
+  protected function createOrUpdate(string $uri, string $destination, bool $rename): FileInterface {

Doc says optional, but function signature doesn't provide a default value for $rename. I assume we want to default to FALSE? We should make that true both in the signature, and in the docs.

In Slack, I proposed to @kim.pepper that we just drop "(optional)" from the docs and leave it as a required param. He agreed that was best, so here we go.

Re-roll after #3232248: Move _file_save_upload_single to a service and deprecate landed. Interdiff is wildly confused, so I won't bother. We should probably be using an MR for this now, to make such operations more straightforward. /shrug But maybe this is the last re-roll we need? 🤞

Argh, nope, that's not right. The rename from SaveDataTest to LegacyFileTest conflicted with the new LegacyFileTest from #3232248: Move _file_save_upload_single to a service and deprecate and I didn't notice how broken that all was. I'm out of time right now, so I can't do this properly tonight. I'll be traveling all day tomorrow, and probably won't be able to do it. If @kim.pepper doesn't beat me to it, I'll try late tmrw or Friday. Sorry!

It all looks good to me. Just a couple of nitpicks:

1. +++ b/core/modules/file/file.services.yml
   @@ -7,3 +7,6 @@ services:
   +    arguments: [ '@file_system', '@stream_wrapper_manager', '@entity_type.manager', '@module_handler', '@file.usage','@current_user' ]
   

   Nitpick: Insert a space between @file_usage', and '@current_user

2. +++ b/core/modules/file/src/FileRepositoryInterface.php
   @@ -0,0 +1,157 @@
   +   *   - FileSystemInterface::EXISTS_RENAME: (default) Append
   +   *     _{incrementing number} until the filename is unique.
   

   Nitpick: Could we list this possible value first as it is the default value (3 times).

3. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -2,58 +2,66 @@
   -    $this->assertEquals($filename, \Drupal::service('file_system')->basename($result->getFileUri()), 'File was named correctly.');
   +    $this->assertEquals($filename, \Drupal::service('file_system')
   +      ->basename($result->getFileUri()), 'File was named correctly.');
   

   Nitpick: This change is out of scope.

4. +++ b/core/modules/file/tests/src/Kernel/FileRepositoryTest.php
   @@ -143,4 +164,64 @@ public function testEntityStorageException() {
   +
   

   Nitpick: This space at the end of the test method can be removed. The same for testCopy().

5. +++ b/core/modules/file/tests/src/Kernel/MoveTest.php
   @@ -55,7 +80,7 @@ public function testExistingRename() {
   +    $result = $this->fileRepository->move(clone $source, $target->getFileUri(), FileSystemInterface::EXISTS_RENAME);
   

   We can remove the third parameter. It is the default value.

Working on #119 suggestions.

All the code changes look good to me.
The IS and CR are in order.
For me it is RTBC.

+1 RTBC from me. #118 is the correct re-roll that I botched in my haste in #115. #119 are legit points, all fixed by #121. I don't see anything else to complain about.

Also happy to report there's only a tiny, easily resolved conflict in file.services.yml between this and the arguably much more important #2940383: [META] Unify file upload logic of REST and JSON:API, so committing this will not slow us down over there.

Thanks!
-Derek

Saving issue credits

Didn't mean to change status

Tagging for release notes

Committed 6407407 and pushed to 9.3.x. Thanks!

Published the change record.

Awesome work here folks, across a number of issues.

In #132 is says 'Tagging for release notes' and it looks like that was missed. Adding tag.

Updating to Drupal 9.3 breaks for file_save_data if the first parameter was passing a resource instead of a string. This was allowed and worked in Drupal 9.2, but FileRepository->writeData() doesn't allow a file handle as the data param, so I cannot use that...

Also, while trying to port my code to Drupal 9.3, I cannot seem to figure out how to programmatically create a file entity because of this error.

Before:

    $handle = fopen($file->uri, 'r');
    if ($handle) {
      /** @var \Drupal\file\FileInterface|false $new_file */
      $new_file = file_save_data($handle, $destination);
      fclose($handle);
    }
    return $new_file;

After...?

    $handle = fopen($file->uri, 'r');
    if ($handle) {
      // The file is copied from an external mount, and it doesn't have a file
      // entity. So the file must be opened and saved, rather than copied using
      // FileRepository->copy().
      $uri = $this->fileSystem->saveData($handle, $destination);

      // DOESN'T WORK!
      // Intelephense error because the createOrUpdate function is protected.
      $new_file = $this->fileRepository->createOrUpdate($uri, $destination, FileSystemInterface::EXISTS_REPLACE);

      fclose($handle);
    }
    return $new_file;

Agreed, that was not intentionally supported and not tested. Somewhat unfortunate, but you will have to either read the file content with file_get_contents() or write your own function to create a file entity.

Okay, thanks.

Here's what I went with, in case anyone else has the same issue. I figured that if support for a resource could be removed from that function, then maybe it won't always be supported for $this->fileSystem->saveData(), as the docblock there says string as well. So I used $this->fileSystem->copy() and check if it's readable first, instead of opening the file.

    if (is_readable($file->uri)) {
      // The file is copied from an external mount.
      $this->fileSystem->copy($file->uri, $destination, FileSystemInterface::EXISTS_REPLACE);

      // Create and save a new file entity.
      $new_file = File::create(['uri' => $file->uri]);
      $new_file->setOwnerId(1);
      $new_file->setPermanent();
      $new_file->save();
    }
    else {
      $new_file = FALSE;
    }