Refactor EntityFile and use process plugins instead

From the proposed solution, #6 should be punted. File streams in PHP are notoriously hard and if someone needs to do that to s3 or some other remote file system, they are probably going to need to write something custom. So give them the basic building blocks and let them build it themselves.

I'm not sure I like (all of) this idea.

I agree that the destination shouldn't use anything from the source. If the destination needs info, it should be mapped.

I don't really want to lose the destination_path_property, I think that's useful.

More importantly, I think anything that we might want to rollback should be done by the destination, not a process. I don't like the idea that the process could download the file, and then if the destination failed for some reason, we'd have a stray file sitting around, with no way to roll it back.

Yes, I like the idea of making destination_path_property a property that can be mapped to. That's fine.

The destination would delete the file entity which in turn should clean-up the file.

That's the "if everything is as expected" situation, so of course it works. But migrate should be robust. For example, a subsequent process could throw a skip row exception—now we just have a junk file sitting around, from a row we thought we skipped.

Working on this at the NOLA sprint, and I have a file_copy process plugin working with d6_file thusly:

  source_full_path:
    plugin: concat
    delimiter: /
    source:
      - constants/source_base_path
      - filepath
  destination_full_path:
    plugin: file_uri
    source:
      - filepath
      - file_directory_path
      - temp_directory_path
      - is_public
  uri:
    plugin: file_copy
    source:
      - '@source_full_path'
      - '@destination_full_path'
    urlencode: true

The EntityFile destination plugin is reduced to getEntity() and processStubRow() implementations. Feeling good about this... Remaining:

1. Move urlencode to its own process plugin.
2. Apply to D7
3. Update the tests to reflect the changes (may leave this as an exercise for the Friday sprinters).
4. Write a change notice.

I'll post a patch once I've got #1 & 2.

OK, here it is, we'll let the testbot show us where tests need to be updated.

Next steps:

1. Feedback - does this at least conceptually look good to the interested parties?
2. Manually test with D7 (I've only manually tested with a D6 site).
3. Fix the tests.
4. Write a change notice.

All good stuff for DrupalCon sprinters to tackle...

Meant to preview, but found save instead.

Here's the interdiff for the above patch. The D7 template wasn't using the source_base_path, so I copied it from the D6 version.

Didn't know the test would not start on #14. So, uploading again for testing.

Working on fixing the tests now - picked up a couple of issues I'm also dealing with here.

1. +++ b/core/modules/file/migration_templates/d7_file.yml
   @@ -9,7 +9,21 @@ source:
   +    urlencode: true
   

   Remove, urlencode is handled by its own plugin.

2. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,225 @@
   +  protected function urlencode($filename) {
   

   Remove from file_copy plugin, is now its own plugin.

Progress on the tests - added a unit test for the urlencode plugin, and transmogrified the EntityFile destination plugin test (which was only testing file-copying functionality) into a CopyFile process plugin test. I think there's more downstream to fix, but let's let the testbot tell us where...

This fixes some of the test failures - I'm not currently seeing what's triggering the others.

The d7\MigrateFileTest failure is because we no longer need to append a slash to source_base_path. Now the concat has a delimiter, so that adds a slash. We potentially end up with THREE slashes, which vfsRoot can't handle.

This shouldn't affect the behavior of what the user types into MigrateUpgradeForm, because that migration won't be using vfsRoot.

I made some progress on the remaining failures:

* The file migrations require that source_base_path is set. But d6/MigrateFileTest does two migrations, only the first one sets that. Should be fixed by using a method to setup the migration, and calling it each time it's needed.
* Setting an empty source_base_path doesn't work anymore, because we no longer just append filename, but concat with a slash delimiter. So we get implode('/', ['', 'foo.jpg']) => '/foo.jpg', when we really wanted ./foo.jpg . Fixed by using $this->root as the source_base_path.

But the above fix breaks tmp migrations. Because now we're doing implode('/', ['/path/to/drupal/root', '/tmp/foo'']) => '/path/to/drupal/root/tmp/foo'. We really want that to stay just '/tmp/foo'.

I'm thinking that we can't rely on concat to build paths. Either we need a custom plugin to handle path concatenation (with correct behavior for absolute/relative paths). Or we need some sort of interesting process steps to get this right.

This has a new process plugin for the d6 file source path. This fixed /d6/MigrateFileTest.php locally. That just leaves the /d6/MigrateUserPictureFileTest.php to fix.

OK, I see we need to update d6_user_picture_file.yml for the new regime, looking at that...

OK, this fixes the user picture file error. One error remaining, around fid 6 in the database fixture.

  'fid' => '6',
  'uid' => '1',
  'filename' => 'some-temp-file.jpg',
  'filepath' => '/tmp/some-temp-file.jpg',
  'filemime' => 'image/jpeg',
  'filesize' => '24',
  'status' => '0',
  'timestamp' => '1420858106',

And the filename and filepath are modified in migrateDumpAlter() to a randomly-generated name. Need a little debugging to see why that temporary file is no longer migrated...

I'm not sure the logic in d6_file_source is correct. Tried to fix it up, and explained what it's doing.

Forgot to make sure source_base_path was set on all test migrations that need it.

It looks like the failing file in the migrate_drupal_ui test is the temp file. I don't really know how that file is supposed to have gotten in /tmp in the first place, how did it ever work before?

yea, I had an error in the process plugin. But don't we still need to check for an empty source_base_path? Otherwise we return "/$filepath".

Ooh, that's a good point. Ok, I'll update it.

Ok, I think this one should work.

Yay!

how that file is supposed to have gotten in /tmp in the first place, how did it ever work before?

I could be wrong, but perhaps because the destination plugin was created with this configuration

$configuration += array(
      'source_base_path' => '',
      'source_path_property' => 'filepath',
      'destination_path_property' => 'uri',
      'move' => FALSE,
      'urlencode' => FALSE,
    );

And then it set the source as
$source = $this->configuration['source_base_path'] . $file;

So /tmp would not be have anything prepended.

migrate_dump_alter() creates the temp file.

So, this is great - now we need a proper review by someone who didn't contribute to the implementation.

1. The first thing I notice is that most of the methods were removed from EntityFile.php, but all of the use statements are still there. It looks to me as though the only ones you need are these:
   

   use Drupal\Component\Utility\Unicode;
   use Drupal\Core\Field\Plugin\Field\FieldType\UriItem;
   use Drupal\migrate\Row;
   use Drupal\migrate\MigrateException;
   use Drupal\migrate\Plugin\migrate\destination\EntityContentBase;
   

2. /**
    * Every migration that uses this destination must have an optional
    * dependency on the d6_file migration to ensure it runs first.
    */
   

   Is the comment on the EntityFile class still needed?

3. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   +   * @return bool
   +   *  TRUE on success, FALSE on failure.
   +   */
   +  protected function writeFile($source, $destination, $replace = FILE_EXISTS_REPLACE) {
   

   Indentation error

4. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   + * Process the file url into a D8 compatible URL.
   

   This description seems inadequate. This plugin handles copying the file.

5. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   +  protected function getOverwriteMode(Row $row) {
   +    if (!empty($this->configuration['rename'])) {
   +      return FILE_EXISTS_RENAME;
   +    }
   +    return FILE_EXISTS_REPLACE;
   +  }
   

   Can we remove the parameter (and its code comment)?

6. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   +   * @return string|false
   +   *  The directory component of the path or URI, or FALSE if it could not
   +   *  be determined.
   +   */
   +  protected function getDirectory($uri) {
   

   indentation error

7. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   +  /**
   +   * Returns if the source and destination URIs represent identical paths.
   +   * If either URI is a remote stream, will return FALSE.
   +   *
   +   * @param string $source
   +   *   The source URI.
   +   * @param string $destination
   +   *   The destination URI.
   +   *
   +   * @return bool
   +   *  TRUE if the source and destination URIs refer to the same physical path,
   +   *  otherwise FALSE.
   +   */
   +  protected function isLocationUnchanged($source, $destination) {
   

   I think that "Determines" would be clearer than "Returns".
   Insert a blank line after the short description. Fix indentation of the @return comment.

8. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,203 @@
   +   * Returns if the given URI or path is considered local.
   

   I think that "Determines" would be clearer than "Returns".

That is all for more, but I am still reviewing.

Thanks #benjifisher, I'll update the patch and respond.

1. The first thing I notice is that most of the methods were removed from EntityFile.php, but all of the use statements are still there.

Removed obsolete uses.

2. Is the comment on the EntityFile class still needed?

The comment really isn't right to begin with, and generally we don't have comments on the destination entity extensions, so removing.

3. Indentation error

Fixed.

4. This description seems inadequate. This plugin handles copying the file.

Copypasta, fixed.

5. Can we remove the parameter (and its code comment)?

Yes we can!

6. indentation error

Fixed.

7. I think that "Determines" would be clearer than "Returns".
Insert a blank line after the short description. Fix indentation of the @return comment.

Done.

8. I think that "Determines" would be clearer than "Returns".

Done.

Awaiting further feedback before rolling a new patch.

Just a few more formatting complaints from CodeSniffer:

1. +++ b/core/modules/file/src/Plugin/migrate/process/Urlencode.php
   @@ -0,0 +1,37 @@
   +class Urlencode extends ProcessPluginBase{
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   +    // Only apply to a full URL
   

   Add a space before the opening brace on the class line.
   Add ending punctuation to the code comments. Ditto for the comment on Line 30.

2. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlencodeTest.php
   @@ -0,0 +1,59 @@
   +   * Perform the urlencode process plugin over the given value.
   +   *
   +   * @param $value
   +   *   Url to be encoded.
   

   missing parameter type

Last couple of comments addressed, here we go.

I checked the interdiff in #49, and it resolves all the issues I pointed out. According to the testbot, we did not break anything.

I did not test the functionality, so I can only claim the "R" part of RTBC.

Just ran a D6->D8 upgrade via the UI. The D6 site was devel generated with about 210 files (file fields, uploads and images) and the migration was successful.

I did notice that the D8 database is cluttered with 765 migrate_map_'hash' and migrate_message_'hash' tables. I checked a few tables and found that they are empty and all the map tables do not have any destination ids. Repeating the migration without the patch results in the same 'extra' map and message tables being generated. So, it is not related to this patch, but why some many?

And same results as above for D7->D8.

@quietone - map/message tables with the generated hash names are due to #2575135: Dummy map/message tables being created. Although, my D6 site generates 9 of each table, 765 seems a bit... much.

Looking at the logic in the d6_file_source plugin, it is not clear to me why that logic would be D6 specific and for D7 we can just use concat?

For D6, there are some tests that pass an empty source_base_path and a relative filepath. I don't know whether this is just a testing artifact, or something that's likely to actually happen.

I'd like to do a quick review sometime in the next day or two to see if this supports what is outlined in #2706405: File migrations *always* copy in files, even if the files were off-line copied to destination, or if that should come in as a follow-up issue.

Although an important thing, I won't mark this as needs work. We can re-add 'preserve_files' back in under #2706405: File migrations *always* copy in files, even if the files were off-line copied to destination after this gets committed. Other feedback below. Marking needs work so we can respond to some of this feedback.

1. +++ b/core/modules/file/src/Plugin/migrate/process/Urlencode.php
   @@ -0,0 +1,37 @@
   +class Urlencode extends ProcessPluginBase {
   

   Nit: Proper camel case is UrlEncode

2. +++ b/core/modules/file/src/Plugin/migrate/process/Urlencode.php
   @@ -0,0 +1,37 @@
   +      $value = str_replace('%3A', ':', $value);
   +      $value = str_replace('%3F', '?', $value);
   +      $value = str_replace('%26', '&', $value);
   

   This seems fragile. Is there really no other way around this?

3. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlencodeTest.php
   @@ -0,0 +1,59 @@
   +class UrlencodeTest extends MigrateTestCase {
   

   And if we camel case the class under test, then this should too.

Looking at the logic in the d6_file_source plugin, it is not clear to me why that logic would be D6 specific and for D7 we can just use concat?

Damn, you sent me down a rathole here... :P. So, this was introduced based on #25 above, and one aspect mentioned there was handling temp files (which might be something like /tmp/... - i.e., absolute paths which shouldn't have the source_base_path prepended). Looking at how D6 temp files are handled:

1. d6_file.yml references temp_directory_path - oops, the D6 variable name is actually file_directory_temp, so this is broken.
2. temp_directory_path - I mean, file_directory_temp - is an absolute path on the D6 site's server - unless the D8 site is on the same server, you can't get to that even if it were properly named.
3. Why would we want to migrate temporary files anyway?

Well, that's out of scope and should be a follow-up issue... In the context of the present issue, though, I would say that since migration of temporary files is totally broken now and probably should be removed anyway, this patch should not worry about them.

So, the main thing the d6_file_source plugin is doing is accommodating an empty source_base_path, which really doesn't make sense if you actually want to copy the files - it would only work if the filepath coming in is an absolute local path (as it would be for the temp files) and the source and destination sites are on the same server, which certainly should not be the default assumption. So, I think we can leave off this custom plugin and just concat? Not changing yet in this patch - @vasi, @quietone, am I missing something?

I don't think 'uri' should be hardcoded in the destination, this is the only thing stopping someone using this destination with a custom entity that doesn't use 'uri' ?

This is the file entity destination, and the file entity's URI value is always in the 'uri' column, right? Are you thinking of some custom file/media entity which extends this entity class, in which case it could override this... Maybe to accommodate that 'uri' would be a class constant the deriving entity could override?

We return early if it's unchanged, that's before we check if the user wanted to overwrite the file? Looks like it could be pre-existing though.

Yes, that's the existing logic. Hmm, right, that would just render the overwriteMode() totally irrelevant - this code would appear to never do anything with the file if it already exists (although heddn suggests in practice it doesn't work that way). Let's call fixing this here out of scope, and when we followup on #2706405: File migrations *always* copy in files, even if the files were off-line copied to destination we can make sure all three responses to an existing destination file (replace, rename, preserve) work as they should.

I know this is pre-existing but the logic is pretty hard to follow, maybe we could simplify it, something like this (not tested):

Done.

Nit: Proper camel case is UrlEncode

Done.

This seems fragile. Is there really no other way around this?

We need to urlencode the path components, yet preserve the path syntax. Seems like a common-enough thing to want that there should be a standard mechanism, but I don't know what that is. Anyway, changing this logic is not in the scope of this issue.

And if we camel case the class under test, then this should too.

Done.

I concur that the d6_file_source is extremely weird. I can't think of any normal situation where one would want an empty source_base_path—the tests that do this should be changed. I do think we will run into lots of annoying-to-fix tests, though. Eg: The VFS mock filesystem deals fine with paths containing two consecutive slashes, but NOT three slashes. Ugh.

We should probably document what makes a valid source_base_path and/or filepath. Eg: Can they be empty? Start with slash? End with slash? etc.

#2729369: Remove support for migrating temporary files added.

This version of the patch:

1. Uses a const in the destination for 'uri'.
2. Returns to a simple concat for D6 source paths.
3. Documents source_base_path in the .yml files using it.
4. Makes sure source_base_path is actually set in the D6 tests.
5. A bit of a cheat, perhaps - removes the D6 temporary file test that fails when source_base_path is set. The test itself was a cheat to begin with - it relied on migrateDumpAlter() creating the temporary file hypothetically being migrated from D6 in the local /tmp, which didn't really prove anything.

If that cheat isn't acceptable, maybe we should do #2729369: Remove support for migrating temporary files first to get rid of the temporary file junk, then this patch will be clean of it.

Missed one d6_file_source...

@mikeryan, thanks for delving in that rabbit hole. I don't think you missed anything. And if temp files don't need to be migrated, then the d6 and d7 migrations are similar and both are easier to understand. Thats a positive for me.

Found one small copy/paste error.

+++ b/core/modules/user/migration_templates/d6_user_picture_file.yml
@@ -6,21 +6,40 @@ source:
+    source_base_path: ''
+    source_base_path: ''

Duplicated line.

Nit fixed, thanks!

Tested this yesterday and today by migrating a D6 and a D7 site, both devel generated. It worked as expected. And the D6 site has a temp file and that did not migrate.

Assigning to myself for review.

Overall, I love this approach. It makes so much more sense for plugins to do one thing, and do it well. And this embraces that philosophy beautifully. Bravo!

The patch is pretty good so far, but I did find some things...

1. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -285,10 +60,10 @@ protected function processStubRow(Row $row) {
   +      $value = Unicode::substr($value, 0, $field_definitions[static::URI_COLUMN]->getSetting('max_length'));
   

   IMHO, there should be a sanity check to ensure that $field_definitions[static::URI_COLUMN] actually exists.

2. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface
   +   */
   +  protected $streamWrapperManager;
   +
   +  /**
   +   * @var \Drupal\Core\File\FileSystemInterface
   +   */
   +  protected $fileSystem;
   

   Both of these properties need a description in the doc comment.

3. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Can't inherit constructor docs :)

4. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,196 @@
   +    if ($this->writeFile($source, $destination, $replace)) {
   +      return $destination;
   +    }
   

   Why are we doing this twice? Shouldn't we try to prepare the directory first, then call $this->writeFile() once?

5. +++ b/core/modules/file/src/Plugin/migrate/process/UrlEncode.php
   @@ -0,0 +1,37 @@
   +    if (strpos($value, '://')) {
   

   Can this be made more explicit, just for clarity's sake? (if strpos() > 0)

6. +++ b/core/modules/file/src/Plugin/migrate/process/UrlEncode.php
   @@ -0,0 +1,37 @@
   +        $components[$key] = rawurlencode($component);
   

   What's the difference between rawurlencode() and urlencode()? A comment here would be helpful.

7. +++ b/core/modules/file/src/Plugin/migrate/process/UrlEncode.php
   @@ -0,0 +1,37 @@
   +      // Actually, we don't want certain characters encoded.
   +      $value = str_replace('%3A', ':', $value);
   +      $value = str_replace('%3F', '?', $value);
   +      $value = str_replace('%26', '&', $value);
   

   str_replace() can take arrays as arguments, so we don't have to call it three times here. Yay, micro-optimization!

8. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
   @@ -0,0 +1,165 @@
   +  /**
   +   * Modules to install.
   +   *
   +   * @var array
   +   */
   

   Should be @inheritdoc.

9. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
   @@ -0,0 +1,165 @@
   +    \Drupal::getContainer()->get('stream_wrapper_manager')->registerWrapper('public', 'Drupal\Core\StreamWrapper\PublicStream', StreamWrapperInterface::NORMAL);
   

   Nit: \Drupal::getContainer() can be $this->container. Also, won't the public stream wrapper be automatically registered in a kernel test?

10. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    foreach ($this->localFileDataCopyProvider() as $data) {
    

    Because this is a PHPUnit-based kernel test, there's no need to loop over the result of $this->localFileDataCopyProvider(). Better to use the data provider functionality provided by PHPUnit -- unless it's hugely slowing the test down, since that *would* reinitialize the test before every set of data.

11. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    touch('/tmp/test-file.jpg');
    

    Ideally, it'd be preferable to use vfsStream for this, since file system mocking is exactly what it's for. But, no urgent need for that if it's a huge pain in the ass.

12. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    foreach ($this->localFileDataMoveProvider() as $data) {
    

    Same thing here about using the data provider pattern the way it's supposed to be used :)

13. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +  public function testNonExistentSourceFile() {
    

    Let's use the @expectedException annotation for this test, rather than catching and asserting the exception manually.

14. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    $this->assertTrue(is_file($expected_destination));
    

    I think there is an assertFileExists() or similar that we can use here, rather than assertTrue().

15. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    $this->plugin = FileCopy::create(\Drupal::getContainer(), $configuration, 'file_copy', []);
    

    Let's use $this->container instead of \Drupal::getContainer().

16. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
    @@ -0,0 +1,165 @@
    +    // The plugin should either throw an exception or return the destination
    +    // path.
    +    $this->assertSame($result, $destination_path);
    

    This looks like a simple utility method. Why is it making an assertion?

17. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
    @@ -0,0 +1,59 @@
    +  /**
    +   * Cover various encoding scenarios.
    +   */
    +  public function testUrls() {
    

    This test might benefit from the data provider pattern that PHPUnit implements. Not urgent, though.

18. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
    @@ -137,6 +137,15 @@ protected function prepareMigrations(array $id_mappings) {
    +  protected function processMigration($migration) {
    +  }
    

    What's up with this? It at least needs a comment explaining why it's empty.

+ $value = Unicode::substr($value, 0, $field_definitions[static::URI_COLUMN]->getSetting('max_length'));
IMHO, there should be a sanity check to ensure that $field_definitions[static::URI_COLUMN] actually exists.

I don't think there's a need for runtime sanity-checking here - the only reason the field definition would not exist is if someone extended the destination plugin for their own file entity variant and mucked up the URI_COLUMN, which the ensuing errors would quickly inform them of. I don't see anything in core doing this sanity-check (parallel example in EntityUser).

+ /**
+ * @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface
+ */
+ protected $streamWrapperManager;
+
+ /**
+ * @var \Drupal\Core\File\FileSystemInterface
+ */
+ protected $fileSystem;

Both of these properties need a description in the doc comment.

Done.

+ /**
+ * {@inheritdoc}
+ */

Can't inherit constructor docs :)

Done (although you might want to take a look at most of our other process plugins...

+ if ($this->writeFile($source, $destination, $replace)) {

Why are we doing this twice? Shouldn't we try to prepare the directory first, then call $this->writeFile() once?

You might want to check with the dev that added this:P. I presume that the reason is performance - not wanting to call file_prepare_directory() on every single file being migrated. This way, file_prepare_directory() is only called at most once per distinct directory on the destination side (and the second writeFile() is only called at those times). Added a comment to that effect.

+ if (strpos($value, '://')) {

Can this be made more explicit, just for clarity's sake? (if strpos() > 0)

Done.

+ $components[$key] = rawurlencode($component);

What's the difference between rawurlencode() and urlencode()? A comment here would be helpful.

rawurlencode() follows RFC 3986, unlike urlencode which converts spaces to plus-signs. Added comment.

+ // Actually, we don't want certain characters encoded.
+ $value = str_replace('%3A', ':', $value);
+ $value = str_replace('%3F', '?', $value);
+ $value = str_replace('%26', '&', $value);

str_replace() can take arrays as arguments, so we don't have to call it three times here. Yay, micro-optimization!

Done.

+ /**
+ * Modules to install.
+ *
+ * @var array
+ */

Should be @inheritdoc.

Done.

+ \Drupal::getContainer()->get('stream_wrapper_manager')->registerWrapper('public', 'Drupal\Core\StreamWrapper\PublicStream', StreamWrapperInterface::NORMAL);

Nit: \Drupal::getContainer() can be $this->container.

Done.

Also, won't the public stream wrapper be automatically registered in a kernel test?

Haven't tried without it, but I'm following the example of MigrateFileTest here.

+ foreach ($this->localFileDataCopyProvider() as $data) {

Because this is a PHPUnit-based kernel test, there's no need to loop over the result of $this->localFileDataCopyProvider(). Better to use the data provider functionality provided by PHPUnit -- unless it's hugely slowing the test down, since that *would* reinitialize the test before every set of data.

Will look at this.

+ touch('/tmp/test-file.jpg');

Ideally, it'd be preferable to use vfsStream for this, since file system mocking is exactly what it's for. But, no urgent need for that if it's a huge pain in the ass.

Will look at this.

+ foreach ($this->localFileDataMoveProvider() as $data) {

Same thing here about using the data provider pattern the way it's supposed to be used :)

Will look at this.

+ public function testNonExistentSourceFile() {

Let's use the @expectedException annotation for this test, rather than catching and asserting the exception manually.

Done.

+ $this->assertTrue(is_file($expected_destination));

I think there is an assertFileExists() or similar that we can use here, rather than assertTrue().

Done.

+ $this->plugin = FileCopy::create(\Drupal::getContainer(), $configuration, 'file_copy', []);

Let's use $this->container instead of \Drupal::getContainer().

Done.

+ // The plugin should either throw an exception or return the destination
+ // path.
+ $this->assertSame($result, $destination_path);

This looks like a simple utility method. Why is it making an assertion?

The assertion is performed for each test that calls this method.

+ /**
+ * Cover various encoding scenarios.
+ */
+ public function testUrls() {

This test might benefit from the data provider pattern that PHPUnit implements. Not urgent, though.

Enough else to do, skipping.

+ protected function processMigration($migration) {
+ }

What's up with this? It at least needs a comment explaining why it's empty.

Comment added.

Didn't have much luck with vfsStream, let's at least see how the rest of the changes fare.

Unforunately, the previous patch doesn't apply any more, which makes interdiff unhappy.

Managed to get an interdiff, now let's see what broke the tests...

So, the problem there was that the data providers are referencing $this->root, but it appears that data providers are not executed in the context of the test class itself - $this->root is empty, screwing everything up. Presumably this is why the original test was iterating the provider explicitly, so restoring to that.

Interdiff is against the reviewed patch.

Note that I've added a change record.

Updated patch, addressed 1,2 and 4.

Removed that also.

+ // Actually, we don't want certain characters encoded.

Do we know why this is the case?

The "certain characters" are part of the URL syntax - ':', '?', '&' - which must remain for the URL to actually work.

Clarified the comment.

Oops, ignore the interdiff in the last comment.

I would like to test this, is there an example somewhere? I found this one, but it doesn't seem to work with 8.1...

• Article: https://evolvingweb.ca/blog/bringing-files-along-for-ride-to-d8
• Example: https://github.com/vasi/migrate_process_file

Hi @ressa, that blog post was for 8.0, and wasn't quite related to this. You can look at the new d6_file and d7_file migrations in the patch for an example.

@ressa - Also see the related change record.

Blocker for #2560795: Source plugins have a hidden dependency on migrate_drupal.

A note - this has no BC layer, so should only be committed to 8.2.x.

Um...no it didn't.

1. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -95,188 +45,13 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   -  protected function writeFile($source, $destination, $replace = FILE_EXISTS_REPLACE) {
   -    if ($this->configuration['move']) {
   -      return (boolean) file_unmanaged_move($source, $destination, $replace);
   -    }
   -    else {
   -      $destination = file_destination($destination, $replace);
   -      $source = $this->urlencode($source);
   -      return @copy($source, $destination);
   -    }
   -  }
   ...
   -  protected function urlencode($filename) {
   -    // Only apply to a full URL
   -    if ($this->configuration['urlencode'] && strpos($filename, '://')) {
   -      $components = explode('/', $filename);
   -      foreach ($components as $key => $component) {
   -        $components[$key] = rawurlencode($component);
   -      }
   -      $filename = implode('/', $components);
   -      // Actually, we don't want certain characters encoded
   -      $filename = str_replace('%3A', ':', $filename);
   -      $filename = str_replace('%3F', '?', $filename);
   -      $filename = str_replace('%26', '&', $filename);
   -    }
   -    return $filename;
   -  }
   
   +++ b/core/modules/file/src/Plugin/migrate/process/UrlEncode.php
   @@ -0,0 +1,36 @@
   +<?php
   +
   +namespace Drupal\file\Plugin\migrate\process;
   +
   +use Drupal\migrate\MigrateExecutableInterface;
   +use Drupal\migrate\ProcessPluginBase;
   +use Drupal\migrate\Row;
   +
   +/**
   + * Apply urlencoding to a URI.
   + *
   + * @MigrateProcessPlugin(
   + *   id = "urlencode"
   + * )
   + */
   +class UrlEncode extends ProcessPluginBase {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   +    // Only apply to a full URL.
   +    if (strpos($value, '://') > 0) {
   +      $components = explode('/', $value);
   +      foreach ($components as $key => $component) {
   +        // urlencode() would convert spaces to + signs.
   +        $components[$key] = rawurlencode($component);
   +      }
   +      $value = implode('/', $components);
   +      // Do not encode URL syntax characters.
   +      $value = str_replace(['%3A', '%3F', '%26'], [':', '?', '&'], $value);
   +    }
   +    return $value;
   +  }
   +
   +}
   

   A few things:

   • Prior to this patch the url encoding only occurred on write and didn't affect any value outside of that method.
   • It'd be great if the UrlEncode plugin could explain why it is necessary
   • The name is very generic but we're only using this for files - yes it is in the File namespace but...

2. +++ /dev/null
   @@ -1,278 +0,0 @@
   -  /**
   -   * The data provider for testing the file destination.
   -   *
   -   * @return array
   -   *   An array of file permutations to test.
   -   */
   -  protected function localFileDataProvider() {
   -    return [
   -      // Test a local to local copy.
   -      [['filepath' => 'core/modules/simpletest/files/image-test.jpg'], 'public://file1.jpg', TRUE, $this->root . '/'],
   -      // Test a temporary file using an absolute path.
   -      [['filepath' => '/tmp/test-file.jpg'], 'temporary://test.jpg', TRUE, ''],
   -      // Test a temporary file using a relative path.
   -      [['filepath' => 'test-file.jpg'], 'temporary://core/modules/simpletest/files/test.jpg', TRUE, '/tmp/'],
   -      // Test a remote path to local.
   -      [['filepath' => 'core/modules/simpletest/files/image-test.jpg'], 'public://remote-file.jpg', TRUE, $this->root . '/'],
   -      // Test a remote path to local inside a folder that doesn't exist.
   -      [['filepath' => 'core/modules/simpletest/files/image-test.jpg'], 'public://folder/remote-file.jpg', TRUE, $this->root . '/'],
   -    ];
   -  }
   

   Is there any equivalent test - this says it is testing a remote path to local - are we doing that now?

Prior to this patch the url encoding only occurred on write and didn't affect any value outside of that method.

Logically, transformations of source data into the proper form for saving should be done in the process pipeline, while the destination plugin should focus on creating the resulting entity (or whatever) in Drupal. It's precisely the point of this patch to render unto the destination what is the destination's.

In IRC you said "we only used to do this change to the source url when writing - now we change the source for everything" - the source itself is not changed, only the value that gets assigned to the destination is affected by applying the urlencode plugin, and no other uses of the source value are affected (getSourceProperty('uri') will not get the urlencoded version).

It'd be great if the UrlEncode plugin could explain why it is necessary

I'll add a comment describing the use case (it's needed if the URL will be invoked by the migration process, if it's not already encoded).

The name is very generic but we're only using this for files - yes it is in the File namespace but..,

True - while the only migrations in core that are using it are for file entities, there could be custom file/media-handling use cases that might make use of this, as well as the file_copy plugin. Perhaps they should move to migrate?

Is there any equivalent test - this says it is testing a remote path to local - are we doing that now?

It said it was testing a remote path, but it wasn't and the test was redundant so I took it out. Is there a good way to test accessing remote (http://) files? I recall in the context of another issue attempting to make such a test and giving up in frustration.

Now, my own comment... On my current project I had need of that file_copy plugin (see #2635622: Process plugin for importing/creating files by URL) to create and map file entities within the context of an image field mapping in a node migration. When the source file was missing, the MigrateException thrown by the file_copy plugin threw out the entire node, which was not desirable. Indeed, my general feeling is that process plugins should at worst throw MigrateSkipProcessException (throwing out only the field they are populating, not the entire entity they're migrating), and it was an oversight on my part to blindly copy the MigrateException throws (which made sense at the entity destination level). I will replace them with MigrateSkipProcessException.

This should be split and remote file access separately supported via guzzle. All this is just fumbling around a known and solved problem. Bonus: we can add proxy support, authentication and everything else needed.

New patch reflecting #92.

@chx: Can you describe a bit more how you would visualize that?

I have to say, trying hard to stabilize the Migrate API for 8.2.x, I'm reluctant to take on any more ambitious refactoring than what we already have in progress...

@mikeryan

only the value that gets assigned to the destination is affected by applying the urlencode plugin,

confuses me because the in HEAD it is happening to the source value...

-    else {
-      $destination = file_destination($destination, $replace);
-      $source = $this->urlencode($source);
-      return @copy($source, $destination);
-    }

Well, let's consider we are migrating core here and core does not even support remote storage in the very first place. It supports public:// and private:// and that's what we need to. Indeed the right course would've been to won't fix #2244555: Use copy() directly instead of file_unmanaged_copy() because the very premise of that issue was wrong:

File migration needs to be able to fetch remote files

Nope, it doesn't. The migrate module needs to have support for remote but migrate_drupal doesn't. So we should remove urlencode entirely from this patch.

Then in a followup we should introduce the file download plugin wrapping a Guzzle HTTP request.

I've fixed a couple of test problems but it looks like my change from MigrateException to MigrateSkipProcessException is wrong when used in file entity migrations as in core (although my contrib use case will need MigrateSkipProcessException). So, I think file_copy needs an option to choose how heavy an exception to throw. Thinking about that...

File migration needs to be able to fetch remote files

Nope, it doesn't. The migrate module needs to have support for remote but migrate_drupal doesn't. So we should remove urlencode entirely from this patch.

That would be a regression - right now we support a remote source_base_path for upgrades. Every time I test D6 migration on my site, I pass http://mikeryan.name/. This may not a good idea for sites with a large volume of files, or where the source server is distant network-wise, but is more than adequate for your typical personal blog site, and I don't want to require people to have to copy their files to the destination server by scp or similar means.

> I pass http://mikeryan.name/.

Whatever you pass that to should change the migration to use the download plugin.

Backing off on the exception changes - MigrateException is right for the core upgrade use case, I'll open a follow-up for supporting a skip-process option for other use cases.

This needed a reroll for this morning's commits, interdiff couldn't apply last patch. Removed a redundant test (when I moved the copy plugin and its test from file to migrate, the original test also remained under file).

Hrrm, how did I manage that? Here's the patch...

Follow-up #2766369: Add skip-method option to file_copy and download process plugins opened.

Botched test namespaces... Plus, there was an odd unrelated failure in there, let's see how we do this time...

Looks like a random test failure, requeued.

> That would be a regression - right now we support a remote source_base_path for upgrades.

That only works because of this very problematic urlencode hack. Clean up and re-add this feature in a followup.

1. +++ b/core/modules/file/migration_templates/d6_file.yml
   @@ -1,21 +1,40 @@
   +# Every migration that references a file by fid should specify this migration
   +# as an optional dependency.
   

   I'd like this to say something like "Every migration that references files migrated from a Drupal 6 site should specify this migration as an optional dependency." Otherwise it sounds like *any* migration that touches files in any way needs to be dependent on d6_file.

2. +++ b/core/modules/file/migration_templates/d6_file.yml
   @@ -1,21 +1,40 @@
   +    # represents the fully qualified path relative to which uris in the files
   

   s/uris/URIs. Should also include something like "See source_full_path configuration in this migration's process pipeline", so that people can look at an example.

3. +++ b/core/modules/file/migration_templates/d7_file.yml
   @@ -6,10 +6,28 @@ migration_tags:
   +    # source_base_path must be set by the tool configuring this migration. It
   +    # represents the fully qualified path relative to which uris in the files
   +    # table are specified, and must end with a /.
   

   Let's do here as in the d6_file migration.

4. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -27,50 +16,9 @@
   +  const URI_COLUMN = 'uri';
   

   Not a big deal, but why is this a constant? Could it perhaps be a default value in $this->configuration instead?

5. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -82,8 +30,10 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +    // we're loading the matching uri.
   

   s/uri/URI

6. +++ b/core/modules/file/tests/src/Kernel/Migrate/d6/FileMigrationTestTrait.php
   @@ -14,15 +14,23 @@ protected function setUpMigratedFiles() {
   +      $source['constants']['source_base_path'] = DRUPAL_ROOT . '/';
   

   DRUPAL_ROOT is deprecated, we are supposed to be using \Drupal::root() now.

7. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
   @@ -0,0 +1,59 @@
   +  protected $migrationConfiguration = [
   +    'id' => 'test',
   +  ];
   

   Needs @inheritdoc.

8. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
   @@ -0,0 +1,59 @@
   +   * Cover various encoding scenarios.
   +   */
   +  public function testUrls() {
   

   Ideally this should take advantage of PHPUnit's @dataProvider pattern (I think it will result in better error reporting), but that's just a nice-to-have.

9. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
   @@ -0,0 +1,59 @@
   +   *   Url to be encoded.
   

   s/Url/URL

10. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
    @@ -0,0 +1,59 @@
    +   *   Encoded url.
    

    s/url/URL

11. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,207 @@
    +/**
    + * Copy a file from one place into another.
    + *
    + * @MigrateProcessPlugin(
    + *   id = "file_copy"
    + * )
    + */
    +class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterface {
    

    Can there be a little more explanation here? What should the input values be, what will be returned, what error conditions are there, etc.

12. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,207 @@
    +      throw new MigrateException("File '$source' does not exist.");
    

    Exception messages are not supposed to end with periods.

13. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,207 @@
    +    if (!file_prepare_directory($dir, FILE_CREATE_DIRECTORY)) {
    

    Should we maybe use the FILE_MODIFY_PERMISSIONS flag here as well (and adjust the exception message accordingly)?

14. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,207 @@
    +    throw new MigrateException("File $source could not be copied to $destination.");
    

    Let's remove the period from the end of the message.

15. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
    @@ -0,0 +1,39 @@
    +    // Only apply to a full URL.
    +    if (strpos($value, '://') > 0) {
    

    Maybe we should sanity-check that $value is, in fact, a string.

16. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
    @@ -0,0 +1,39 @@
    +      $components = explode('/', $value);
    +      foreach ($components as $key => $component) {
    +        // urlencode() would convert spaces to + signs.
    +        $components[$key] = rawurlencode($component);
    +      }
    

    Why are we doing explode() here? It seems like that might produce unexpected consequences with certain inputs. Can we use parse_url() instead? That gives us the added benefit of being able to error if the input value cannot be parsed as a URL.

17. +++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
    @@ -137,6 +137,16 @@ protected function prepareMigrations(array $id_mappings) {
    +  protected function processMigration($migration) {
    

    $migration should be type hinted.

18. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,148 @@
    +    foreach ($this->localFileDataCopyProvider() as $data) {
    

    Why not use the @dataProvider annotation on this method? There's no need to call the data provider directly.

19. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,148 @@
    +      list($source_path, $destination_path, $expected) = $data;
    

    Given the assertion messages later on, I'm a little unclear what $expected is supposed to represent.

20. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,148 @@
    +  protected function localFileDataCopyProvider() {
    

    We should test what happens with remote URIs.

21. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,148 @@
    +    foreach ($this->localFileDataMoveProvider() as $data) {
    

    Use @dataProvider rather than calling the data provider method directly.

22. +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
    @@ -0,0 +1,59 @@
    +  protected $migrationConfiguration = [
    

    Needs @inheritdoc.

23. +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
    @@ -0,0 +1,59 @@
    +  public function testUrls() {
    

    Not a big deal, but this method should be using the @dataProvider pattern.

24. +++ b/core/modules/user/migration_templates/d6_user_picture_file.yml
    @@ -6,22 +6,40 @@ source:
    +    # represents the fully qualified path relative to which uris in the files
    

    s/uris/URIs

25. +++ b/core/modules/user/migration_templates/d6_user_picture_file.yml
    @@ -6,22 +6,40 @@ source:
    +  # Every migration that references a file by fid should specify d6_file as an
    

    Again, let's adjust this comment so it's clear that only migrations which reference files *imported from a D6 site* depend on this migration.

minor tweak for compatibility with current 8.2.x . Will follow up to address @phenaproxima's comments, but this way it's clearer

Tried to take care of the easier feedback items.
1-3 : adjusted comments for clarity.
4 : Left it as a constant for now. I'm also interested in the reasoning.
5-7: applied the suggested changes.
8 : used @dataProvider, and reused the comments as value array keys.
9-10: applied the suggested changes.
11: Left it alone for now, but agreed this needs more clarity.
12-15: applied the suggested changes.
16: Switched to parse_url(), but we still need to explode on forward slashes so we're only url encoding path components.
17-18: applied the suggested changes.
19: Given that $expected was always always TRUE, I just removed it entirely and used assertTrue instead.
20: Still needs to be written.
21-25: applied the suggested changes.

STILL TO DO:

4.

+++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
@@ -27,50 +16,9 @@
+  const URI_COLUMN = 'uri';

Not a big deal, but why is this a constant? Could it perhaps be a default value in $this->configuration instead?

11.

+++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
@@ -0,0 +1,207 @@
+/**
+ * Copy a file from one place into another.
+ *
+ * @MigrateProcessPlugin(
+ *   id = "file_copy"
+ * )
+ */
+class FileCopy extends ProcessPluginBase implements ContainerFactoryPluginInterface {

Can there be a little more explanation here? What should the input values be, what will be returned, what error conditions are there, etc.

16. core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php:35-47 I would love to avoid using explode entirely. Better ideas? This still feels ugly to me.

20. core/modules/migrate/tests/src/Unit/process/CopyFileTest.php needs a test for remote URIs.

Thanks for snapping up this patch, @ohthehugemanatee!

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,207 @@
   +    if (!file_prepare_directory($dir, FILE_CREATE_DIRECTORY + FILE_MODIFY_PERMISSIONS)) {
   

   The constants need to be bitwise-ORed, not added.

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -0,0 +1,207 @@
   +      throw new MigrateException("Could not create, or could not write to directory '$dir'");
   

   Let's change this to: "Could not create or write to directory '$dir'"

1. Bitwise operator fixed
2. Changed the wording

Also tried to make it pass testing:
* I guess pecl_http is in my environment, but not the test environment. :| Replaced http_build_url with GuzzleHttp\Psr7\Uri::fromParts() .
* removed the explode() logic from URL handling entirely.
* fixed special character handling.

- whoops, missed that exception text change.
- added a remote file test

I had another look at the two "to do" items.
- const vs config: replaced the const with a config item that has a default value. It's pretty rare that someone will need a custom column, but hey let's support it.
- core/modules/migrate/src/Plugin/migrate/process/FileCopy.php "Can there be a little more explanation here?": Looking at all the other process plugins we provide, none of the others provide more explanation than this. If we want to raise the bar for how much info to include in process plugin docblocks, that's another ticket IMO.

If this passes tests, this is ready for review.

le sigh - caught some other 8.2.x commits in my last patch. Fixed in this version.

double le sigh - attaching a valid patchfile this time.

Can there be a little more explanation here?

Just want to point out that there is an issue proposing to put the documentation for the process plugins in the codebase #2776179: [meta] Add process plugin documentation to the codebase.

* Fixed the ipmlementation of @dataprovider for testSuccessfulMoves
* moved remote file copy into its own test

I don't know why it's having permission trouble, though. I get a different permission error on my local (Drupal\Tests\migrate\Unit\process\CopyFileTest::testSuccessfulCopies with data set "local to local copy" ('/core/modules/simpletest/file...st.jpg', 'public://file1.jpg') PHPUnit_Framework_Exception: sh: 1: : Permission denied). But when I pull the code out and run it outside of the test context, it works fine. Ideas?

Refactored CopyFile test to extend FileTestBase, because they have convenient file creation and setup methods. Not sure if this is bad form or not, but they're Doing It Right and I want some of that.

anyway, i'm having trouble with phpunit getting "permission denied" for specific tests on my local setup... even when the tests are empty. So sorry for the spam, testbot, but I need you to run this for me.

+++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
@@ -27,50 +20,18 @@
+  protected $uri_column;
...
+    $this->uri_column = isset($configuration['uri_column']) ? $configuration['uri_column'] : 'uri';

The column name business comes from https://www.drupal.org/node/2695297#comment-11192565 point #2. It's a hypothetical and unlikely-in-the-real-world edge case and I don't think it's worth having configuration for it - let's just drop it and hardcode uri.

I've been finding these interdiffs hard to read. So, I made myself a few and the most useful is between #109 and #123. Maybe it will help someone else. And the difference between #103 and #108 was only in the migration yml files, removing the 'provider: migrate_drupal' statements.

1. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
   @@ -0,0 +1,65 @@
   +   *   Url to be encoded.
   

   s/Url/URL

2. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
   @@ -0,0 +1,65 @@
   +   *   Encoded url.
   

   s/Url/URL

3. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
   @@ -0,0 +1,154 @@
   +    $this->filesystem = $this->container->get('file_system');
   ...
   +    );
   

s/filesystem/fileSystem. But the test still fails.

@quietone: Thanks for the interdiff! That helped me get caught up on the changes since I left...

Having trouble with tests locally (insisting the temporary stream wrapper isn't there, and not finding t() in FileTestBase), so letting the testbot try this iteration.

1. Removed the uri column name configuration business - this is not something that's likely to ever be needed, and even if it were per-migration configuration doesn't make sense, it's something you would want to override in your destination class.
2. Removed the remote file test - I think figuring out a proper means to test remote files is out-of-scope here (remember, the file copying functionality is not new, it's just moved), and it's definitely harder than just pointing at a real file on a live server and assuming it will always be accessible, in whatever environment tests are running. Trust me, I tried back in #2553311: Drupal file migrations need to urlencode paths. #2613722: Figure out how to test migration of remote (http://) files is the place to try to figure this out.
3. Short array syntax in the process plugin tests.

Yep, helps to actually upload the files...

+++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
@@ -0,0 +1,145 @@
+  public function testSuccessfulCopies($data) {
+      list($source_path, $destination_path) = $data;
+      $this->doImport($source_path, $destination_path);
+      $message = sprintf('File %s exists', $destination_path);
+      $this->assertTrue(is_file($destination_path), $message);
+      // Make sure we didn't accidentally do a move.
+      $this->assertTrue(is_file($source_path), $message);
+  }

Just noticed extra indentation here - let's fix this on the next iteration.

OK, the error here is the same one I saw locally running CopyFileTest via run-tests.sh:

PHP Warning: file_put_contents(): Unable to find the wrapper "temporary" - did you forget to enable it when you configured PHP? in /var/www/html/core/tests/Drupal/KernelTests/Core/File/FileTestBase.php on line 197

FileTestBase extends Drupal\KernelTests\KernelTestBase. Note that Drupal\simpletest\KernelTestBase registers the temporary stream wrapper in setUp():

    $this->registerStreamWrapper('temporary', 'Drupal\Core\StreamWrapper\TemporaryStream');

registerStreamWrapper() is part of the simpletest KernelTestBase, but not the Drupal KernelTestBase. From that registerStreamWrapper() implementation, I added

    $this->container->get('stream_wrapper_manager')->registerWrapper('temporary', 'Drupal\Core\StreamWrapper\TemporaryStream', StreamWrapperInterface::LOCAL_NORMAL);

to CopyFileTest::setUp(), but I'm still getting the complaint about the temporary stream wrapper not existing. I don't see what I'm missing, but here's the patch using this...

Digression: StreamWrapperManager::registerWrapper() begins with:

    if (in_array($scheme, stream_get_wrappers(), TRUE)) {
      stream_wrapper_unregister($scheme);
    }

So, you call registerWrapper() with a scheme that's already registered, and it unregisters it? Surprise!

So, you call registerWrapper() with a scheme that's already registered, and it unregisters it? Surprise!

Oh, never mind, I see that it unregisters so it can reregister with (potentially) new info...

Thanks @mikeryan and @quietone!

The remaining issue is

PHP Fatal error: Call to undefined function Drupal\KernelTests\Core\File\t() in /var/www/html/core/tests/Drupal/KernelTests/Core/File/FileTestBase.php on line 198

The offending line is from FileTestBase::createUri() :

    $this->assertTrue(is_file($filepath), t('The test file exists on the disk.'), 'Create test file');

I'm not sure what to do here.

* Why would that line fail here, but pass during the File tests? I don't see other children of FileTestBase doing anything special to make t() available.
* Why is FileTestBase using procedural t() in the first place? AFAIK it should be using the translation service, or StringTranslationTrait... but correcting FileTestBase is out of scope for this task.
* FileTestBase::createUri() is calling assertTrue() wrong anyway. There should only be 2 parameters, not 3... but correcting FileTestBase is out of scope for this task.
* FileTestBase::createUri() is the only example I can find in core of using string translation in a test result message. So I don't think this is required. But correcting FileTestBase is out of scope for this task.

We could re-implement createUri() on CopyFileTest, using the translation service. It would mean we can use the translation service for all our other messages. But that seems unnecessary, and I don't see other core tests using string translation for messages.

Attached a patch that removes string translation from that line in FileTestBase::createUri(). There are a couple of other instances of t() in there, but not in methods we use. Is this the Right Way to do this?

OK, now we're back to
Unable to find the wrapper "temporary" - did you forget to enable it when you configured PHP?

I'm stumped on this. We register the "temporary" wrapper in setUp. Besides, none of the other children of FileTestBase have this problem, and they don't explicitly register any wrappers.

I feel like there's something fundamental I'm missing here. What's the difference between the run contexts of Drupal\KernelTests\Core\File\UnmanagedCopyTest and Drupal\Tests\migrate\Unit\process\CopyFileTest ?

Debugged this and learned that the dataProvider functions were being called before the setup. And without the setup the file system wasn't setup and thus the failures. I talked to mikeryan on IRC about this and we agreed to rework this to remove the dataproviders. I'm sure someone will speak up if that isn't the way to proceed.

I started from the patch in #134 in order to restore the string translation removed in #138. Seems to me that if the translation needs work, it is for another issue.

Oops. Here is the correctly named interdiff.

The patch in #142 passed, so setting to NR.

Awesome, good find!

So I'm trying to test with this config

process:
  field_file:
    -
      plugin: default_value
      default_value: /var/www/drupal/public_html/web/core/misc/druplicon.png
    -
      plugin: file_copy

But no file is saved to the node. Will dig into it this afternoon.

@ohthehugemanatee: You're not passing a destination path to the file_copy plugin.

Try this:

process:
  field_file:
    -
      plugin: default_value
      default_value: 
        - /var/www/drupal/public_html/web/core/misc/druplicon.png
        - public://druplicon.png
    -
      plugin: file_copy

aw jeez @mikeryan way to chime in with the easy answer. :)

(and thanks)

why back to 8.2.x? This is a >1500 line patch... does this mean it can make it into 8.2?

I've been trying hard to get all BC breaks into 8.2.x so we can upgrade the basic framework to beta experimental status and let everyone count on a stable API from 8.2.x forward, I *really* don't have to wait another 6 months for that.

Tested with these configs:

Straightforward file copy: WORKS

  field_file:
    -
      plugin: default_value
      default_value:
        - /var/www/drupal/public_html/web/core/misc/druplicon.png
        - public://druplicon.png
    -
      plugin: file_copy
    -
      plugin: entity_generate

File copy with move=TRUE: WORKS

  field_file:
    -
      plugin: default_value
      default_value:
        - public://druplicon.png
        - public://druplicon_moved.png
    -
      plugin: file_copy
      move: true
    -
      plugin: entity_generate

File copy with rename=TRUE: BROKEN

First I created public://drupalicon_overwriteme.png . Then ran this migration.

  field_file:
    -
      plugin: default_value
      default_value:
        - /var/www/drupal/public_html/web/core/misc/druplicon.png
        - public://druplicon_overwriteme.png
    -
      plugin: file_copy
      rename: true
    -
      plugin: entity_generate

It created drupalicon_overwriteme_0.png as expected, but the file path returned was still public://druplicon_overwriteme.png . So we need work on FileCopy::writeFile() to have it return the actual destination. Or maybe just call file_destination() to get the foreseen destination in ::transform(). This means we need to review the test, too.

@ohthehugemanatee, thx for the testing.

This should address the destination file name problem.

ahh @quietone you beat me to the punch! :) I actually did a double take looking at your patch just now, because we even chose the same variable names.

Attaching my version of the patch here, too. It's extremely similar. IMO a tiny bit preferable because of a simpler FileCopy::writeFile and switching from assertTrue(is_file()) to assertFileExists().

changes to FileCopy:
* simplified FileCopy::writeFile() to just choose between file_unmanaged_move and file_unmanaged_copy, returning the actual destination.
* Corresponding changes to ::transform to return the destination we get back from ::writeFile(), instead of the requested destination.

changes to CopyFileTest:
* doImport now returns the destination that the importer returns
* every time we run doImport, I added a test to make sure the returned value is as expected (except for testNonExistentSourceFile)
* replaced manual file existence checks with assertFileExists and assertFileNotExists.

This patch passed all three manual tests above.