Add skip-method option to file_copy and download process plugins

> However, in contrib we'd like to use this plugin in other contexts - in particular, to copy a file and create a file entity within a file/image field mapping in a node migration

Even for contrib that's a bad, bad idea. Write a file migration and a node migration. Very easy. It's an architectural mess otherwise. There is one destination in a migration. While obviously you can save a file in a process plugin ,there's nothing to stop you, that's a mess. That's not how Drupal 8 migration works.

I've experienced a similar problem when I was working in a migration to Drupal 8.4.5 from a custom mysql database. When a source file does not exist the migration process fails and this stops the migration group from processing.

First I tried throwing MigrateSkipProcessException when the origin file does not exist but transform process continues and destination EntityFile.php throws MigrateException('Destination property uri not provided').

Finally I used MigrateSkipRowException and works fine for me. Row is skipped, destination field leaves empty and group migration completes all configured migrations

We should throw a MigrateSkipRowException everytime a file is not found. As the issue summary proposes, we should have a skip_process_on_failure option. If this option is set to true, we throw a MigrateSkipRowException exception, otherwise we should still throw a MigrateException exception.

Hi, sorry for my bad english.

I have added the skip_process_on_failure option on FileCopy and Download process Plugin, but for now the tests are missing

Thanks @robertom. The patch works well for me.

Re-rolled patch for Drupal 8.6.9

Patch was not applying to 8.6.13 so I rerolled it against 8.8.x.

edit: the patch in #13 still probably apply cleanly on 8.6.13, my issue was somewhere else.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
@@ -26,6 +27,8 @@
+ *   MigrateSkipRowException, if FALSE, throw a MigrateException exception.

@@ -152,7 +155,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+        throw new MigrateSkipRowException("{$e->getMessage()} ($source)");

This is supposed to be a MigrateSkipProcessException, not a MigrateSkipRowException MigrateSkipRowException has the same effect as MigrateException

Also moving to NW because this still needs tests of the new functionality.

Rerolled for 8.9.

Changed MigrateSkipRowException in MigrateSkipProcessException.

Needs to go back to NW for tests, but just letting testbot test first.

To solve the issue I just change the exception to skip row in the core migrate module migrate/process/fileCopy.php as additional to patch ver 15.
It solve my issue.

Thanks for the work here!
Generalizing it would be probably a good idea, changing the title to reflect it.
Having an option to skip either via row or process, and not a raw exception would be great to be able to inspect the migrate messages table after the run, instead of needing to re-run manually the given migration.
Keeping NW for that, and related tests.

A note on how to do this without a patch is to manually verify.
This is not always possible, but should be OK for the simple cases.
E.g. Given an filepath already available on source_full_path destination property, we could do the following

skip_row_if_source_file_does_not_exist:
  - 
    plugin: callback
    source: '@source_full_path'
    callable: file_exists
  - 
    plugin: skip_on_empty
    method: row
    message: 'The source_full_path does not exist on the filesystem'

9. Add a MigrateSkipRowException when the source file is not found to FileCopy.
And then some cleanup and adding tests.

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Download.php
   @@ -26,6 +27,9 @@
   + * - skip_process_on_failure: (optional) Boolean, if TRUE,throw a
   + *   MigrateSkipRowException otherwise throw a MigrateException. Defaults to
   + *   False.
   
   @@ -152,7 +156,12 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +      if (empty($this->configuration['skip_process_on_failure'])) {
   +        throw new MigrateException("{$e->getMessage()} ($source)");
   +      }
   +      else {
   +        throw new MigrateSkipProcessException();
   +      }
   

   The doc doesn't match the actual exceptions thrown

2. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -33,6 +35,9 @@
   + * - skip_process_on_failure: (optional) Boolean, if TRUE, throw a
   + *   MigrateSkipRowException otherwise, throw a MigrateException. Defaults to
   + *   False.
   
   @@ -156,7 +161,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   +
   +    if (empty($this->configuration['skip_process_on_failure'])) {
   +      throw new MigrateException("File $source could not be copied to $destination");
   +    }
   +    else {
   +      throw new MigrateSkipProcessException();
   +    }
   

   Same here.

3. +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php
   @@ -133,7 +138,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
   -      throw new MigrateException("File '$source' does not exist");
   +      throw new MigrateSkipRowException("File '$source' does not exist");
   

   Not sure why we arbitratily change this from a MigrateException to a MigrateSkipRow exception, it seems like we should use the same logic we do below.

   There's also one more MigrateException thrown in this plugin that we didn't change. was that intentional?

I had recently opened #3162959: Migrate should allow continuing the migration on a failed download for this. I only covered the GuzzleException's over there, as that was my use-case. I.e. that patch doesn't contain the exception change, when, for example, the file_exists() on the source file fails in FileCopy. However, over there, I introduced a configurable way to either skip the process or skip the whole row which may be interesting for this.

As it may help others: also see https://www.drupal.org/project/migrate_skip_on_404 (from #3113394: Add process plugin to skip files that don't exist)

Re-rolled the patch for 9.3.x

Hit this once again, but like it was already reported in #8 skipping the process instead of the whole row is problematic for migrations with a file entity destination. So I wanted to bring in the patch from the issue mentioned in #27 to make this behavior configurable, so that either the process or the row is skipped depending on the configuration.

In doing so, however, I realized that in my opinion part of the current patch is incorrect, namely the changing of the MigrateException when a FileException is caught. The FileException is only thrown when there was an error in the actual copying process, not when the source file does not exist. As far as I can tell this was never explicitly discussed here, the discussion was always about the missing source files. Not sure if it makes sense to allow skipping such a FileException, as well - I personally think it doesn't - but either way I don't think it's in scope for this issue.

So removed that from this patch.

Sorry if this is a bit "railroad"-y, I don't mean to step on anyone's toes here by changing course a bit, but I went through all the patches above and there is quite a bit of "ping-pong" going on with different things being changed differently by the various patches without a lot of discussion of the actual details, so this is supposed to be me throwing my preference into the mix, not me invalidating any other suggestion.

Attaching an interdiff to #32 for completeness sake, but this is mostly a different patch.

This still needs tests, so back to NW.
In my opinion, the configurable exception is a bit much here. The skip process exception almost sounds reasonable, but maybe you want to do something else in the process on failure, like provide a default. What really should happen here is the plugin should simply return null, and then let the migration decide what to do, whether that's a skip_on_empty, method: row, or a default_value, or whatever. Of course, for BC reasons we can't do this in these plugins, and there's not an easy way to change that behavior in a backwards compatible way.

So.. no core migrations call download, everything calls file_copy. file copy uses download if it detects a non-local uri, but in practice for me, and in core, I never use download directly, as file_copy covers both the download and local copy use cases.

What I would like to see here is a new process plugin, call it file_transfer, or file_move, retrieve_file, get_file, something like that. The new plugin should pull all the logic in from file_copy and download, but just return NULL if it can't successfully copy the file. Core migrations that use file_copy should use the new plugin, and follow it with a skip_on_empty method: row, which isn't exactly the same behavior as the migrate exception thrown now, but it's generally better, the whole point of this is that it's buggy to fail the whole migration on a failure to download a file from one row. And file_copy and download should both be deprecated in favor of the new plugin. This leaves the responsibility of dealing with what to do on a failed copy to the migration itself, with a default of setting the result to NULL like a skip process would, but still allowing the migration to deal with that null in further plugins in the process chain. Yes, if you do nothing, and leave the NULL, then the file:entity destination will complain, but that's not really the process plugin's business. It has no idea what your destination is, or what you are doing with the file, or anything like that. Only the actual migration is qualified to know if it needs to add special handling for the NULL.

@mikelutz (#18)
For the record.

This is supposed to be a MigrateSkipProcessException, not a MigrateSkipRowException.
MigrateSkipRowException has the same effect as MigrateException

The difference is that with MigrateSkipRowException, the migration still counts as "successful" for the sake of dependent migrations.
With MigrateException, the entire migration counts as failed, which blocks dependent translations.