Process plugins should not be allowed to skip rows

To sum up today's discussions: it was suggested to return empty if the lookup fails.

But if a required subthing in a thing (like a field in an entity) has a default then empty will get overridden by default. This very well might not be what we want -- we just changed the value of a subthing which might not meet user expectations at all. The decision at the time was to better not a thing at all than have subtly altered things.

We do not have in band signaling for saying "hey destination, we don't have an idea of what this thing is, you probably want to skip it or tell the user it is missing or something".

So we went with out of band signaling ie throwing an exception.

To sum up: the problem here is of user expectations. What stands out more? Missing things or changed things? What is easier to fix? I do not know. I am just asking the questions.

Let's look closely at the scenario where this happens:

1. A field in a migration is a reference to an item being imported by another migration.
2. The incoming reference cannot be resolved to an already-migrated item. This may be because there is a) a dangling reference in the source data, or b) we have chicken-and-egg issues where the target item exists in the source but has not yet been migrated.
3. Some (content) migrations support stubbing in these cases - we create a stub item, and if b) applies it will eventually be overwritten by the real target item and all is well. If a) applies, we're left with stub items in the database, which hopefully are easily identifiable (on the contrib side, we should think about tools for identifying and removing unresolved stubs).
4. If the incoming reference could not be resolved, and the destination does not support stubbing, we fall through.

In Migrate 2, when we fell through we simply returned an empty value - and that was generally fine. The main problem in practice was being slow to notice missing references, and being somewhat tedious to track them down. That can be helped by logging something like "Failure to resolve source ID 123 in field_related_article" to the migration message table.

In D8 the scenario is a little more serious, though. Now we're migrating configuration as well as content, which is both non-stubbable and more likely to give problems if omitted. Although, how often does one deal with references to configuration? I'm having trouble thinking of use cases other than text formats (which is where this trouble began) and roles, and in those cases I think we can fall through to a sensible default.

Regardless, the scope of the processing pipeline is the migration of a single field, and that is not the place to decide to scrap the entire entity being migrated. The place to do that would be the destination plugin - more particularly, I would hope that when we ultimately call entity_save() (or whatever core API is appropriate), it would fully validate the object we're giving it and throw an exception if anything is missing or invalid - ideally migration wouldn't have to do its own validation. Whether the core APIs do that consistently is an open question that needs investigation.

Back to the issue at hand - given the scope of the processing pipeline, I believe the necessary changes to the migration plugin itself are simply:

1. Log the situation to the message table
2. Return an empty value instead of throwing SkipRow (leaving it to the migration itself whether to apply a default value).

However, at the very least it seems we have tests that were depending on that SkipRow, and perhaps there are other components of migrate/migrate_drupal that need to account for the empty values, so there is still some work to go here.

Continuing on #7 based on the problems raised by #8 -- if what we need is detailed signalling from process to destination why don't we beef up Row to maintain a list of destination properties where the processing has failed and let destination work from that? Destination can add a default or throw away the thing as it fits. There's more than just Migration, StaticMap can also fail for example.

The next step is retitleing the issue to remove MigrateSkipRowException ; to change every instance to a new MigrateFailedProcessException ; to catch this and add the info to Row via a new method and to change the affected destinations to act sensibly. I would also rename MigrateSkipProcessException to MigrateFinishedProcessException and carefully change the current calls to either MigrateFinishedProcessException or MigrateFailedProcessException .

> There is already a MigrateSkipProcessException for precisely this purpose -- can't we just use that?
> skip_row_if_not_set and skip_row_on_empty plugins

Skipping the rest of process might happen because it's just not relevant (success) or because of failure. The current skip process exception doesn't make this distinction. The plugins probably should stay but they should get a fail or somesuch configuration option so that if a skip occurs then we throw the fail exception and consequently record a failure .

Issue summary sounds great, +1 on that approach.

Just to clarify, "abort the processing pipeline" is referring to the pipeline for the one field right? Not the entire migration.

I don't see how wanting to start D7 makes this postponed, lets at least leave it as active in case someone else wants to pick it up.

Personally, I disagree. As you said last night on the call, this patch broke a large amount of the tests, every test broke in a different way which made them hard to debug and not something we could ask for help on from novices.

If we have 50 hard problems now, we'll have a 100 when D7 is done.

Reroll

Try again.

Third time lucky?

We need to get back to this, at least the original issue. While perhaps we shouldn't prevent all process plugins from skipping rows (perhaps there are custom scenarios where this makes sense), we should not have the default behavior of the core process plugins (I'm looking at you, migration!) do this - rather, they should return NULL and the migration config can choose (by adding skip_row_if_empty) to skip if desired.

Right now this is causing trouble with #2484405: User pictures do not need a dedicated process plugin, forcing us to implement a custom process plugin for user picture fields just to catch the SkipRow.

Going back over all the discussion here and remembering now all the stuff around the general principle, ugh. I think, though, that in the narrow case of the migration process plugin that it should definitely not throw SkipRow itself (if skipping the row makes sense for a particular migration, it can always follow the migration plugin with skip_row_is_empty), so I think I'll open a separate issue just for that narrow use case.

(if skipping the row makes sense for a particular migration, it can always follow the migration plugin with skip_row_is_empty)

I quite like that approach, +1

Yes, +1 on not changing this functionality. I'm having to override the CCKFile right now so it *will not* catch the skip row exception.