Allow process plugins to flag a row to be skipped

So, I'm a bit torn on this last commit here. I would like to fix sub_process such that calling a row skip in the new way here skips the row inside the sub_process, as I would think would be expected. Currently throwing a skip row inside the sub_process causes the exception to bubble all the way back up to the main migration executable and causes the entire parent row to be skipped, which I always considered to be a bug, but there's a fine line between a bug and established behavior, and I need opinions.
Having sub_process respect the new flag differently than a MigrateSkipRowException is not a BC break, however, when we then switch the core plugins to to use the new flag, they force new behavior in sub_process which needs to either be determined to be a bug fix (and is fine to do) or a BC break (in which case we would need to keep the current behavior by having sub_process pass the skip flag up to the parent, and either introduce a new configuration option for sub_process to decide whether to keep the skip internal or pass it up the chain, or to deprecate sub_process and replace it with a new plugin that just keeps the skip internal, neither of which should probably be handled in this issue)

I classify fixing sub_process as a bug fix and not a behavior change, but I have to acknowledge that the fix may be sufficiently disruptive that we may have to treat it as a behavior change and go the long route, so I'm hoping the other maintainers and community can weigh in here.

Yeah, with that last patch, the behavior definitely does change, that was what I was playing with.

Previously sub_process would let the exception pass all the way through to the executable where the message would be logged, now it's not being passed all the way through. I didn't log it in this patch, we could easily enough, but it would still be a behavior change since we aren't passing the skip all the way back up.

I'm leaning towards passing it all the way back up with the message and fixing sub_process in a new issue.

I know, but I really need Benji to do a fuller review so we can see if I need to do any other architecture changes that might affect the CR before I write it.

Made some adjustments that broke some tests, and I'm still debating if they are worth keeping at all. Currently, when SourcePluginBase runs all the prepare_row hooks, if a hook return false, the remaining hooks still run, and the row is skipped, while if a hook throws a skip_row exception, the remaining hooks are skipped and we go straight to the catch block after the hook invocations. The original version of this patch left it so that calling $row->skip in a prepare row hook still let all the remaining hooks run before deciding to skip the row. While this difference in behavior from the exception is fine (We don't rely on the behavior in core, and it's okay for the replacement of a deprecated api to work slightly differently) for performance reasons, it seems like it would be nice to stop hook processing once a skip is flagged. The latest commits do that, but broke a few tests that will be a little tricky to rework. I think ultimately we should keep the efficient behavior and eventually deprecate the ability for prepare_row hooks to return FALSE as a signal to skip a row, requiring them to use $row->skip instead, but I don't have time to fix all the tests today.

It doesn't appear to need a rebase. That issue was merged on Jan 12, and I picked this back up and rebased on Jan 13. It's still NW because the refactor needs more tests, and I'm playing with adding the ability to set the row map status on skip, which would let us deprecate using MigrateException inside a process plugin to skip a row and mark it failed. Hoping to get to it this week.

Assigning to me for more tests and features, and requested changes.

I would argue it is exactly what exceptions are made for: to break out from the ordinary code flow.

But we don't want to break out of the normal code flow here. Skipping rows and finalizing pipelines doesn't happen because of an exceptional condition, bad data, or an error. These are operations that are done by process functions in their normal workflow, and expected and supported behaviors. They are operations on an object, and we have the object available, so it should be able to know its status.

SubProcess shows the need for manually propagate instead of the exception automatically doing so.

I'm not sure precisely what you mean here. I'll assume that you are referring to the fact that the MR has to set the row to skip when it encounters a condition where it wants the row to skip. This is first off only being done to preserve buggy behavior that was partially exposed by this issue. #3365895: When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row is attempting to fix that bug. It's a prime example of the issues caused by using exceptions to signal things in the normal flow of execution. Some called function deep inside of subprocess throws a Skip Row exception, and subprocess didn't think to handle it, and it bubbles up and breaks the whole outer row. This makes it more difficult to code those kind of bugs.

Look at the number of checks for skipping before and after. What use cases are made possible by adding this complexity?

This code is in a transition state, but the ultimate goal here is to remove the usage of MigrateSkipRowException and MigrateException (the latter of which does the same thing as MigrateSkipRowException, but allows you to set an arbitrary row status while bypassing the row) in process plugins, and replace them with a single set of api calls on the row object itself. This makes logical sense to me, as the row object should know and report its status. As far as the number of skip checks, with the exception of sub_process, these are 1:1 with the existing try-catch blocks which are going away. and #3365895: When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row currently adds a try-catch block there too, which will be replaced by a skip check either here or there depending on which issue is committed first. The endgame code will be able to replace multiple catch blocks with a single API check, so we are reducing complexity.

What problems are being solved here?

See above comments on reducing complexity and preventing unintended bugs

Also, I am not sure why is it called "getSkip". Let's look at events: the method is called isPropagationStopped so the analogue would be isProcessingSkipped.

Alwasy happy to debate naming conventions, the PR is a WIP, but initially it as a simple getter and was named as such, but something like isRowSkipped would work too. I'm going to see what it all looks like once the migrateException stuff is included and see what makes the most sense.