Problem/Motivation

The EntityFile currently does:

$file = $row->getSourceProperty($this->configuration['source_path_property']);

This is wrong, the destinations should never look at the source directly. Its also a complicated and opinionated destination, if you already have the files in the right place for example, its quite tricky to use. Also, it is the only destination plugin which contains a preg_replace clearly showing it's more process than destination.

Proposed resolution

  1. Create a FileCopy process plugin to handle most of the move/copy logic in the destination.
  2. Remove the urlencode feature, this can be done in the source if you need it.
  3. Remove destination_path_property from EntityFile, it doesn't need to be configurable, if you need that, write a new destination.
  4. Remove source_path_property from EntityFile, no longer needed.
  5. Remove source_base_path from EntityFile, no longer needed.
  6. Fix file_umanaged_copy() to support remote file, new issue?

We can now use the process plugin like so:

source_full_path:
  plugin: concat
  delimiter: /
  source:
    - constants/source_base_path
    - source_relative_path
uri:
  plugin: file_copy
  source: @source_full_path

Remaining tasks

Write patch

User interface changes

n/a

API changes

Yes, if you were using the destination in a custom migration, you may need to now use the process plugin in your migration yml.

Data model changes

n/a

Files: 
CommentFileSizeAuthor
#183 refactor_entityfile_and-2695297-183.patch52.38 KBmikeryan
#183 interdiff.txt1.67 KBmikeryan
#176 interdiff.txt7.29 KBmikeryan
#176 refactor_entityfile_and-2695297-176.patch52.26 KBmikeryan
#168 refactor_entityfile_and-2695297-168.patch54.29 KBohthehugemanatee
#156 interdiff.txt855 bytesohthehugemanatee
#156 refactor_entityfile_and-2695297-156.patch54.28 KBohthehugemanatee
#154 refactor_entityfile_and-2695297-154.patch54.09 KBohthehugemanatee
#154 interdiff.txt5.07 KBohthehugemanatee
#153 interdiff.txt5.22 KBquietone
#153 refactor_entityfile_and-2695297-154.patch54.13 KBquietone
#144 interdiff-134-142.txt5.21 KBquietone
#142 refactor_entityfile_and-2695297-142.patch53.61 KBquietone
#138 refactor_entityfile_and-2695297-138.patch54.39 KBohthehugemanatee
#134 interdiff.txt1.83 KBmikeryan
#134 refactor_entityfile_and-2695297-134.patch53.75 KBmikeryan
#130 interdiff.txt7.75 KBmikeryan
#130 refactor_entityfile_and-2695297-129.patch53.55 KBmikeryan
#128 interdiff-109-123.txt24.01 KBquietone
#123 interdiff.txt2.79 KBohthehugemanatee
#123 refactor_entityfile_and-2695297-123.patch55.62 KBohthehugemanatee
#121 interdiff.txt768 bytesohthehugemanatee
#121 refactor_entityfile_and-2695297-118.patch55.46 KBohthehugemanatee
#113 refactor_entityfile_and-2695297-113.patch58.82 KBohthehugemanatee
#110 interdiff.txt13.7 KBohthehugemanatee
#110 refactor_entityfile_and-2695297-110.patch54.34 KBohthehugemanatee
#109 refactor_entityfile_and-2695297-109.patch52.97 KBohthehugemanatee
#104 interdiff.txt996 bytesmikeryan
#104 refactor_entityfile_and-2695297-103.patch53.14 KBmikeryan
#101 refactor_entityfile_and-2695297-100.patch53.16 KBmikeryan
#94 interdiff.txt24.12 KBmikeryan
#94 refactor_entityfile_and-2695297-94.patch58.54 KBmikeryan
#82 interdiff.txt598 bytesmikeryan
#81 refactor_entityfile_and-2695297-81.patch50.88 KBmikeryan
#78 2695297-76-78-interdiff.txt528 bytesSharique
#78 refactor_entityfile_and-2695297-78.patch50.9 KBSharique
#65 interdiff.txt520 bytesmikeryan
#65 refactor_entityfile_and-2695297-65.patch50.69 KBmikeryan
#63 interdiff.txt478 bytesmikeryan
#63 refactor_entityfile_and-2695297-63.patch50.72 KBmikeryan
#61 interdiff.txt7.17 KBmikeryan
#61 refactor_entityfile_and-2695297-61.patch50.71 KBmikeryan
#58 interdiff.txt7.3 KBmikeryan
#58 refactor_entityfile_and-2695297-58.patch48.57 KBmikeryan

Comments

benjy created an issue. See original summary.

benjy’s picture

Issue summary: View changes
benjy’s picture

Issue summary: View changes
heddn’s picture

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.

vasi’s picture

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.

benjy’s picture

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

Well we could keep it although this is Drupal's EntityFile, i'm not sure it's needed. Also, we maybe be able to make it so you can just map to a new property and work it out from there once we refactor into a process plugin.

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.

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

vasi’s picture

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.

mikeryan’s picture

mikeryan’s picture

Issue tags: +Migrate critical
mikeryan’s picture

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.

mikeryan’s picture

Status: Active » Needs review
FileSize
21.09 KB

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...

Status: Needs review » Needs work

The last submitted patch, 11: refactor_entityfile_and-2695297-11.patch, failed testing.

benjy’s picture

I think this is looking pretty good, lets try get it green then i'll do a full review.

quietone’s picture

quietone’s picture

Status: Needs work » Needs review
FileSize
541 bytes

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.

quietone’s picture

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

Status: Needs review » Needs work

The last submitted patch, 16: refactor_entityfile_and-2695297-14.patch, failed testing.

mikeryan’s picture

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.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
40.35 KB
21.91 KB

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...

Status: Needs review » Needs work

The last submitted patch, 19: refactor_entityfile_and-2695297-19.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
40.89 KB
2.63 KB

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

Status: Needs review » Needs work

The last submitted patch, 21: refactor_entityfile_and-2695297-21.patch, failed testing.

vasi’s picture

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.

vasi’s picture

Status: Needs work » Needs review
vasi’s picture

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.

The last submitted patch, 14: refactor_entityfile_and-2695297-14.patch, failed testing.

The last submitted patch, 23: 2695297-23.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2695297-25.patch, failed testing.

quietone’s picture

Status: Needs work » Needs review
FileSize
43.6 KB
1.28 KB

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.

Status: Needs review » Needs work

The last submitted patch, 29: refactor_entityfile_and-2695297-29.patch, failed testing.

mikeryan’s picture

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

mikeryan’s picture

Status: Needs work » Needs review
FileSize
44.48 KB
749 bytes

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...

Status: Needs review » Needs work

The last submitted patch, 32: refactor_entityfile_and-2695297-32.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
44.47 KB
945 bytes

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

Status: Needs review » Needs work

The last submitted patch, 34: 2695297-34.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review
FileSize
47.82 KB
5.81 KB

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

vasi’s picture

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?

quietone’s picture

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".

Status: Needs review » Needs work

The last submitted patch, 36: 2695297-36.patch, failed testing.

vasi’s picture

Status: Needs work » Needs review

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

vasi’s picture

Ok, I think this one should work.

quietone’s picture

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.

mikeryan’s picture

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.

benjifisher’s picture

Status: Needs review » Needs work
  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.

Ryan Weal’s picture

mikeryan’s picture

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

mikeryan’s picture

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.

benjifisher’s picture

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

mikeryan’s picture

Status: Needs work » Needs review
FileSize
48.65 KB
5.39 KB

Last couple of comments addressed, here we go.

benjifisher’s picture

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.

quietone’s picture

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?

quietone’s picture

And same results as above for D7->D8.

mikeryan’s picture

@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.

benjy’s picture

  1. --- a/core/modules/file/migration_templates/d6_file.yml
    +++ b/core/modules/file/migration_templates/d6_file.yml
    
    @@ -9,13 +9,26 @@ source:
    +  source_full_path:
    ...
    +      plugin: d6_file_source
    
    --- a/core/modules/file/migration_templates/d7_file.yml
    +++ b/core/modules/file/migration_templates/d7_file.yml
    
    @@ -9,7 +9,20 @@ source:
    +  source_full_path:
    ...
    +      plugin: concat
    

    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?

    1. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
      @@ -82,7 +25,9 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
      +    $destination = $row->getDestinationProperty('uri');
      

      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' ?

    2. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,202 @@
      +    // If the start and end file is exactly the same, there is nothing to do.
      +    if ($this->isLocationUnchanged($source, $destination)) {
      +      return $destination;
      +    }
      +
      +    $replace = $this->getOverwriteMode($row);
      

      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.

    3. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,202 @@
      +    if (!$success) {
      +      $dir = $this->getDirectory($destination);
      +      if (file_prepare_directory($dir, FILE_CREATE_DIRECTORY)) {
      +        $success = $this->writeFile($source, $destination, $replace);
      +      }
      +      else {
      +        throw new MigrateException("Could not create directory '$dir'");
      +      }
      +    }
      +
      +    if ($success) {
      +      return $destination;
      +    }
      +    else {
      ...
      +    }
      

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

      if ($this->writeFile($source, $destination, $replace)) {
        return $destination;
      }
      
      $dir = $this->getDirectory($destination);
      if (!file_prepare_directory($dir, FILE_CREATE_DIRECTORY)) {
        throw new MigrateException("Could not create directory '$dir'");  
      }
      
      if ($this->writeFile($source, $destination, $replace)) {
        return $destination;
      }
      
      throw new MigrateException("File $source could not be copied to $destination.");
      
    vasi’s picture

    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.

    heddn’s picture

    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.

    heddn’s picture

    Status: Needs review » Needs work

    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.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    48.57 KB
    7.3 KB

    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.

    vasi’s picture

    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.

    mikeryan’s picture

    mikeryan’s picture

    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.

    Status: Needs review » Needs work

    The last submitted patch, 61: refactor_entityfile_and-2695297-61.patch, failed testing.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    50.72 KB
    478 bytes

    Missed one d6_file_source...

    quietone’s picture

    @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.

    mikeryan’s picture

    Nit fixed, thanks!

    quietone’s picture

    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.

    phenaproxima’s picture

    Assigned: Unassigned » phenaproxima

    Assigning to myself for review.

    phenaproxima’s picture

    Status: Needs review » Needs work

    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.

    mikeryan’s picture

    + $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.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    51.07 KB

    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.

    Status: Needs review » Needs work

    The last submitted patch, 70: refactor_entityfile_and-2695297-70.patch, failed testing.

    mikeryan’s picture

    FileSize
    9.67 KB

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

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    51.02 KB
    7.16 KB

    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.

    mikeryan’s picture

    Note that I've added a change record.

    benjy’s picture

    Status: Needs review » Needs work

    This looks like it is about ready to me, just a few nitpicks on code style and some extra comments.

    1. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,213 @@
      +    if ($this->configuration['move']) {
      +      return (boolean) file_unmanaged_move($source, $destination, $replace);
      +    }
      +    else {
      +      $destination = file_destination($destination, $replace);
      +      return @copy($source, $destination);
      +    }
      ...
      +    if (!empty($this->configuration['rename'])) {
      +      return FILE_EXISTS_RENAME;
      +    }
      +    return FILE_EXISTS_REPLACE;
      

      Lets drop the else { on the first block so this code is a consistent style

    2. +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,213 @@
      +    if (substr($dir, -3) == '://') {
      +      return $this->fileSystem->realpath($dir);
      +    }
      +    else {
      +      return $dir;
      +    }
      ...
      +    }
      +    else {
      +      return FALSE;
      +    }
      

      Same here.

    3. +++ b/core/modules/file/src/Plugin/migrate/process/UrlEncode.php
      @@ -0,0 +1,36 @@
      +      // Actually, we don't want certain characters encoded.
      

      Do we know why this is the case?

    4. +++ b/core/modules/file/tests/src/Kernel/Migrate/CopyFileProcessPluginTest.php
      @@ -0,0 +1,150 @@
      +   * ¶
      

      Extra white space.

    Sharique’s picture

    Updated patch, addressed 1,2 and 4.

    benjy’s picture

    Assigned: phenaproxima » Unassigned
    +++ b/core/modules/file/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,209 @@
    +  protected function isLocationUnchanged($source, $destination) {
    ...
    +    }
    +    else {
    +      return FALSE;
    +    }
    

    I still see one here. I think this patch is RTBC for me, would be good to get a comment on the URL encode stuff though.

    Sharique’s picture

    benjy’s picture

    Status: Needs review » Reviewed & tested by the community

    Rest looks good, some more manual testing would be great but the best way for that is probably to have it committed.

    mikeryan’s picture

    + // 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.

    mikeryan’s picture

    Clarified the comment.

    mikeryan’s picture

    FileSize
    598 bytes

    Oops, ignore the interdiff in the last comment.

    mikeryan’s picture

    ressa’s picture

    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...

    vasi’s picture

    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.

    mikeryan’s picture

    @ressa - Also see the related change record.

    mikeryan’s picture

    mikeryan’s picture

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

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 81: refactor_entityfile_and-2695297-81.patch, failed testing.

    phenaproxima’s picture

    Status: Needs work » Reviewed & tested by the community

    Um...no it didn't.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs review
    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?

    mikeryan’s picture

    Status: Needs review » Needs work

    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.

    chx’s picture

    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.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    58.54 KB
    24.12 KB

    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...

    alexpott’s picture

    @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);
    -    }
    

    Status: Needs review » Needs work

    The last submitted patch, 94: refactor_entityfile_and-2695297-94.patch, failed testing.

    chx’s picture

    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.

    mikeryan’s picture

    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.

    chx’s picture

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

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

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    24.12 KB

    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).

    mikeryan’s picture

    FileSize
    53.16 KB

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

    mikeryan’s picture

    Status: Needs review » Needs work

    The last submitted patch, 101: refactor_entityfile_and-2695297-100.patch, failed testing.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    53.14 KB
    996 bytes

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

    Status: Needs review » Needs work

    The last submitted patch, 104: refactor_entityfile_and-2695297-103.patch, failed testing.

    mikeryan’s picture

    Status: Needs work » Needs review

    Looks like a random test failure, requeued.

    chx’s picture

    > 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.

    phenaproxima’s picture

    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.

    ohthehugemanatee’s picture

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

    ohthehugemanatee’s picture

    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.

    phenaproxima’s picture

    Status: Needs review » Needs work

    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'"

    The last submitted patch, 110: refactor_entityfile_and-2695297-110.patch, failed testing.

    ohthehugemanatee’s picture

    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.

    ohthehugemanatee’s picture

    Status: Needs work » Needs review
    FileSize
    59.14 KB
    3.08 KB

    - 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.

    ohthehugemanatee’s picture

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

    Status: Needs review » Needs work

    The last submitted patch, 115: refactor_entityfile_and-2695297-116.patch, failed testing.

    ohthehugemanatee’s picture

    double le sigh - attaching a valid patchfile this time.

    The last submitted patch, 114: refactor_entityfile_and-2695297-115.patch, failed testing.

    quietone’s picture

    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.

    Status: Needs review » Needs work

    The last submitted patch, 117: refactor_entityfile_and-2695297-117.patch, failed testing.

    ohthehugemanatee’s picture

    Status: Needs work » Needs review
    FileSize
    55.46 KB
    768 bytes

    * 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?

    Status: Needs review » Needs work

    The last submitted patch, 121: refactor_entityfile_and-2695297-118.patch, failed testing.

    ohthehugemanatee’s picture

    Status: Needs work » Needs review
    FileSize
    55.62 KB
    2.79 KB

    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.

    Status: Needs review » Needs work

    The last submitted patch, 123: refactor_entityfile_and-2695297-123.patch, failed testing.

    The last submitted patch, 123: refactor_entityfile_and-2695297-123.patch, failed testing.

    mikeryan’s picture

    +++ 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.

    The last submitted patch, 123: refactor_entityfile_and-2695297-123.patch, failed testing.

    quietone’s picture

    FileSize
    24.01 KB

    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.

    mikeryan’s picture

    Status: Needs work » Needs review

    @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.
    mikeryan’s picture

    Yep, helps to actually upload the files...

    mikeryan’s picture

    +++ 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.

    The last submitted patch, 113: refactor_entityfile_and-2695297-113.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 130: refactor_entityfile_and-2695297-129.patch, failed testing.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    53.75 KB
    1.83 KB

    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!

    mikeryan’s picture

    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...

    Status: Needs review » Needs work

    The last submitted patch, 134: refactor_entityfile_and-2695297-134.patch, failed testing.

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    ohthehugemanatee’s picture

    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?

    ohthehugemanatee’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 138: refactor_entityfile_and-2695297-138.patch, failed testing.

    ohthehugemanatee’s picture

    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 ?

    quietone’s picture

    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.

    Status: Needs review » Needs work

    The last submitted patch, 142: interdiff-134-142.patch, failed testing.

    quietone’s picture

    FileSize
    5.21 KB

    Oops. Here is the correctly named interdiff.

    quietone’s picture

    Status: Needs work » Needs review

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

    ohthehugemanatee’s picture

    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.

    mikeryan’s picture

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

    mikeryan’s picture

    Version: 8.3.x-dev » 8.2.x-dev

    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
    
    ohthehugemanatee’s picture

    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?

    mikeryan’s picture

    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.

    ohthehugemanatee’s picture

    Status: Needs review » Reviewed & tested by the community

    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’s picture

    Status: Reviewed & tested by the community » Needs work
    quietone’s picture

    Status: Needs work » Needs review
    FileSize
    54.13 KB
    5.22 KB

    @ohthehugemanatee, thx for the testing.

    This should address the destination file name problem.

    ohthehugemanatee’s picture

    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.

    mikeryan’s picture

    Status: Needs review » Needs work

    So, between the two nearly-identical patches, I prefer the return of the file_unmanaged_move() result in #154, but not:

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -120,14 +123,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
    -    return @copy($source, $destination);
    +    return file_unmanaged_copy($source, $destination, $replace);
    

    Unfortunately, file_unmanaged_copy() won't work with remote URLs (we actually do have a reason for copy() here).

    ohthehugemanatee’s picture

    Status: Needs work » Needs review
    FileSize
    54.28 KB
    855 bytes

    replaced that file_unmanaged_copy with copy

    chx’s picture

    Yeah. This was done before and it was wrong then as well. Do not skip Drupal core APIs because they do not do what you need from them. That's what contrib does: a fortunate hack that happens to do what you want to do -- until you get badly burned later. I already mentioned remote capabilities do not belong to this patch and you need a followup for a download process plugin utilizing Guzzle.

    Alternatively, file a patch to fix the file API.

    mikeryan’s picture

    Status: Needs review » Needs work

    The use of copy() to support remote files is not being introduced by this patch, it's being moved, and I don't want to expand the scope of this issue - let's fix what this patch is intended to do (moving configuration and processing out of the destination plugin and into their logical places in source and process) and followup with a more robust means of handling remote files.

    mikeryan’s picture

    Status: Needs work » Needs review
    chx’s picture

    Status: Needs review » Needs work

    > The use of copy() to support remote files is not being introduced by this patch

    Nope it was introduced long ago and I didn't catch it then and it was a hack then and it's an excellent time to get rid of it. As I said in #97 core has public and temporary wrappers and while downloading is a good idea it simply doesn't belong here.

    mikeryan’s picture

    Status: Needs work » Needs review

    The current patch needs someone else to review it.

    I still disagree this is the place to change the copy() behavior.

    chx’s picture

    Status: Needs review » Postponed

    Sure. https://www.drupal.org/node/2244555#comment-11503439 is the place to get rid of it then.

    ohthehugemanatee’s picture

    Status: Postponed » Needs review

    I don't understand why the status goes to "postponed". We were already using copy() for remote files, and this patch maintains that behavior. It in no way depends on a better implementation of remote file handling.

    I agree that we should get rid of copy(), but "change how we handle remote files", "make file_unmanaged_copy support remote files", or "add a process plugin for guzzling remote files" are all way outside the scope of this issue. Whichever approach we take, it's also not a blocker. We have a working patch that addresses this issue as scoped, and doesn't change the existing approach to remote files.

    quietone’s picture

    Sorry, but two small items.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,214 @@
    +    // We attempt the copy first to avoid calling file_prepare_directory() any
    

    Can this be changed to reflect that writeFile will do a copy or move? s/copy/copy or move/ ?

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,214 @@
    +   * @return bool
    

    But sometimes not a boolean, sometimes the destination. What is the standard for this? I don't see anything in https://www.drupal.org/node/1354#return. File.inc just has '@return' without a data type for cases like this but other files in core have multiple return data types separated by an or, i.e. '@return string|bool'. Personally, I prefer the later, showing all possible data types that will be returned.

    chx’s picture

    > We were already using copy() for remote files, and this patch maintains that behavior.

    And that was wrong. Adding anything to core is trivial, even if it's wrong and removing it as this issue shows is impossible.

    Do whatever. Unfollowed.

    chx’s picture

    Parting shot: if someone has basic auth (for example for allowing the migration of private files from remote) then this hack breaks. The clear solution is , as I repeatedly said is to remove hackish remote capabilities and in a followup add a file download plugin.

    ohthehugemanatee’s picture

    ohthehugemanatee’s picture

    Updated comments per @quietone's suggestions. Needs someone else to test...

    Status: Needs review » Needs work

    The last submitted patch, 168: refactor_entityfile_and-2695297-168.patch, failed testing.

    quietone’s picture

    Retesting. The error was unrelated to this patch.

    quietone’s picture

    Status: Needs work » Needs review

    Tests passing, back to needs review.

    chx’s picture

    Issue summary: View changes
    Chi’s picture

    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?

    @ohthehugemanatee This might be your issue too.
    https://github.com/sebastianbergmann/environment/pull/13

    ohthehugemanatee’s picture

    Wow, fantastic find @Chi! This is indeed the problem. Nice job figuring that out.

    phenaproxima’s picture

    Status: Needs review » Needs work

    Oh, we are getting there!

    1. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
      @@ -82,7 +29,9 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
      +    $destination = $row->getDestinationProperty('uri');
      

      I'd like to add a sanity check here -- if $destination is empty, can we throw a MigrateException? I ask because file migration is prone to failure because of all the moving parts, and any extra defenses against weird, hard-to-trace failures will be good things.

    2. +++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
      @@ -84,9 +84,7 @@ public function prepareRow(Row $row) {
      +    $path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);
      

      I'd like to wrap this logic in a protected method so that, if the source_base_path constant is not set for some reason (or otherwise invalid), we can throw an exception. Play defense!

    3. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
      @@ -0,0 +1,65 @@
      +  /**
      +   * Cover various encoding scenarios.
      +   * @dataProvider urlDataProvider
      +   */
      

      Nit: There should be a blank line between the description and @dataProvider.

    4. +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
      @@ -0,0 +1,65 @@
      +  /**
      +   * Perform the urlencode process plugin over the given value.
      +   *
      +   * @param string $value
      +   *   URL to be encoded.
      +   *
      +   * @return string
      +   *   Encoded URL.
      +   */
      +  protected function doTransform($value) {
      +    $executable = new MigrateExecutable($this->getMigration(), new MigrateMessage());
      +    $row = new Row([], []);
      +
      +    return (new UrlEncode([], 'urlencode', []))
      +      ->transform($value, $executable, $row, 'foobaz');
      +  }
      

      Kind of a nitpick, but why is this in its own method? Can't we move all this directly into testUrls()?

    5. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,214 @@
      +    // If we're stubbing a file entity, return a uri of NULL so it will get
      

      s/uri/URI

    6. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,214 @@
      +    // We attempt the copy/move first to avoid calling file_prepare_directory() any
      

      Nit: The "any" makes this line exceed 80 characters.

    7. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
      @@ -0,0 +1,214 @@
      +    // If writeFile didn't work, make sure there's a writable directory in place.
      

      Same thing here, with that last period.

    8. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
      @@ -0,0 +1,60 @@
      +    if (is_string($value) && strpos($value, '://') > 0) {
      

      This seems a bit too trusting to me. Can we use parse_url() (or a helper method wrapping around it) here to prove that it really is a URL? And if it's not, I think we should maybe throw \InvalidArgumentException rather than return the input value.

      In other words, let's just do everything we do in the if block anyway, for all input values :)

    9. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
      @@ -0,0 +1,60 @@
      +      $url_parts_to_encode = array('path', 'query', 'fragment');
      +      foreach ($parsed_url as $parsed_url_key => $parsed_url_value) {
      +        if (in_array($parsed_url_key, $url_parts_to_encode)) {
      +          // urlencode() would convert spaces to + signs.
      +          $urlencoded_parsed_url_value = rawurlencode($parsed_url_value);
      +          // Restore special characters depending on which part of the URL this is.
      +          switch ($parsed_url_key) {
      +            case 'query':
      +              $urlencoded_parsed_url_value = str_replace('%26', '&', $urlencoded_parsed_url_value);
      +              break;
      +
      +            case 'path':
      +              $urlencoded_parsed_url_value = str_replace('%2F', '/', $urlencoded_parsed_url_value);
      +              break;
      +          }
      +
      +          $parsed_url[$parsed_url_key] = $urlencoded_parsed_url_value;
      +        }
      +      }
      

      TBH, I find this loop a little awkward. I'd prefer it to be straight-up procedural for clarity and simplicity's sake...but I concede that this is more a code style nitpick than anything. So consider this request a "nice to have", rather than "back to the ol' drawing board".

    10. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
      @@ -0,0 +1,60 @@
      +      $value = (string)Uri::fromParts($parsed_url);
      

      Nit: Need a space after (string).

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

      I'd rather name this prepareMigration() than processMigration(). The word "process" sounds like it's going to do some seriously heavy lifting, and I'm guessing that is not the point of this method.

    12. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
      @@ -0,0 +1,156 @@
      +  /**
      +   * @var FileSystemInterface
      +   */
      

      Missing description.

    13. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
      @@ -0,0 +1,156 @@
      +    $data_sets = [
      +      // Test a local to local copy.
      +      [
      +        $this->root . '/core/modules/simpletest/files/image-test.jpg',
      +        'public://file1.jpg'
      +      ],
      +      // Test a temporary file using an absolute path.
      +      [
      +        $file_absolute,
      +        'temporary://test.jpg'
      +      ],
      +      // Test a temporary file using a relative path.
      +      [
      +        $file_absolute,
      +        'temporary://core/modules/simpletest/files/test.jpg'
      +      ],
      +    ];
      

      This belongs in a data provider method, unless there's a compelling reason not to use one...but I don't see any real reason not to...? (It seems OK to me to move the $file preparation logic into the data provider. If I'm not mistaken, it will only run once.)

    14. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
      @@ -0,0 +1,156 @@
      +    $data_sets = [
      +      // Test a local to local copy.
      +      [
      +        $local_file,
      +        'public://file1.jpg'
      +      ],
      +      // Test a temporary file using an absolute path.
      +      [
      +        $file_1_absolute,
      +        'temporary://test.jpg'
      +      ],
      +      // Test a temporary file using a relative path.
      +      [
      +        $file_2_absolute,
      +        'temporary://core/modules/simpletest/files/test.jpg'
      +      ],
      +    ];
      

      Ditto for all this.

    15. +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
      

      I think this file might be duplicated in the File module, in this same patch.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    52.26 KB
    7.29 KB

    +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
    @@ -82,7 +29,9 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
    + $destination = $row->getDestinationProperty('uri');

    I'd like to add a sanity check here -- if $destination is empty, can we throw a MigrateException? I ask because file migration is prone to failure because of all the moving parts, and any extra defenses against weird, hard-to-trace failures will be good things.

    Done.

    +++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
    @@ -84,9 +84,7 @@ public function prepareRow(Row $row) {
    + $path = str_replace($this->configuration['constants']['source_base_path'], NULL, $path);

    I'd like to wrap this logic in a protected method so that, if the source_base_path constant is not set for some reason (or otherwise invalid), we can throw an exception. Play defense!

    It's valid for source_base_path to not be set (when the source paths are absolute).

    +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
    @@ -0,0 +1,65 @@
    + /**
    + * Cover various encoding scenarios.
    + * @dataProvider urlDataProvider
    + */

    Nit: There should be a blank line between the description and @dataProvider.

    Done.

    +++ b/core/modules/file/tests/src/Unit/Plugin/migrate/process/UrlEncodeTest.php
    @@ -0,0 +1,65 @@
    + /**
    + * Perform the urlencode process plugin over the given value.
    + *
    + * @param string $value
    + * URL to be encoded.
    + *
    + * @return string
    + * Encoded URL.
    + */
    + protected function doTransform($value) {
    + $executable = new MigrateExecutable($this->getMigration(), new MigrateMessage());
    + $row = new Row([], []);
    +
    + return (new UrlEncode([], 'urlencode', []))
    + ->transform($value, $executable, $row, 'foobaz');
    + }

    Kind of a nitpick, but why is this in its own method? Can't we move all this directly into testUrls()?

    Not sure if I did this, or maybe ohthehugemanatee, but I don't see a compelling reason to move it. I think it does help clarify what this chunk of code is doing.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,214 @@
    + // If we're stubbing a file entity, return a uri of NULL so it will get

    s/uri/URI

    Done.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,214 @@
    + // We attempt the copy/move first to avoid calling file_prepare_directory() any

    Nit: The "any" makes this line exceed 80 characters.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
    @@ -0,0 +1,214 @@
    + // If writeFile didn't work, make sure there's a writable directory in place.

    Same thing here, with that last period.

    Done.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
    @@ -0,0 +1,60 @@
    + if (is_string($value) && strpos($value, '://') > 0) {

    This seems a bit too trusting to me. Can we use parse_url() (or a helper method wrapping around it) here to prove that it really is a URL? And if it's not, I think we should maybe throw \InvalidArgumentException rather than return the input value.

    In other words, let's just do everything we do in the if block anyway, for all input values :)

    It doesn't have to be an URL, it may be a local file path.

    +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
    @@ -0,0 +1,60 @@
    + $url_parts_to_encode = array('path', 'query', 'fragment');
    + foreach ($parsed_url as $parsed_url_key => $parsed_url_value) {
    + if (in_array($parsed_url_key, $url_parts_to_encode)) {
    + // urlencode() would convert spaces to + signs.
    + $urlencoded_parsed_url_value = rawurlencode($parsed_url_value);
    + // Restore special characters depending on which part of the URL this is.
    + switch ($parsed_url_key) {
    + case 'query':
    + $urlencoded_parsed_url_value = str_replace('%26', '&', $urlencoded_parsed_url_value);
    + break;
    +
    + case 'path':
    + $urlencoded_parsed_url_value = str_replace('%2F', '/', $urlencoded_parsed_url_value);
    + break;
    + }
    +
    + $parsed_url[$parsed_url_key] = $urlencoded_parsed_url_value;
    + }
    + }

    TBH, I find this loop a little awkward. I'd prefer it to be straight-up procedural for clarity and simplicity's sake...but I concede that this is more a code style nitpick than anything. So consider this request a "nice to have", rather than "back to the ol' drawing board".

    Please, this section has already had enough out-of-scope refactoring as it is (the functionality is not new, only moved, and I wish now that suggestions of refactoring it had been moved to a follow-up).

    +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
    @@ -0,0 +1,60 @@
    + $value = (string)Uri::fromParts($parsed_url);

    Nit: Need a space after (string).

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

    I'd rather name this prepareMigration() than processMigration(). The word "process" sounds like it's going to do some seriously heavy lifting, and I'm guessing that is not the point of this method.

    +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,156 @@
    + /**
    + * @var FileSystemInterface
    + */

    Missing description.

    Done.

    +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,156 @@
    + $data_sets = [
    + // Test a local to local copy.
    + [
    + $this->root . '/core/modules/simpletest/files/image-test.jpg',
    + 'public://file1.jpg'
    + ],
    + // Test a temporary file using an absolute path.
    + [
    + $file_absolute,
    + 'temporary://test.jpg'
    + ],
    + // Test a temporary file using a relative path.
    + [
    + $file_absolute,
    + 'temporary://core/modules/simpletest/files/test.jpg'
    + ],
    + ];

    This belongs in a data provider method, unless there's a compelling reason not to use one...but I don't see any real reason not to...? (It seems OK to me to move the $file preparation logic into the data provider. If I'm not mistaken, it will only run once.)

    +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
    @@ -0,0 +1,156 @@
    + $data_sets = [
    + // Test a local to local copy.
    + [
    + $local_file,
    + 'public://file1.jpg'
    + ],
    + // Test a temporary file using an absolute path.
    + [
    + $file_1_absolute,
    + 'temporary://test.jpg'
    + ],
    + // Test a temporary file using a relative path.
    + [
    + $file_2_absolute,
    + 'temporary://core/modules/simpletest/files/test.jpg'
    + ],
    + ];

    Ditto for all this.

    There is a compelling reason, back in #142 (tl;dr - data providers are called before setUp()).

    +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php

    I think this file might be duplicated in the File module, in this same patch.

    Done.

    phenaproxima’s picture

    Okay, fair enough. Assuming the tests pass, I think this is RTBC as far as I'm concerned. I'd like to get confirmation on that from @benjy, though.

    chx’s picture

    Can we recognize #2783075: Add a "download" process plugin, remove remote capability from FileCopy is fairly ready and nuke remote in here already?

    phenaproxima’s picture

    I am very much in favor of a dedicated plugin for remote file handling, rather than over-engineering FileCopy.

    But I'm also pragmatically considering the fact that this issue is a Migrate-critical blocker and #2783075: Add a "download" process plugin, remove remote capability from FileCopy is not. If it's not going to be much trouble to remove the remote file support from this patch (and I don't see why it would be), I say let's do it now and get the other patch in ASAP. If it does turn out to be difficult, let's commit this patch as it is and remove remote support from FileCopy in #2783075: Add a "download" process plugin, remove remote capability from FileCopy.

    phenaproxima’s picture

    Status: Needs review » Reviewed & tested by the community

    Discussed in the weekly Migration hangout with the other maintainers. We've decided to proceed with this patch as-is for now, then move remote file support into the download plugin as a follow-up. This is not to say that remote file support is unimportant (precisely the opposite, in fact), but this is blocking further work, it's Migrate critical, and in light of that it's OK for this not to be architecturally pure in the short term.

    chx’s picture

    > it's OK for this not to be architecturally pure in the short term.

    In my opinion it is worth more to be architecturally clean finally (the hack was added two years ago) and lack remote support for the short term. But. That's old term thinking apparently. Welcome to Drupal 8. I have not changed the status of this issue and I will not do more than voice my protest.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
    1. This change looks really nice and it is great that there is a change record to help early adopters
    2. +++ b/core/modules/migrate/tests/src/Unit/process/CopyFileTest.php
      @@ -0,0 +1,158 @@
      +use Drupal\Core\File\FileSystemInterface;
      

      Not used.

    3. +++ b/core/modules/migrate/tests/src/Unit/process/UrlEncodeTest.php
      @@ -0,0 +1,65 @@
      +      'The definitive use case - encoding spaces in URLs' => array('http://example.com/url with spaces.html', 'http://example.com/url%20with%20spaces.html'),
      

      I think we should add a sub directory with spaces here. Just to make sure %2f's are replaced with / properly.

    mikeryan’s picture

    Status: Needs work » Needs review
    FileSize
    1.67 KB
    52.38 KB

    Final (Mike crosses his fingers) patch...

    phenaproxima’s picture

    Status: Needs review » Reviewed & tested by the community

    Rockin'.

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed and pushed 78714b9 to 8.3.x and f3de7e6 to 8.2.x. Thanks!

    • alexpott committed 78714b9 on 8.3.x
      Issue #2695297 by mikeryan, ohthehugemanatee, quietone, vasi, Sharique,...
    neclimdul’s picture

    The friendly neighborhood "this broke the phpunit runner" notice. At least in a bare checkout running the unit test suite. Seems this is a kernel test in the unit test suite which causes problems if you don't have all the requirements to run a kernel test setup.

    neclimdul’s picture

    Posted follow up.

    mikeryan’s picture

    What's the policy on publishing change records? When there's a release?

    Speaking of follow-up issues: #2793731: Obsolete destination properties in d6_file/d6_user_picture_file.

    benjifisher’s picture

    According to https://groups.drupal.org/node/402688,

    Starting February 14, issues that require API change records must have these change records written before patches are committed. This is Drupal 8 core's valentine to contributed modules. :)

    That is more than 2.5 years old, and I do not see anything official on d.o. :-(

    Another thing that may or may not be out of date is my recollection that any issue with the "needs change record" tag should be marked critical.

    mikeryan’s picture

    We have the draft change record - the question was when to publish it. The relevant clause:

    Once the patch for the issue is committed, the core maintainer will simply mark the issue fixed (like any regular issue). The "Published" checkbox can then be checked to make the change record appear in the main Change record list.

    So, I guess it should be published now. The open question is who does that (which is vague above) - my assumption was always that the core maintainer who marked the issue fixed would do that. But, maybe it's up to the patch author (or one of the patch authors)?

    benjy’s picture

    I think anyone can publish them, I've published the CR just now. https://www.drupal.org/node/2747025

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.