Allow process plugins to stop further processing on a pipeline

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.

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 📌 Allow process plugins to flag a row to be skipped Needs review 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.

The last submitted patch, 52: 3245997-52.drupal.Allow-process-plugins-to-stop-further-processing-on-a-pipeline.patch, failed testing. View results →

The last submitted patch, 52: 3245997-52.drupal.Allow-process-plugins-to-stop-further-processing-on-a-pipeline.patch, failed testing. View results →

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 somehow wants to reset other 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.

quietone → closed merge request !5348

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