- Issue created by @mikelutz
- Status changed to Needs review
over 1 year ago 7:40pm 17 August 2023 - last update
over 1 year ago 1 fail - 🇺🇸United States danflanagan8 St. Louis, US
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 →
- Status changed to Needs work
over 1 year ago 7:57pm 17 August 2023 - 🇺🇸United States danflanagan8 St. Louis, US
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.
- Status changed to Needs review
over 1 year ago 1:48pm 18 August 2023 - last update
over 1 year ago 1 fail - 🇺🇸United States danflanagan8 St. Louis, US
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 →
- last update
over 1 year ago 30,036 pass, 2 fail - 🇺🇸United States danflanagan8 St. Louis, US
> 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 →
- 🇺🇸United States danflanagan8 St. Louis, US
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.
- last update
about 1 year ago 30,495 pass - 🇺🇸United States benjifisher Boston area
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.
- 🇺🇸United States danflanagan8 St. Louis, US
@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 passmy_array_of_arrays
as the source. Following this bugfix, we can deduce that an internal row has been skipped in sub_process if the arraydest_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...
- Status changed to Needs work
about 1 year ago 12:04am 11 November 2023 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.)
- Status changed to Needs review
12 months ago 4:50pm 26 December 2023 - 🇺🇸United States benjifisher Boston area
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.
- 🇺🇸United States benjifisher Boston area
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.
- Status changed to RTBC
12 months ago 5:50pm 26 December 2023 - 🇺🇸United States mikelutz Michigan, USA
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.
- last update
12 months ago 1 fail - last update
12 months ago 1 fail - last update
12 months ago 1 fail - 🇳🇿New Zealand quietone
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.
-
longwave →
committed 4ec07d68 on 11.x
Issue #3365895 by danflanagan8, benjifisher, mikelutz: When sub_process...
-
longwave →
committed 4ec07d68 on 11.x
- Status changed to Fixed
10 months ago 3:46pm 17 February 2024 Automatically closed - issue fixed for 2 weeks with no activity.