Problem/Motivation

There are some calls to methods on the idMap through out the sourceBase but it isn't initialized until the rewind() method is called. This generally works because those methods are called with in a loop so rewind() has been called but there isn't a requirement on all of those and it will cause a fatal if that happens.

I left this normal. It would be minor in my opinion but the fatal is a pretty big consequence of the edge case.

Proposed resolution

Initialize idMap in constructor.

CommentFileSizeAuthor
#10 2591533-10.interdiff.txt4.52 KBneclimdul
#10 idmap_can_be_called-2591533-10.patch9.16 KBneclimdul
#7 idmap_can_be_called-2591533-7.patch9.21 KBneclimdul
#7 idmap_can_be_called-2591533-7-FAILURE.patch7.51 KBneclimdul
#2 idmap_can_be_called-2591533-2.patch1.18 KBneclimdul

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new idmap_can_be_called-2591533-2.patch1.18 KB
neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review

err... here's a patch.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Cool.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: idmap_can_be_called-2591533-2.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
commented
Issue tags: +Needs tests

stupid bot... I should write a test for this anyways

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new idmap_can_be_called-2591533-7-FAILURE.patch7.51 KB
new idmap_can_be_called-2591533-7.patch9.21 KB

Tests! Lots and lots of tests!

Because these tests run outside of the iterator, they exhibit the behavior and the ones that skip rows should fail without the change.

The last submitted patch, 7: idmap_can_be_called-2591533-7-FAILURE.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented 
+++ b/core/modules/migrate/tests/src/Unit/MigrateSourceTest.php
@@ -222,6 +229,144 @@ public function testNewHighwater() {
+    $module_handler->invokeAll('migrate_prepare_row', array($row, $source, $migration))

New code, let's use the short array syntax here and below.

Yes, lots of tests! Fully testing prepareRow() might be argued as a bit out-of-scope, but it is a significant hole and this is a good excuse to get it plugged.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new idmap_can_be_called-2591533-10.patch9.16 KB
new 2591533-10.interdiff.txt4.52 KB

Bah, certain that got pulled over when I copied the module invoke syntax out of the plugin. Quick fix for that.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

  • webchick committed 5fff18f on 8.0.x 
    Issue #2591533 by neclimdul, mikeryan: idMap can be called before it is...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Testbot seems to be a bit constipated atm, but https://dispatcher.drupalci.org/job/default/34040/testReport/ shows a clean test run.

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +rc eligible

Retroactively tagging, since this impacts the migration system which is experimental.

  • webchick committed 5fff18f on 8.1.x 
    Issue #2591533 by neclimdul, mikeryan: idMap can be called before it is...

Status: Fixed » Closed (fixed)

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