Reuse option in FileCopy migrate process plugin not work with remote files

After looking at that patch, it would be good to have a "test only" patch. To see the failures. Just grab the test part of the patch and add it to a new patch on that issue. Don't include the fix. We want to see the failure, to verify that we are in fact really testing things.

Adding a tests only patch.

Closed #2712743: File entity destination does not allow partial migrations as duplicate.

It looks like we have a test here. Can we get a fix patch also uploaded?

Looking very good.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
   @@ -118,10 +121,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +    if (!($final_destination)) {
   

   Nit: extra parenthesis are not needed.

The patch works for me, thanks edysmp and heddn! I removed the parenthesis from the patch, as stated in #13.

I like this patch, but I think we should not use a trait for this, since it's really very specifically affecting only two process plugins. Let's convert the trait to an abstract class and have FileCopy and Download extend it.

Converted trait to an abstract class as recommended by @phenaproxima in #15.

Only one request here, but otherwise this is RTBC. Great work!

+++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
@@ -0,0 +1,30 @@
+abstract class FileProcessBase extends ProcessPluginBase {

Let's add a __construct() here which adds default values for 'rename' and 'reuse' to the plugin configuration, just to keep things consistent across all classes which descend from this one.

I think we'll also need a change record, so I'm tagging for that.

Added __construct(). Removed a couple of redundant use statements.

Returning to Needs Work because it stills needs a change record.

+++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
@@ -10,6 +11,34 @@
+   * The file system service.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  protected $fileSystem;

I think we should probably remove this; it presumes implementation details of the child classes. It's sad, but in this case I think it's okay to let both FileCopy and Download bring in the file_system service themselves.

Aww, that's me trying to be too clever. Reverted.

Returning to Needs Work because it stills needs a change record.

Back to NR. Should hopefully be a quick turn around as we were just waiting on a CR.

I can't do that. Back to NR.

Assigning for review

I used this recently on a migration to set the HTTP referer option in the file copy plugin config and passing that along to download. That was just a side effect. The original feature about reusing the files in the destination worked as well.

Nice change. The CR look fine as well.

Just a few thing before RTBC.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
   @@ -85,10 +84,9 @@ class Download extends ProcessPluginBase implements ContainerFactoryPluginInterf
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $file_system);
   
   +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -90,10 +89,8 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $file_system);
   

   FileProcessBase signature uses 3 parameters not 4. Maybe just inject the filesystem in FileProcessBase instead.

2. +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php
   @@ -76,11 +77,16 @@ public function testSuccessfulCopies() {
   +   *
   +   * @param string $source_path
   +   * @param string $destination_path
   

   Needs parameter description

3. +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php
   @@ -101,6 +107,24 @@ public function testSuccessfulReuse() {
   +   * @return array
   

   Needs description

4. +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php
   @@ -101,6 +107,24 @@ public function testSuccessfulReuse() {
   +        'local_destination_path' => 'public://file1.jpg'
   ...
   +        'remote_destination_path' => 'public://file2.jpg'
   

   Needs comma at end

Should be needs work

1. Removed redundant parameter. @phenaproxima advised against injecting the filesystem into FileProcessBase in #20.
2. Added descriptions.
3. Removed @return (unnecessary for data providers).
4. Added commas.

Edit: styling

Thanks, I don't know how I missed phenaproxima's comment.

Off we go!

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
   @@ -85,7 +84,6 @@ class Download extends ProcessPluginBase implements ContainerFactoryPluginInterf
   @@ -118,10 +116,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   
   +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,48 @@
   +   * Constructs a download process plugin.
   

   It's not a download process plugin.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,48 @@
   +/**
   + * Provides functionality for file process plugins.
   + */
   +abstract class FileProcessBase extends ProcessPluginBase {
   

   We need to document what the 'rename' and 'reuse' configuration mean. It's not clear.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,48 @@
   +      'rename' => FALSE,
   +      'reuse' => FALSE,
   

   One thing that is super confusing here is the you can't have both

   'rename' => TRUE,
   'reuse' => TRUE,
   

   That makes no sense. It'll always rename. It would be better to move something like:

   'file_exists' => 'rename'
   

   'file_exists' => 'replace'
   

   'file_exists' => 'use existing'
   

   (since reuse is not that clear either). This can be follow up. BUT the problem is we're adding new stuff to Download so it would be nice to make it better the first time.

1. Corrected the docs.
2. Documented the configuration keys.
3. Created a follow-up (#2979597: Rename configuration keys in FileProcessBase process plugin.), mainly because I'm concerned about the BC implications.

@Jo Fitzgerald but the thing is here we're expanding the configuration options in Download plugin by adding the reuse option. That feels a shame because we're introducing complexity and configuration options that can contradict.

@alexpott I am happy to merge in the patch I have added to the follow-up, but how should the BC issues be addressed?

If the 'reuse' or 'rename' options are set we trigger a deprecation notice. If the 'file_exists' option does not exist then we set it according to the 'reuse' or 'rename' options.

NW for the last few comments.

Replaced $configuration keys 'reuse' and 'replace' with 'file exists' as described in #31.3 and added the BC from #35.

• Corrected the code that was causing deprecation errors.
• Corrected the coding standards errors.

I don't understand the test that is now failing, FileCopyTest::testSuccessfulReuse(), so I'm finding it hard to debug. The assert on line 102 passes if sleep(1) is removed, but then the assert on line 108 fails.

Leaving as Needs Work for someone else to investigate the test failures

+++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
@@ -0,0 +1,76 @@
+    if (in_array('reuse', $config_keys)) {
+      @trigger_error("Using the key 'reuse' is deprecated, use 'file_exists' => 'use existing' instead.", E_USER_DEPRECATED);
+    }
+    if (in_array('rename', $config_keys)) {
+      @trigger_error("Using the key 'rename' is deprecated, use 'file_exists' => 'rename' instead.", E_USER_DEPRECATED);
+    }

This should set the 'file_exists' value appropriately. Also how about array_key_exists('reuse', $configuration) rather than in_array()

Let's have a change record here too, which we can refer to in the deprecation notice.

Made the changes requested by @alexpott in #40, but I still can't work out why the test is failing. Leaving as Needs Work.

Also still needs change record.

I made 2 silly mistakes when editing the test. Both of which are corrected in this patch.

Still needs change record.

Updated the IS a little bit and started a Change Record, Changes to Download and FileCopy process plugin . The CR is really rough and hopefully now that it is started someone else will improve it.

Edit: add link to CR

1. This is looking good in terms of BC and introducing the new tri-state configuration option.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -90,10 +91,8 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $file_system);
   

   I don't think FileProcessBase needs $file_system

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,74 @@
   +      @trigger_error("Using the key 'reuse' is deprecated, use 'file_exists' => 'use existing' instead.", E_USER_DEPRECATED);
   ...
   +      @trigger_error("Using the key 'rename' is deprecated, use 'file_exists' => 'rename' instead.", E_USER_DEPRECATED);
   

   These message need to end with a link to the CR.

4. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,74 @@
   +    if (!isset($this->configuration['file_exists'])) {
   +      if (!empty($this->configuration['rename'])) {
   +        return FILE_EXISTS_RENAME;
   +      }
   +      elseif (!empty($this->configuration['reuse'])) {
   +        return FILE_EXISTS_ERROR;
   +      }
   +      else {
   +        return FILE_EXISTS_REPLACE;
   +      }
   +    }
   

   This code should be unnecessary because the constructor guarantees 'file_exists' is set.

5. There should be a deprecated code unit test that checks that existing configuration correctly sets the 'file_exists' configuration and triggers the expected deprecation notices.

2. Removed file system.
3. Linked to CR.
4. Removed redundant code.
5. Added unit test for deprecation notices.

Getting close. Except for the change record, I'm not seeing much else that needs fixing here.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
   @@ -19,8 +18,12 @@
   + *   - 'rename' - Append _{incrementing number} until the filename is
   + *       unique.
   

   Superdupernit: 'unique' is indented too far.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,63 @@
   + *   - 'rename' - Append _{incrementing number} until the filename is
   + *       unique.
   

   Same here.

3. +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php
   @@ -101,6 +109,22 @@ public function testSuccessfulReuse() {
   +  public function sourceAndDestinationPathProvider() {
   

   Usually we name data providers after the test method, so this should probably be providerSuccessfulReuse().

4. +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php
   @@ -222,6 +246,9 @@ public function testDownloadRemoteUri() {
   +    $this->container->set('http_client', $this->getMock(Client::class));
   

   I think we use createMock() now...?

5. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
   @@ -0,0 +1,69 @@
   +/**
   + * Tests the file copy process plugin.
   + *
   + * @group migrate
   + *
   + * @coversDefaultClass \Drupal\migrate\Plugin\migrate\process\FileCopy
   + */
   +
   +class  FileCopyTest extends MigrateProcessTestCase {
   

   Nit: Let's just tag the entire class with @group legacy, rather than each individual method. There's also an empty line between the doc comment and the class declaration, and there should only be one space after 'class'.

6. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
   @@ -0,0 +1,69 @@
   +   * Tests that the rename configuration key will throw an exception.
   

   Nit: Should be "...will trigger a deprecation notice."

7. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
   @@ -0,0 +1,69 @@
   +    $stream_wrapper_manager = $this->prophesize(StreamWrapperManagerInterface::class)->reveal();
   +    $file_system = $this->prophesize(FileSystemInterface::class)->reveal();
   +    $download_plugin = $this->prophesize(MigrateProcessInterface::class)->reveal();
   +    $this->plugin = new FileCopyExt($configuration, 'test', [], $stream_wrapper_manager, $file_system, $download_plugin);
   

   We can put this stuff in a helper method to avoid repeating it twice.

8. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
   @@ -0,0 +1,69 @@
   +    $this->assertArraySubset(['file_exists' => 'rename'], $this->plugin->getConfiguration());
   

   I didn't know about assertArraySubset()! Nice find, and thanks!

9. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
   @@ -0,0 +1,69 @@
   +   * Tests that the reuse configuration key will throw an exception.
   

   s/throw an exception/trigger a deprecation notice.

10. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
    @@ -0,0 +1,69 @@
    +    $this->plugin->transform('', $this->migrateExecutable, $this->row, '');
    

    Why is this being called?

11. +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php
    @@ -0,0 +1,69 @@
    +class FileCopyExt extends FileCopy implements ContainerFactoryPluginInterface {
    

    Let's follow the existing precedent here and name the class TestFileCopy. Also, FileCopy already implements ContainerFactoryPluginInterface so I don't think we need to use it again here.

Addressed all of @phenaproxima's points in #47.

Only the Change Record to finish now.

This looks great. But a thing occurred to me...

+++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
@@ -0,0 +1,63 @@
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
+    if (array_key_exists('reuse', $configuration)) {
+      @trigger_error("Using the key 'reuse' is deprecated, use 'file_exists' => 'use existing' instead. See https://www.drupal.org/node/2981389.", E_USER_DEPRECATED);
+      $configuration['file_exists'] = 'use existing';
+    }
+    if (array_key_exists('rename', $configuration)) {
+      @trigger_error("Using the key 'rename' is deprecated, use 'file_exists' => 'rename' instead. See https://www.drupal.org/node/2981389.", E_USER_DEPRECATED);
+      $configuration['file_exists'] = 'rename';
+    }
+    $configuration += ['file_exists' => 'replace'];
+    parent::__construct($configuration, $plugin_id, $plugin_definition);
+  }
+
+  /**
+   * Determines how to handle file conflicts.
+   *
+   * @return int
+   *   FILE_EXISTS_REPLACE (default), FILE_EXISTS_RENAME, or FILE_EXISTS_ERROR
+   *   depending on the current configuration.
+   */
+  protected function getOverwriteMode() {
+    switch ($this->configuration['file_exists']) {
+      case 'rename':
+        return FILE_EXISTS_RENAME;
+
+      case 'use existing':
+        return FILE_EXISTS_ERROR;
+
+      default:
+        return FILE_EXISTS_REPLACE;
+    }
+  }

So I just realized something...we don't really need getOverwriteMode(), do we? Couldn't we just set $configuration['file_exists'] to one of the FILE_EXISTS_* constants in __construct(), and use that during processing?

Also, I think we need to extend those array_key_exists() checks so that they only change $configuration['file_exists'] if the 'rename'/'replace' keys are not empty, not just if they are set (since they could be set to FALSE). We should probably cover those cases and extend the test coverage.

To needs work based on feedback in #49.

WIP

Addressed points in #49.

Man, I'm really sorry to do this.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,43 @@
   +    if (array_key_exists('reuse', $configuration) && !empty($configuration['reuse'])) {
   +      @trigger_error("Using the key 'reuse' is deprecated, use 'file_exists' => 'use existing' instead. See https://www.drupal.org/node/2981389.", E_USER_DEPRECATED);
   +      $configuration['file_exists'] = FILE_EXISTS_ERROR;
   +    }
   +    if (array_key_exists('rename', $configuration) && !empty($configuration['rename'])) {
   +      @trigger_error("Using the key 'rename' is deprecated, use 'file_exists' => 'rename' instead. See https://www.drupal.org/node/2981389.", E_USER_DEPRECATED);
   +      $configuration['file_exists'] = FILE_EXISTS_RENAME;
   +    }
   

   I think we need to separate the array_key_exists() check from the empty() check. That's because we do want to trigger the deprecation notice if the array key exists at all, but we only want to change $configuration['file_exists'] if the old key is not empty.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php
   @@ -0,0 +1,43 @@
   +    $configuration += ['file_exists' => FILE_EXISTS_REPLACE];
   

   This is incomplete; we need to process the $configuration['file_exists'] value, which can be one of 'replace', 'reuse', or 'use existing', into a FILE_EXISTS_* constant. The fact that we haven't caught this suggests that our test coverage is incomplete.

3. +++ b/core/modules/migrate/tests/src/Kernel/process/DownloadTest.php
   @@ -51,7 +51,7 @@ public function testNonDestructiveDownload() {
   +    $actual_destination = $this->doTransform($destination_uri, ['file_exists' => FILE_EXISTS_RENAME]);
   

   To illustrate my previous point, this should be 'file_exists' => 'rename'.

Addressed points raised by @phenaproxima in #53.

Test correction (similar to #53.3).
Coding standards corrections.

All feedback addressed. I reviewed the change records as well and made a small tweak. So that looks good. Let's move onward.

Adding issue credit to all participants.

Committed and pushed a6d53088df to 8.7.x and b0b64c6fa4 to 8.6.x. Thanks!

Hooray! Great to see this get over the line. Thanks to everyone who helped with the (many) reviews, especially @phenaproxima and @alexpott.