From a6d53088dfc054a00e217476413d685d5c942fde Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sun, 22 Jul 2018 13:44:57 +0100 Subject: [PATCH] Issue #2877839 by Jo Fitzgerald, edysmp, heddn, Nebel54, phenaproxima, alexpott, quietone: Reuse option in FileCopy migrate process plugin not work with remote files --- .../src/Plugin/migrate/process/Download.php | 24 +-- .../src/Plugin/migrate/process/FileCopy.php | 37 ++--- .../migrate/process/FileProcessBase.php | 59 +++++++ .../tests/src/Kernel/process/DownloadTest.php | 2 +- .../tests/src/Kernel/process/FileCopyTest.php | 42 ++++- .../tests/src/Unit/process/FileCopyTest.php | 149 ++++++++++++++++++ 6 files changed, 266 insertions(+), 47 deletions(-) create mode 100644 core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php create mode 100644 core/modules/migrate/tests/src/Unit/process/FileCopyTest.php diff --git a/core/modules/migrate/src/Plugin/migrate/process/Download.php b/core/modules/migrate/src/Plugin/migrate/process/Download.php index cfcba8115b9..cc5ed859f01 100644 --- a/core/modules/migrate/src/Plugin/migrate/process/Download.php +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php @@ -6,7 +6,6 @@ use Drupal\Core\File\FileSystemInterface; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\migrate\MigrateException; use Drupal\migrate\MigrateExecutableInterface; -use Drupal\migrate\ProcessPluginBase; use Drupal\migrate\Row; use GuzzleHttp\Client; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -19,8 +18,12 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * - destination URI, e.g. 'public://images/foo.img' * * Available configuration keys: - * - rename: (optional) If set, a unique destination URI is generated. If not - * set, the destination URI will be overwritten if it exists. + * - file_exists: (optional) Replace behavior when the destination file already + * exists: + * - 'replace' - (default) Replace the existing file. + * - 'rename' - Append _{incrementing number} until the filename is + * unique. + * - 'use existing' - Do nothing and return FALSE. * - guzzle_options: (optional) * @link http://docs.guzzlephp.org/en/latest/request-options.html Array of request options for Guzzle. @endlink * @@ -42,7 +45,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * source: * - source_url * - destination_uri - * rename: true + * file_exists: rename * @endcode * * This will download source_url to destination_uri and ensure that the @@ -53,7 +56,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * id = "download" * ) */ -class Download extends ProcessPluginBase implements ContainerFactoryPluginInterface { +class Download extends FileProcessBase implements ContainerFactoryPluginInterface { /** * The file system service. @@ -85,7 +88,6 @@ class Download extends ProcessPluginBase implements ContainerFactoryPluginInterf */ public function __construct(array $configuration, $plugin_id, array $plugin_definition, FileSystemInterface $file_system, Client $http_client) { $configuration += [ - 'rename' => FALSE, 'guzzle_options' => [], ]; parent::__construct($configuration, $plugin_id, $plugin_definition); @@ -118,10 +120,12 @@ class Download extends ProcessPluginBase implements ContainerFactoryPluginInterf list($source, $destination) = $value; // Modify the destination filename if necessary. - $replace = !empty($this->configuration['rename']) ? - FILE_EXISTS_RENAME : - FILE_EXISTS_REPLACE; - $final_destination = file_destination($destination, $replace); + $final_destination = file_destination($destination, $this->configuration['file_exists']); + + // Reuse if file exists. + if (!$final_destination) { + return $destination; + } // Try opening the file first, to avoid calling file_prepare_directory() // unnecessarily. We're suppressing fopen() errors because we want to try diff --git a/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php index f598b811984..88f22123224 100644 --- a/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php @@ -9,7 +9,6 @@ use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface; use Drupal\migrate\MigrateException; use Drupal\migrate\MigrateExecutableInterface; use Drupal\migrate\Plugin\MigrateProcessInterface; -use Drupal\migrate\ProcessPluginBase; use Drupal\migrate\Row; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -26,10 +25,12 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * Available configuration keys: * - move: (optional) Boolean, if TRUE, move the file, otherwise copy the file. * Defaults to FALSE. - * - rename: (optional) Boolean, if TRUE, rename the file by appending a number - * until the name is unique. Defaults to FALSE. - * - reuse: (optional) Boolean, if TRUE, reuse the current file in its existing - * location rather than move/copy/rename the file. Defaults to FALSE. + * - file_exists: (optional) Replace behavior when the destination file already + * exists: + * - 'replace' - (default) Replace the existing file. + * - 'rename' - Append _{incrementing number} until the filename is + * unique. + * - 'use existing' - Do nothing and return FALSE. * * Examples: * @@ -48,7 +49,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface; * id = "file_copy" * ) */ -class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterface { +class FileCopy extends FileProcessBase implements ContainerFactoryPluginInterface { /** * The stream wrapper manager service. @@ -90,8 +91,6 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf public function __construct(array $configuration, $plugin_id, array $plugin_definition, StreamWrapperManagerInterface $stream_wrappers, FileSystemInterface $file_system, MigrateProcessInterface $download_plugin) { $configuration += [ 'move' => FALSE, - 'rename' => FALSE, - 'reuse' => FALSE, ]; parent::__construct($configuration, $plugin_id, $plugin_definition); $this->streamWrapperManager = $stream_wrappers; @@ -109,7 +108,7 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf $plugin_definition, $container->get('stream_wrapper_manager'), $container->get('file_system'), - $container->get('plugin.manager.migrate.process')->createInstance('download') + $container->get('plugin.manager.migrate.process')->createInstance('download', $configuration) ); } @@ -150,7 +149,7 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf } } - $final_destination = $this->writeFile($source, $destination, $this->getOverwriteMode()); + $final_destination = $this->writeFile($source, $destination, $this->configuration['file_exists']); if ($final_destination) { return $final_destination; } @@ -181,24 +180,6 @@ class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterf return $function($source, $destination, $replace); } - /** - * 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() { - if (!empty($this->configuration['rename'])) { - return FILE_EXISTS_RENAME; - } - if (!empty($this->configuration['reuse'])) { - return FILE_EXISTS_ERROR; - } - - return FILE_EXISTS_REPLACE; - } - /** * Returns the directory component of a URI or path. * diff --git a/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php new file mode 100644 index 00000000000..8b7d8100000 --- /dev/null +++ b/core/modules/migrate/src/Plugin/migrate/process/FileProcessBase.php @@ -0,0 +1,59 @@ + 'use existing' instead. See https://www.drupal.org/node/2981389.", E_USER_DEPRECATED); + if (!empty($configuration['reuse'])) { + $configuration['file_exists'] = FILE_EXISTS_ERROR; + } + } + 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); + if (!empty($configuration['rename'])) { + $configuration['file_exists'] = FILE_EXISTS_RENAME; + } + } + $configuration += ['file_exists' => FILE_EXISTS_REPLACE]; + parent::__construct($configuration, $plugin_id, $plugin_definition); + } + +} diff --git a/core/modules/migrate/tests/src/Kernel/process/DownloadTest.php b/core/modules/migrate/tests/src/Kernel/process/DownloadTest.php index ee56e32c525..11674c0bc2f 100644 --- a/core/modules/migrate/tests/src/Kernel/process/DownloadTest.php +++ b/core/modules/migrate/tests/src/Kernel/process/DownloadTest.php @@ -51,7 +51,7 @@ class DownloadTest extends FileTestBase { $destination_uri = $this->createUri('another_existing_file.txt'); // Test non-destructive download. - $actual_destination = $this->doTransform($destination_uri, ['rename' => TRUE]); + $actual_destination = $this->doTransform($destination_uri, ['file_exists' => 'rename']); $this->assertSame('public://another_existing_file_0.txt', $actual_destination, 'Import returned a renamed destination'); $this->assertFileExists($actual_destination, 'Downloaded file was created'); } diff --git a/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php index cdc12955359..e107f415740 100644 --- a/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php +++ b/core/modules/migrate/tests/src/Kernel/process/FileCopyTest.php @@ -9,6 +9,7 @@ use Drupal\migrate\Plugin\migrate\process\FileCopy; use Drupal\migrate\MigrateExecutableInterface; use Drupal\migrate\Plugin\MigrateProcessInterface; use Drupal\migrate\Row; +use GuzzleHttp\Client; /** * Tests the file_copy process plugin. @@ -76,30 +77,52 @@ class FileCopyTest extends FileTestBase { /** * Test successful file reuse. + * + * @dataProvider providerSuccessfulReuse + * + * @param string $source_path + * Source path to copy from. + * @param string $destination_path + * The destination path to copy to. */ - public function testSuccessfulReuse() { - $source_path = $this->root . '/core/modules/simpletest/files/image-test.jpg'; - $destination_path = 'public://file1.jpg'; - $file_reuse = file_unmanaged_copy($source_path, $destination_path); + public function testSuccessfulReuse($source_path, $destination_path) { + $file_reuse = $this->doTransform($source_path, $destination_path); + clearstatcache(TRUE, $destination_path); + $timestamp = (new \SplFileInfo($file_reuse))->getMTime(); $this->assertInternalType('int', $timestamp); // We need to make sure the modified timestamp on the file is sooner than // the attempted migration. sleep(1); - $configuration = ['reuse' => TRUE]; + $configuration = ['file_exists' => 'use existing']; $this->doTransform($source_path, $destination_path, $configuration); clearstatcache(TRUE, $destination_path); $modified_timestamp = (new \SplFileInfo($destination_path))->getMTime(); $this->assertEquals($timestamp, $modified_timestamp); - $configuration = ['reuse' => FALSE]; - $this->doTransform($source_path, $destination_path, $configuration); + $this->doTransform($source_path, $destination_path); clearstatcache(TRUE, $destination_path); $modified_timestamp = (new \SplFileInfo($destination_path))->getMTime(); $this->assertGreaterThan($timestamp, $modified_timestamp); } + /** + * Provides the source and destination path files. + */ + public function providerSuccessfulReuse() { + return [ + [ + 'local_source_path' => static::getDrupalRoot() . '/core/modules/simpletest/files/image-test.jpg', + 'local_destination_path' => 'public://file1.jpg', + ], + [ + 'remote_source_path' => 'https://www.drupal.org/favicon.ico', + 'remote_destination_path' => 'public://file2.jpg', + ], + ]; + } + /** * Test successful moves. */ @@ -179,7 +202,7 @@ class FileCopyTest extends FileTestBase { $source = $this->createUri(NULL, NULL, 'temporary'); $destination = $this->createUri('foo.txt', NULL, 'public'); $expected_destination = 'public://foo_0.txt'; - $actual_destination = $this->doTransform($source, $destination, ['rename' => TRUE]); + $actual_destination = $this->doTransform($source, $destination, ['file_exists' => 'rename']); $this->assertFileExists($expected_destination, 'File was renamed on import'); $this->assertSame($actual_destination, $expected_destination, 'The importer returned the renamed filename.'); } @@ -222,6 +245,9 @@ class FileCopyTest extends FileTestBase { * The URI of the copied file. */ protected function doTransform($source_path, $destination_path, $configuration = []) { + // Prepare a mock HTTP client. + $this->container->set('http_client', $this->createMock(Client::class)); + $plugin = FileCopy::create($this->container, $configuration, 'file_copy', []); $executable = $this->prophesize(MigrateExecutableInterface::class)->reveal(); $row = new Row([], []); diff --git a/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php new file mode 100644 index 00000000000..07a953b53c4 --- /dev/null +++ b/core/modules/migrate/tests/src/Unit/process/FileCopyTest.php @@ -0,0 +1,149 @@ + 'rename' instead. See https://www.drupal.org/node/2981389. + */ + public function testDeprecationNoticeRename($configuration, $expected) { + $this->assertPlugin($configuration, $expected); + } + + /** + * Data provider for testDeprecationNoticeRename. + */ + public function providerDeprecationNoticeRename() { + return [ + [['rename' => TRUE], FILE_EXISTS_RENAME], + [['rename' => FALSE], FILE_EXISTS_REPLACE], + ]; + } + + /** + * Tests that the reuse configuration key will trigger a deprecation notice. + * + * @dataProvider providerDeprecationNoticeReuse + * + * @param array $configuration + * The plugin configuration. + * @param $expected + * The expected value of the plugin configuration. + * + * @expectedDeprecation Using the key 'reuse' is deprecated, use 'file_exists' => 'use existing' instead. See https://www.drupal.org/node/2981389. + */ + public function testDeprecationNoticeReuse($configuration, $expected) { + $this->assertPlugin($configuration, $expected); + } + + /** + * Data provider for testDeprecationNoticeReuse. + */ + public function providerDeprecationNoticeReuse() { + return [ + [['reuse' => TRUE], FILE_EXISTS_ERROR], + [['reuse' => FALSE], FILE_EXISTS_REPLACE], + ]; + } + + /** + * Tests that the plugin constructor correctly sets the configuration. + * + * @dataProvider providerFileProcessBaseConstructor + * + * @param array $configuration + * The plugin configuration. + * @param $expected + * The expected value of the plugin configuration. + */ + public function testFileProcessBaseConstructor($configuration, $expected) { + $this->assertPlugin($configuration, $expected); + } + + /** + * Data provider for testFileProcessBaseConstructor. + */ + public function providerFileProcessBaseConstructor() { + return [ + [['file_exists' => 'replace'], FILE_EXISTS_REPLACE], + [['file_exists' => 'rename'], FILE_EXISTS_RENAME], + [['file_exists' => 'use existing'], FILE_EXISTS_ERROR], + [['file_exists' => 'foobar'], FILE_EXISTS_REPLACE], + [[], FILE_EXISTS_REPLACE], + ]; + } + + /** + * Creates a TestFileCopy process plugin. + * + * @param array $configuration + * The plugin configuration. + * @param $expected + * The expected value of the plugin configuration. + */ + protected function assertPlugin($configuration, $expected) { + $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 TestFileCopy($configuration, 'test', [], $stream_wrapper_manager, $file_system, $download_plugin); + $plugin_config = $this->plugin->getConfiguration(); + $this->assertArrayHasKey('file_exists', $plugin_config); + $this->assertSame($expected, $plugin_config['file_exists']); + } + +} + +/** + * Class for testing FileCopy. + */ +class TestFileCopy extends FileCopy { + + /** + * Gets this plugin's configuration. + * + * @return array + * An array of this plugin's configuration. + */ + public function getConfiguration() { + return $this->configuration; + } + +}