Allow process plugins to stop further processing on a pipeline

This is an interesting idea, but semantically, I don't think the name MigrateSkipProcessException applies here, since the processing isn't being skipped, just ended early.

How about a different exception called something like MigrateEndProcessException or something like that?

It's better DX to have a separate class name rather than rely on a boolean parameter.

This can be accomplished using the null coalesce plugin as follows

_tmp:
  -  process: my_process
     source: field_x
  -  process: follow_up_process
field_x:
  process: null_coalesce
  source:
    - @_tmp
    - field_x

In general, using exceptions as signals is not good. It exists in migrate for legacy reasons, and is quite difficult to remove in a backwards compatible way, but we do not want to introduce new exceptions-as-signals, nor add features to the existing ones that would make them even more difficult to remove at some point in the future.

I'm not convinced that there is a use case for this that can't be handled in other ways with a properly structured migration. IF we were to choose to implement the functionality (and I'm far from convinced that we should) A better way to go would be to add a `public function final(): bool` method to process plugins, that returns false in the base class that the executable can call after a process runs to allow the process to declare itself to be the final plugin in the process and then the executable can skip all follow-up processes. then your plugin can return either null or the original value, declare itself final, and be done.

And as I type that up, I realize its a good first step in removing MigrateSkipProcess exception altogether, so in the course of writing this comment, I've come full circle, and now think we should introduce a way for a process plugin to declare itself final..

Here's a proof of concept patch. I'm not sure what it will break. For testing, I replace the skip process exception on skip_on_empty with a NULL return value and a declaration of final.

CS fixes

Test Fixes

Found an old issue attempting the original request here that was closed - works as designed

So that works. I like this much better than a new exception. So from here, we need explicit tests of the system, need to decide if we can get away with adding final to the interface directly, or of we will need to add it to the base and trigger an error if a plugin doesn't implement it for now, and then add it to the interface in D10, and we need a change record either way.

Also, given that final has a very specific meaning in php that isn't this, I'm not locked into the semantics of the naming, though I don't have a better suggestion off the top of my head. I'm open to ideas.

I opened two followups to work towards removing MigrateSkipProcessException,
#3247329: Remove usages of MigrateSkipProcessException from core process plugins
and
#3247331: Deprecate MigrateSkipProcessException

Looks good!

> Also, given that final has a very specific meaning in php that isn't this, I'm not locked into the semantics of the naming, though I don't have a better suggestion off the top of my head. I'm open to ideas.

How about 'interrupt'?

+++ b/core/modules/migrate/src/Plugin/MigrateProcessInterface.php
@@ -52,4 +52,13 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+   * Indicates whether additional process plugins should run.

I'd maybe word this differently too, to make it clear that it's *preventing* later plugins declared in the pipeline to run.

I agree, this is very nice!

But what about the companion MigrateSkipRowException? Feels like if we change one we should change both. I don't have a solution to offer at this stage (didn't sleep so well) but this is the only question I have.

I'm not dealing with that here. That one is a lot trickier, because that exception currently bubbles up several more levels in the executable before it is caught, so this trick won't cut it without adding a bunch more ugly code to the executable. We can tackle it at some point, but it would be a separate issue. Skip process only bubbles up one level, so it's much more straightforward.

Even this is going to take some functionality away from the system, since the final/ return null has to happen in the process plugin, but the exception might be thrown in some function the process plugin calls. I don't think we do that in core, other than in the skipInvalid method in migrate_lookup, but that is easily refactored, and if you are throwing that exception deep in external code, you probably need to refactor your code anyway.

I didn't mean to suggest working to change the SkipRow here but felt it worth a mention. And even after sleeping I haven't put my mind to solving how to change that. Right now there are other issues demanding my attention.

I did a search for synonyms of final and other related words and offer 'terminal' and 'ultimate' as options. Not fond of 'interrupt' because that suggests the same action, the pipeline, will start again.

I started the change record.

I'm leaning either towards stopPropegation, to borrow the term for similar functionality in the event system, or (I think better) stopPipeline which is probably a little more accurate and intuitive to what we are doing here.

Reroll the patch for 9.4.x

FWIW, things like this can be accomplished with the if_condition process plugin or the switch_on_condition process plugin from Migration Conditions.

I think this example is nice: https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/mi...

Changed the wording to use isPipelineStopped(), added a new interface to avoid adding to the process plugin interface.

This looks really neat, @mikelutz!

Setting back to NW for the custom commands failure, just in case you didn't see that. I know you're working on lots of issues today. :)

Style fixes.

Really cool stuff, @mikelutz.

Regarding test coverage, obviously you've made explicit updates to the skip_on_empty test to test changes made to the skip_on_empty plugin. Then I would argue there's some implicit test coverage for the changes in MigrateExecutable in that I see about a half dozen uses of skip_on_empty with method process in various migration yamls and those are presumably covered by tests.

However, I'd like to see a test case where the pipeline is stopped but the return value is not null. Maybe we could add a process plugin called stop_pipeline with a property that could be passed as true or false and then add a couple simple tests that leverage so we can test cases where the pipeline value is stopped at a value other than null. Maybe that plugin would be defined in a test module. I see one plugin that's in a test module (test_skip_row_process). Or maybe it goes in the migrate module "just because". I'd probably find a valid use for it when combined with (you guessed it!) Migrate Conditions. :)

Anyway, great work. I'm interested in hearing your thoughts on expanding test coverage. Cheers!

Agree explicit pipeline tests are needed here, I don't think we need a whole dummy plugin, I think prophecies will do just fine.

Added another explicit test of the methods in ProcessPluginBase, and updated the change record.

This work is so nice. I applied the patch to a D10 site and did some manual testing with it.

First, I played with skip_on_empty to try to stumble upon any regressions. In particular I wondered if there was any change in behavior for the case where I pass an array to skip_on_empty where one of the array items is empty. The behavior was the same before and after the patch. (The array is returned with the empty items converted to nulls.)

Second, I created a new plugin that uses the new stop pipeline feature. The code looks like this (it's a natural thing to add to Migrate Conditions):

/**
 * @MigrateProcessPlugin(
 *   id = "stop_on_condition",
 *   handle_multiples = TRUE
 * )
 */
class StopOnCondition extends ProcessPluginWithConditionBase {

  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    if ($this->condition->evaluate($value, $row)) {
      $this->stopPipeline();
    }
    return $value;
  }

}

It does exactly what I wanted it to do. Very cool!

Regarding the code itself...

I think the test coverage here is spot on. My only tentative suggestion would be to use getMockForAbstractClass() in Drupal\Tests\migrate\Unit\process\ProcessPluginBaseTest instead of defining the new class in there. But that's just a matter of taste. It doesn't really matter to me.

I also looked in migrate_tools, which declares its own MigrateExecutable, and confirmed that the processPipeline is not being overridden there. I was worried about the possibility of a knock-on effect there, but it looks like we're safe.

Regarding the CR, skip_on_empty is not mentioned directly. I think we should say that it no longer throws a MigrateSkipProcessException. Or should that be its own CR?

I'm ready to RTBC this except for the nit with the CR.

I considered using mocks there, but I also need reflection to access the protected method, and when I added it all up, just putting an empty concrete class there for testing seemed easiest.

I'm not sure we need a CR for something not throwing an exception anymore, but I don't mind adding it as a footnote to the existing CR. I've updated the CR with that note.

All sounds good to me. Thanks for the awesome work here, @mikelutz.

There's one more bug here I realized. The plugins are kept from row to row, so the member isn't going to be reset on each row. I need to fix that or skip on empty will behave strangely.

I take it back, I fixed it for skip_on_empty by resetting it in the sourcePluginBase transform method, but it is a tricky thing, in general if you are going to use it, you do need to reset it at the top of your transform method, and I hate that.. I'm wondering if we should add a reset method to the new interface that the executable can call so you don't have to worry about it in your transform function..

This is slightly more complicated, but a much better DX than expecting the developer to reset the stoppage at the beginning of the transform method. I'm also not super excited with the way it behaves on multiple when the plugin doesn't handle multiples, but it matched what the exception did, and I don't want to change that or we won't be able to replace the exception with this in the follow-ups, and it would change how skip on empty works now, so I think it's okay. It's better than what we have with the exception anyway.

Good catch, @mikelutz. I only tested with a single row, so I did not encounter the bug with my custom process plugin. D'oh!

Yeah, your custom process would have been broken, which I noticed. I debated leaving it so you are responsible for resetting it yourself, but it was going to lead to way too many issues coming up in slack, so better to have the system clear it between runs, I think.

I went back and tested my custom process plugin against the patch in #36 with more than one row of data and confirmed that the stopPipeline value got stuck.

Then I applied the patch in #42 and the value no longer got stuck. I agree that this is better DX and will make for better SX (Slack Experience). Thanks!

I'm also not super excited with the way it behaves on multiple when the plugin doesn't handle multiples, but it matched what the exception did...and it would change how skip on empty works now, so I think it's okay.

I agree. And I want to re-iterate that this was one of the possible regressions I considered during manual testing and found that skip_on_empty behaves the same way with an array before and after the patches in #36 and #42.

I'll throw this back to RTBC (though I must say I'm always a bit shy/embarrassed when I've already done it once). Cheers!

I started to review the code and was finding quite a few places that I thought needed doc changes. Instead of pushing back to NW I have decided to make them myself. Setting back to NR.

I did some work on the CR. But I think it still needs attention, particularly the part beginning , 'For process plugins extending'.

What happens with the SubProcess plugin? Which pipeline is stopped -- the inner one or the main one? That should be documented somewhere.

The inner one, same as throwing a migrate skip process exception in a subprocess does now.

@quietone is right, the CR needs an update to that section I made changes to that section since I lost updated the change record. I'll try to fix it today, I can add a note about subprocess.

For the CR updates.

Didn't review as this may be one of those migration tickets that require the experts. But can help test where I can.

I like all of @quietone's updates in #46. Thanks!

But after twice RTBC-ing this, I'm wondering if we might want to expand the MigrateExecutable test just a wee bit. What would you think, @mikelutz, about adding a test method to MigrateExecutableTest that processes more than one row? Essentially adding a third test that puts testStopPipeline and testContinuePipeline into a single test. I tried to put something together but was struggling with MethodProphecy. I wanted to use willReturnOnConsecutiveCalls() but that apparently doesn't exist with prophecies.

Maybe you don't think that's even necessary. But coming back with fresh(ish) eyes it looks like maybe we aren't sufficiently testing the change you made in #42 where we delegate resetting the plugin between rows to the migrate executable.

What do you think?

I've updated the change record to reflect the new code.

As far as tests, I've added a ->shouldBeCalled() to the resetStop calls in the existing tests. But I don't think we need to do anything further for testing. The existing executable tests prove that resetStop() is called (now). They prove that the result of isPipelineStopped correctly determines whether to continue processing or not. They also prove that process plugins that don't implement the new interface will work. The process plugin base tests prove that resetStop correctly affects the return value of isPipelineStopped in our implementation. So we've proven that multiple calls to a stoppable plugin will correctly reset between rows already, and additional tests will just increase test run times and decrease maintainability as there would be no way for the test you propose to fail without one of the existing tests also failing, so I don't believe we should add an additional test. Adding the ->shouldBeCalled() to the resetStop prophecy is a good call, as otherwise we wouldn't be proving that the executable is resetting the stop prior to running a transform. So I think that bit is necessary to make the test coverage complete.

Verified all the tests fail as expected

testStopPipeline

Expected :'first_plugin'
Actual   :'final_plugin'

testContinuePipeline

Some predictions failed:
Double\MigrateProcessInterface\StopPipelineInterface\P1:
  No calls have been made that match:
      Double\MigrateProcessInterface\StopPipelineInterface\P1->resetStop()
    but expected at least one.

testMultipleTransforms

Drupal\migrate\MigrateSkipProcessException

CR makes sense to me. Feel good marking this.

Thanks, @mikelutz and @smustgrave. I apologize for not responding after @mikelutz's very persuasive comment in #52. I give a +1 for the RTBC.

I got the sense that @benjifisher wanted to review this too. I'll ping him in the #migrate channel and alert him that it's back to RTBC. Cheers!

Benji was looking at #3247718: Allow process plugins to flag a row to be skipped which does a similar thing for MigrateSkipRowException that this issue does for MigrateSkipProcessException, although he hasn't gotten to it, and I'm happy for anybody to review that issue that would like to, it's been stuck for a bit waiting on reviews.

Unrelated test failure.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Compared the MR generated patch to the original patch, no changes other than a couple line numbers. Restoring RTBC pending passing tests.

> They also prove that process plugins that don't implement the new interface will work.

Are there any of these? Is there not a 1:1 relationship between MigrateProcessInterface and ProcessPluginBase?

As per BC policy:

we reserve the ability to add methods to these interfaces in minor releases to support new features

and therefore not sure we need the new interface or checks at all, we can just add to both MigrateProcessInterface and ProcessPluginBase and anyone doing anything non standard (ie. implementing the interface but not using the base class) has to add the method themselves?

And if so, should we make resetStop() a more generic reset() method that is guaranteed to be called before each iteration around the plugin executable loop, in case we need to extend this in the future, or if a plugin wants to reset other internal state?

Everybody saw it, Longwave said I could change the interface!

Seriously, that is my preference, I've just gotten pushback in the past when following the BC promise to the letter when there is an alternative that goes above and beyond, hence the new interface. It does seem like lately we moving away from going above and beyond the BC promise when doing so involves adding extra unnecessary code and interfaces, so I'm all for it.

Change record updated.

MR updated and passing. Ready for review.

I'm almost as happy as @mikelutz that @longwave suggested changing the interface. :)

Changes look good. Throwing back to RTBC.

Committed fed1277 and pushed to 11.x. Thanks!

Also published the change record.

Not eligible for backport as it's a feature request and minor behaviour change.

Now that D10.3.0 is out, I've released version 2.2.0 of Migrate Conditions featuring the stop_on_condition process plugin pasted into #37.

What about callback plugin provided by the core, don't we need to implement logic to allow skipping the process inside callable?

What about callback plugin provided by the core, don't we need to implement logic to allow skipping the process inside callable?

We do not. The primary purpose of the callback function is to call built-in PHP string transformation methods on the pipeline. Any attempt to provide a mechanism to allow methods called from callable to access the pipeline stopping mechanism here would only work if the called methods were aware of it (i.e., custom methods). Since we are talking custom code at this point, the "workaround" is to implement your logic in a process plugin instead of a method to be used with a callable.

There might have been an argument to be made in #3247331: Deprecate MigrateSkipProcessException that with the deprecation of the skip process exception, there is no longer a method for callable invoked methods to skip the process, but given the primary purpose of callable mentioned above along with the "workaround" for custom code, This is a change we are making intentionally.

What should happen here is to add a note about this to the change record at https://www.drupal.org/node/3414511 , stating skipping the remaining process from inside a callable callback is no longer supported as of Drupal 12