skip_on_empty documentation is confusing when skipping a process

Here's a first approach.

I highly suspect this should be closed as "Works as Designed".

Please see #2692373: Value should be null when is produced skip process →

Maybe documentation could be clarified somewhere though?

It does work as designed, but we should use the issue to update the documentation to be more clear. The proposed solution would be a backwards compatibility break.

I am a little disturbed that the proposed patch doesn't cause any test errors showing the BC break though. I understand why, nearly all migrate tests involve migrating into new entities, there are precious few tests involving migrating data into existing entities, and it's not the first issue where someone has proposed a patch that changes behavior on running migrations into existing entities that doesn't trigger test failures. Technically core doesn't actually provide a way to rerun migrations with update without drush, and the core migration runners in migrate_drupal_ui expect you are migrating into an empty database, so we don't really test the case where you are re-running a migration with updates, but lots of people do, so maybe we should add some tests around it.

I'll argue a bit more, reminding that even I myself have not yet concluded on what should be the desired behaviour.

The way it is implemented now has an internal inconsistency to the plugin itself. The plugin can be configured to run with:

 * - method: (optional) What to do if the input value is empty. Possible values:
 *   - row: Skips the entire row when an empty value is encountered.
 *   - process: Prevents further processing of the input property when the value
 *     is empty.

The "row" method skips the target entity completely, no entity is being created. But the "process" method does not actually skip the field, it does update it with null value, which is equivalent to the source actually having something null-ish.
If the "process" behaviour is what we want, then shouldn't the "row" method also save a new entity with all the values null/default?

I understand the "skip" intention to be "don't touch it":
Row on new entity: Don't touch it, so don't save a new one.
Row on existing entity: Don't touch it, so don't update it at all.
Process on new entity: Don't touch it, so let it get it's default value as it has been declared in the field configuration.
Process on existing entity: Don't touch it, so let it keep what it already has, if it has something.

@efpapado I had the same reaction as you when I went deeper into my test on this plugin. To solve this, I created a destination plugin that does the job of checking before overwriting the data, I can share it if you want.

Updating the documentation to make it clear that skipping a process sets the value of the process to NULL.

Seems like a simple change. Updated the tag too.

Migrating Pittsburgh 2023 to Pittsburgh2023 tag for cleanup

The value is set to NULL in \Drupal\migrate\MigrateExecutable::processPipeline. I think this change is a bit misleading. I've made a change. What do you think?

As a practical matter, I'm not sure the extra clarification is worth it given ✨ Allow process plugins to stop further processing on a pipeline RTBC is also rtbc, and would actually change this to have the plugin set the null directly. We would have to change the documentation back again in that issue.

Still, this issue is likely to be backported and that one won't be, so maybe it would be good for 10.1 or 9.5, but I'm not sure the technical distinction is worth noting, even for the backport, as the functional effects are the same.

I too am not sure making the distinction is worth changing the docs for. SkipOnEmpty has been around since 2015 and there there hasn't been a demand for a change.

This can then be postponed on ✨ Allow process plugins to stop further processing on a pipeline RTBC or the change here be moved there. I am in two minds about that. For now I will postpone it.

And moving to the migration system where the migrate maintainers work.

I meant postponed.

The issue this was postponed on has been fixed. Un-postponing!

I see two comments form migrate system maintainers indicating that documenting the distinction may not be worth it. I think that's especially true given the details of how the related issue was fixed.

So should this be closed out then?

For #19

mikelutz → changed the visibility of the branch 3336070-skiponempty-is-ignored to hidden.

mikelutz → changed the visibility of the branch 3336070-skiponempty-is-ignored to active.

This can be committed with review. The distinction mentioned in #19 is the distinction beteween the plugin setting the value to NULL vs the Executable setting the value to NULL when the SkipProcess Exception was thrown. Personally I don't think there is a distinction because throwing a SkipProcessException set the value to NULL, so I feel like the plugin was responsible for the NULL anyway. But the point is moot, as ✨ Allow process plugins to stop further processing on a pipeline RTBC refactored away the exception and now the process plugin does literally set the value to NULL. I consider the doc change trivial, but it's fine to commit, as it does improve the docs.

Thanks for taking a look @mikelutz

Backported to 10.3.x as a docs fix.

Committed and pushed a810a91658 to 11.x and d5c1624e7f to 11.0.x and 9dcf9acca4 to 10.4.x and d95f041bda to 10.3.x. Thanks!

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