Don't overwrite media files on import

Hello,

Again thanks for your issues and patches.

Here is my review:

1. +++ b/modules/entity_share_client/src/EventSubscriber/FileEntityInsertSubscriber.php
   @@ -0,0 +1,57 @@
   +      $file_destination = $this->fileSystem->getDestinationFilename($entity->getFileUri(), FileSystemInterface::EXISTS_RENAME);
   

   I think this should be an option because there are use cases where you want your files to be overwritten.

   But an option in the Physical file import processor plugin on the 8.x-3.x. For 8.x-2.x this should stay in your custom code.

2. +++ b/modules/entity_share_client/src/Service/JsonapiHelper.php
   @@ -379,13 +379,14 @@ class JsonapiHelper implements JsonapiHelperInterface {
   +      $file_destination = $entity->getFileUri();
   ...
   -          $result = @file_put_contents($remote_uri, $file_content);
   +          $result = @file_put_contents($file_destination, $file_content);
   

   No problem to change those lines though to help developing code for such use cases.

If you can make a patch for 8.x-3.x this would be amazing :)

For 8.x-2.x, I can commit the two changed lines in JsonapiHelper if you want.

Hello,

Thanks for the patch. Here is a quick reply and review because I currently don't have time for Entity Share (and maybe no more time until the end of the year as I am affected to some clients projects).

+++ b/modules/entity_share_client/src/Plugin/EntityShareClient/Processor/PhysicalFile.php
@@ -68,6 +70,29 @@ class PhysicalFile extends ImportProcessorPluginBase {
+        'rename' => FALSE,

The import_config.schema.yml file https://git.drupalcode.org/project/entity_share/-/blob/8.x-3.x/modules/e... needs to be updated.

Also, even if not required because a default value is provided. I think an update hook to set the default value to existing config will be nice to have.

Also tests should be updated to ensure the different option values are tested.

Thanks again for your patch and your work :)

Hello @perelesnyk and thanks for your work!

Updating the issue status.

I will check when I will have some time.

Hello @perelesnyk,

Here is my review.

1. +++ b/modules/entity_share_client/entity_share_client.install
   @@ -97,3 +98,24 @@ function entity_share_client_update_8201() {
   +function entity_share_client_update_8303() {
   

   I was about to say 8300. But nope, the problem is from entity_share_client_update_8201 which should be 8303... OMG, fortunately the entity_share_client_update_8201 check the previous config structure so it can be renamed.

   So for this new update it should be 8304

2. +++ b/modules/entity_share_client/entity_share_client.install
   @@ -97,3 +98,24 @@ function entity_share_client_update_8201() {
   +      if (isset($settings['physical_file']) && !isset($settings['physical_file']['rename'])) {
   +        $settings['physical_file']['rename'] = 0;
   

   Should it be "FALSE" instead of "0"? As it is a boolean expected.

3. +++ b/modules/entity_share_client/src/Plugin/EntityShareClient/Processor/PhysicalFile.php
   @@ -68,6 +70,29 @@ class PhysicalFile extends ImportProcessorPluginBase {
   +      '#title' => $this->t('Rename imported files with the same name, instead of overwriting'),
   

   Rename files

4. +++ b/modules/entity_share_client/src/Plugin/EntityShareClient/Processor/PhysicalFile.php
   @@ -68,6 +70,29 @@ class PhysicalFile extends ImportProcessorPluginBase {
   +      '#description' => $this->t('If the file with the same name exists, the imported file will be saved as filename_0 or filename_1... etc.'),
   

   If a file with the same name exists, rename the imported file instead of overwriting the existing file. The imported file will be saved as filename_0 or filename_1... etc.

5. +++ b/modules/entity_share_client/src/Plugin/EntityShareClient/Processor/PhysicalFile.php
   @@ -99,14 +124,22 @@ class PhysicalFile extends ImportProcessorPluginBase {
   +      $file_destination = $this->fileSystem->getDestinationFilename($processed_entity->getFileUri(), $file_overwrite_mode);
   

   Does this also mean that if you have one entity with a file attached, if you import this entity twice, it will create 2 files? In this case, I think the automated test can be simplified.

   Or maybe even merged in FileTest

6. +++ b/modules/entity_share_client/tests/src/Functional/FileRenameTest.php
   @@ -0,0 +1,236 @@
   +class FileRenameTest extends EntityShareClientFunctionalTestBase {
   

   I think you can inherit from FileTest.php to avoid duplicated code.

7. +++ b/modules/entity_share_client/tests/src/Functional/FileRenameTest.php
   @@ -0,0 +1,236 @@
   +  public function checkFileAliases($alias_exists = FALSE) {
   

   I think the method should be named checkFileRenamed, otherwise it is confusing I think.

8. +++ b/modules/entity_share_client/tests/src/Functional/FileRenameTest.php
   @@ -0,0 +1,236 @@
   +  public function testRenameParameter() {
   

   TestBasicPull so you can inherit from FileTest

9. +++ b/modules/entity_share_client/tests/src/Functional/FileRenameTest.php
   @@ -0,0 +1,236 @@
   +        'rename' => 1,
   

   As there is a new option, other test classes needs to be updated to have the default configuration explicit.

Also some PHPDoc and coding standards are not OK.

I will try to manually test it and rework tests.

Forget about point 3 and 4, this is not the plugin name directly, only option so it is ok to have a long title.

Ok and point 5 is yes, even with one entity it will create new files.

So I think a warning should be added in the UI.

Testing test rework.

Tests are green @perelesnyk if the last patch is ok for you it will be merged and a new release done :)

THanks for you reply :).

I will take a moment today.

Merged!

Going to make a new release.