EntityFile destination incorrectly assumes temporary files are absolute paths

At first glance, the patch looks good to me (and testbot has been passing the one that isn't marked "FAIL"...hmmm). Here it is again with a few relatively minor modifications (removed deprecated file-related functions calls in favor of injecting the file_system service).

EntityFileTest should now pass.

I added a not-very-elegant kludge to EntityFile::import() to work around a quirk of file_prepare_directory(). If FileSystemInterface::dirname() returns a scheme -- for example, passing public://foobaz.txt will return public:// --, file_prepare_directory() will reject that URI and EntityFile will throw a MigrateException.

The fix checks the output of FileSystemInterface::dirname() against a regex and runs it through FileSystemInterface::realpath() if it's a scheme without a path, before file_prepare_directory() is called.

Updating status.

Changed preg_match() call to substr().

Sure thing. This patch should fail EntityFileTest with an exception like:

Drupal\migrate\MigrateException: Could not create directory <em class="placeholder">public://</em> in Drupal\migrate\Plugin\migrate\destination\EntityFile->import() (line 76 of /path/to/core/modules/migrate/src/Plugin/migrate/destination/EntityFile.php).

...and yet somehow it didn't fail. Strange; it fails quite nicely in my local environment.

Some of @chx's work in #2482329: Remove deprecated calls from EntityFile and make it less local overlaps with the work in #9 -- namely, the removal of deprecated calls and injection of the file_system service. I'm thinking that issue should be postponed until this patch is committed closed, and its fixes integrated into this patch.

This patch smoothes over EntityFile's handling of remote streams by checking if the source and destination URIs are both local. I didn't add any specific test cases for this, because a) I'm not sure what exactly how to go about testing this, and b) there are no remote stream wrappers bundled with core that don't extend LocalStream (which made me throw a WtfException on IRC).

After refactoring EntityFile, this passes the tests on my localhost.

Did a bit of minor refactoring requested by @chx after he reviewed.

Regarding a fail patch -- I'm not sure we need one, because it appears to be flat-out broken in HEAD. On the latest 8.0.x (e0aae8c), MigrateFileTest will fail epically with a fatal error, and EntityFileTest will fail with a warning and a MigrateException ("cannot create directory public://").

After applying the patch in #20, both tests pass with flying colors.

I <3 dependency injection. Updated the patch to inject the string_translation service into EntityFile.

This integrates the changes from #2483939: EntityFile should throw an exception if the source file doesn't exist -- it ensures the source file exists and throws an exception if it doesn't.

At @chx's suggestion, this patch refactors EntityFile so that file_prepare_directory() is only called if needed -- that is, if an initial attempt to save the destination file fails.

This looks awesome. Thanks much. I made two very minor changes please do NOT credit me: one empty() was not necessary and the only change in FileUri was whitespace so I reverted the file.

> Can we not calculate this in the constructor?

When testing, you'd need to mock bundleinfo for no benefit so ... no. The rest, agreed.

Removed translation from the exceptions.

Regarding unit tests for the helper methods -- I'm fully up for it, if it's not too much of a pain (or, for that matter, considered unkosher) to forcibly change those protected methods to public ones for testing.

I added the instanceof assertions because, if the file isn't loaded, the test will try to access a non-object and thus blow up with a fatal error, which is annoying as hell. Adding a fail saves us a trip to the "Clean environment" button. :)

I agree with benjy here.

Removed the instanceof checks from MigrateFileTest, and added unit tests of EntityFile's protected helpers.

Awesome work, this fixes a huge number of errors on my test import.

1. +++ b/core/scripts/run-tests.sh
   @@ -1,3 +1,4 @@
   +#!/usr/bin/env php
   

   I agree but we can't do that here. :)

2. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityFileTest.php
   @@ -39,8 +40,10 @@ class EntityFileTest extends KernelTestBase {
   +    // $this->installConfig(['system', 'file']);
   

   Just remove this.

3. +++ b/core/modules/migrate/tests/src/Unit/destination/EntityFileTest.php
   @@ -157,4 +278,39 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function writeFile($source, $destination, $replace = FILE_EXISTS_REPLACE) {
   +    return parent::writeFile($source, $destination, $replace);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getOverwriteMode(Row $row) {
   +    return parent::getOverwriteMode($row);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDirectory($uri) {
   +    return parent::getDirectory($uri);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isLocationUnchanged($source, $destination) {
   +    return parent::isLocationUnchanged($source, $destination);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isLocalUri($uri) {
   +    return parent::isLocalUri($uri);
   +  }
   +
   

   Why?

4. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php
   @@ -8,6 +8,7 @@
   +use Drupal\file\FileInterface;
   
   @@ -64,6 +65,7 @@ public function testFiles() {
   +
   

   ?

Regarding #3 -- it was requested to have unit tests of those helper methods, which are all protected in EntityFile. So I had to override them with public versions in order to test. Everything else, is valid, and I will re-roll the patch today.

All fixed, except for #3.

Did another review and looks fine. Definitely fixes a lot of failures in my test site migration.

All fixed.

@benjy, I prefer the idea of using reflection to alter the method accessibility, so that's what I did in this patch. I also changed EntityFileTest to a KernelTestBase (turns out it doesn't really need to be a web test, thank Cthulhu) and moved it out of the unit test suite.

Quick re-roll.

*shakes fist at testbot*

Back to RTBC after testbot tantrum.

Committed 6ac8e46 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/migrate/src/Tests/EntityFileTest.php b/core/modules/migrate/src/Tests/EntityFileTest.php
index 2fb4b62..509e98b 100644
--- a/core/modules/migrate/src/Tests/EntityFileTest.php
+++ b/core/modules/migrate/src/Tests/EntityFileTest.php
@@ -112,7 +112,7 @@ public function testWriteFile() {
     $plugin->configuration['move'] = TRUE;
     $this->assertTrue($method->invoke($plugin, 'temporary://baz.txt', 'public://foo.txt'));
 
-    // Trying to move a non-existent file shold return FALSE.
+    // Trying to move a non-existent file should return FALSE.
     $this->assertFalse($method->invoke($plugin, 'temporary://invalid.txt', 'public://invalid.txt'));
 
     // Copying over a file that already exists should replace the existing file.
@@ -193,7 +193,7 @@ public function testIsLocationUnchanged() {
   }
 
   /**
-   * Tests various inovcations of the isLocalUri() method.
+   * Tests various invocations of the isLocalUri() method.
    */
   public function testIsLocalUri() {
     $plugin = $this->destination;

Fixed minor spelling mistakes on commit.