I ran into an instance today where prepareRow() was getting called a bunch of times, even after the limit had been reached. The reason this was happening is two-fold. MigrateSource::next() has a while loop that continues until it finds a successful row (prepareRow doesn't return FALSE, etc.). The foreach loop in Migration::import() does it's check at the beginning of the loop to see if the item limit has been reached. Let's walk through what this means if we were to set the limit to 1 item:

  1. Migration::import() calls MigrateSource::next() the first time
  2. next() iterates through until it finds a valid row
  3. We enter the foreach loop, item limit has not been reached so it continues.
  4. The item is successfully migrated
  5. You would expect that the migration would then be done, but what happens instead is that the foreach loop continues, and next() gets called again (since we didn't hit a break).
  6. next() iterates through until it finds another valid row
  7. THEN we break out of the loop because we now see that we've reached the limit

I'll attach a SimpleTest to show what I mean here in a bit.

CommentFileSizeAuthor
#9 migrate-1537352-item-limit-prepare-row-9.patch3.88 KBq0rban
#8 migrate-1537352-item-limit-prepare-row-8.patch5.01 KBq0rban
#7 migrate-1537352-item-limit-prepare-row-7.patch6.35 KBq0rban
#6 migrate-1537352-item-limit-prepare-row-6.patch6.42 KBq0rban
#3 migrate-1537352-item-limit-prepare-row-3.patch9.67 KBq0rban
#2 migrate-1537352-item-limit-prepare-row-tests-only-2.patch7.55 KBq0rban
#1 migrate-1537352-item-limit-prepare-row-tests-only-1.patch7.79 KBq0rban

Comments

q0rban’s picture

q0rban commented
Status: Active » Needs review
StatusFileSize
new migrate-1537352-item-limit-prepare-row-tests-only-1.patch7.79 KB
q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1537352-item-limit-prepare-row-tests-only-2.patch7.55 KB

Updated patch. I didn't realize there were processCount() and importedCount() methods.

Why isn't the test bot recognizing these?

q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1537352-item-limit-prepare-row-3.patch9.67 KB

Ok, and here's the patch with the fix.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Needs work

The fix itself looks good, but a whole test module just to test this one case seems like overkill - would it be possible to build a test on migrate_example's wine.inc (the basis for the existing tests)?

Thanks.

q0rban’s picture

q0rban commented

Sure, I can do that. I saw in the README, however, that there are a bunch of missing tests, so I just assumed we'd need to set up our own test class for these, since it's going to require some things in those tests that you don't want visible in an example module. For example, in this migration, I'm setting up an incrementing variable in the migration in prepareRow(), and then adding a method to return that parameter. I'll work on trying to get an equivalent using the existing example migrations.

q0rban’s picture

q0rban commented
Status: Needs work » Needs review
StatusFileSize
new migrate-1537352-item-limit-prepare-row-6.patch6.42 KB

Ok, here you are! Doesn't seem as clean to me, as it kind of murks up the Migrate Example module.

q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1537352-item-limit-prepare-row-7.patch6.35 KB

Cleaned out the unnecessary __construct(), and updated comments.

q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1537352-item-limit-prepare-row-8.patch5.01 KB

Removing unnecessary logic from prepareRow() and unnecessary changes to beer.install.

q0rban’s picture

q0rban commented
StatusFileSize
new migrate-1537352-item-limit-prepare-row-9.patch3.88 KB

Aha! Just thought of a way of doing this without needing to modify Migrate Example at all. I can use the timers to grab the number of times prepareRow() is called. Sorry for spamming this issue with patches.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Committed to D6 and D7, thanks!

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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