When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row

This one caught my eye. I thought I'd try to cook up a test. Here's a Kernel test that exposes the bug. I don't think there's a reasonable way to do this with a unit test especially if we're to be agnostic as to whether the related issue is fixed or not. This covers skipping a process within a sub_process too, which I think is a reasonable case to add coverage. (Spoiler: it passes!)

This patch attempts to run only the new fail test, but its the first time I've tried this since #3313833: Make it easy to run only tests for one specific core module → so I might mess up.

The last submitted patch, 2: 3365895-2-FAIL.patch, failed testing. View results →

Sweet:

1) Drupal\Tests\migrate\Kernel\process\SubProcessWithSkipTest::testSubProcessSkip with data set "skip row" ('row', array('something outside of sub_process', array(array('bar', 'bar'))))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 'first' => 'something outside of sub_process'
- 'second' => Array (...)
)

So that failed as expected. Putting a skip row inside a sub_process indeed skips the outer row.

We chatted about this at the migrate meeting 📌 [meeting] Migrate Meeting 2023-08-17 2100Z Needs work

@benjifisher rightly pointed out that the various array key names and values were pretty confusing. Here's an updated patch that tries to make things easier to follow. If this fails correctly I'll follow with a patch that reverts the changes in drupalci.yml.

The last submitted patch, 5: 3365895-5-FAIL.patch, failed testing. View results →

> I'll follow with a patch that reverts the changes in drupalci.yml.

I'm actually going to post a patch that has a fix included as well. The interdiff just shows the fix and does not include the changes to drupalci.yml because who wants to see that?

The last submitted patch, 7: 3365895-7.patch, failed testing. View results →

I finally had a chance to take a look at those failures and after way too long figured out I had neglected to remove the line I wanted to replace the the try/catch. Oops. Let's see if tests are happier with this one.

We discussed this issue during 📌 [meeting] Migrate Meeting 2023-11-09 2100Z Active : specifically the question of whether we need to provide backwards compatibility (BC) for the current behavior.

We agreed that we do not have to provide BC if we can suggest a work-around and document it in a change record. I am adding the issue tag for that.

@benjifisher and I came up with a drop in bc skip in slack.

Assume that the sub_process is used to set the destination value dest_1 and that we pass my_array_of_arrays as the source. Following this bugfix, we can deduce that an internal row has been skipped in sub_process if the array dest_1<code> has fewer elements that the source array <code>my_array_of_arrays. Using the contrib Migrate Conditions → , we can skip if that condition is met.

dest_1:
  plugin: sub_process
  source: my_array_of_arrays
  process:
    property:
      plugin: something_that_might_skip
      source: array_key

_bc_skip:
  plugin: skip_on_condition
  method: row
  message: 'BC forever! See #3365895'
  source: my_array_of_arrays
  condition:
    plugin: greater_than
    property: '@dest_1'

This relies on php using array length when considering if one array is greater than another. Migrate Conditions has some test cases that demonstrates this behavior: https://git.drupalcode.org/project/migrate_conditions/-/blob/1.0.x/tests...

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.)

I created MR 5954 from the patch in #9. I hid the earlier patches except for the test-only patch in #5, and I did not hide the interdiffs. (I decided not to spend time adding them to the MR as individual commits.)

I do not like the policy of having the bot change the status from NR to NW. It is too easy to lose track of issues that are actually close to RTBC, like this one. I only noticed this issue because we discussed it at one of the recent migration meetings. I mentioned this issue in the #needs-review-queue-initiative Slack channel.

Back to NR.

I added the change record (CR) The sub_process process plugin catches MigrateSkipRowException exceptions → based on the discussion in 📌 [meeting] Migrate Meeting 2023-11-09 2100Z Active . I am removing the issue tag for a CR.

CR looks good, new test looks good. The "fix" is very straightforward, most of the discussion on this issue is around whether to define this as a bug, and I'm confident that it is. The CR provides options for those that wish to replicate the buggy behavior. I'm marking this RTBC.

I'm triaging RTBC issues → . I read the IS, the comments, the MR and change record. And it is a very complete change record as well! I didn't find any unanswered questions or other work to do.

I downloaded the diff from the MR and that latest patch so I could compare them. The only differences where indexes.So, the MR is correct.

I updated the proposed resolution and added links to the meeting where BC was discussed.

Leaving at RTBC.

The change makes sense, but this is only eligible for commit to 11.x (10.3.x) as it is a minor behaviour change and we don't want to surprise anyone in a patch release.

Committed 4ec07d6 and pushed to 11.x. Thanks!

Also published the change record.

longwave → closed merge request !5954

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