Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Now I figured out that if I set $this->currentRow to NULL, that means stop the iteration, because valid() does a !is_null($this->currentRow).
So, definitely we do not want to set it to NULL, because the iteration end up. Removing the break is definitely right because there is no iteration there.
The only problem is that if I return FALSE on a migration class prepareRow() method, the row is anyway processed.
Not really sure about what to do.
Is this expected behaviour? (process row on FALSE return by a prepareRow() method)
Maybe this is another issue, if you think so, I will open another issue, but the break remove definitely belongs here.
I see the problem now, my bad on my commit... What I did was copy the local logic from oracle.inc, which is testable for me (I don't have any MS SQL Server environment available) - but missed the larger picture, which is the Oracle next() contains a loop that continues until it finds a valid row. The MSSQL next() is counting on the Iterator class to do the looping, and as you saw there isn't really a way to tell it to skip a row, the looping logic has to come over from the Oracle implementation as well. Basically,
I found one logic bug: The while is validating the condition $this->currentRow, but that value can be FALSE when we reach the end of the batch, and still we have values. The real problem is that the mssql_fetch_batch call is not near all mssql_fetch_object calls. This patch do so.
For a quick review(and reproduce the bug), try importing something with a batch limit less than the size of the whole import, so you need more than one mssql_fetch_batch.
PS: BTW if you use git am < some.patch you can give real vcs attribution ;-)
I don't have time tonight to do it myself, but I'd like to simplify/consolidate a bit (I hate to see the same snippet of code twice!). I'd like to see that whole sequence (fetch_object/fetch_batch/fetch_object) in a getNextObject() method - this would also bring the structure closer to oracle.inc, the more similar they are the easier it will be to apply common changes across all the sources in the future.
OK, committed your fix, plus additional work to bring the Oracle and SQL Server plugins in line with each other. First I used 'git am', worked nicely to give you authorship on D7 (I backported the whole batch to D6 as one). Hopefully we can put this one to rest now:-).
Comments
Comment #1
mikeryanStrangely, it was half there in the D6 version. Committed for both D6 and D7, thanks!
Comment #3
marvil07 CreditAttribution: marvil07 commentedNow that I am using dev version again I notice that the commit was different than my patch here.
Please remove the "break;" line, we do not have a iteration there.
Comment #4
marvil07 CreditAttribution: marvil07 commentedActually, I think you wanted to add a
Instead of the break.
I think that is needed to avoid processing of the row.
So, new patch :-)
Comment #5
marvil07 CreditAttribution: marvil07 commentedSorry, actually both last two patches are necessary.
Comment #6
marvil07 CreditAttribution: marvil07 commentedI am confused.
Now I figured out that if I set
$this->currentRow
to NULL, that means stop the iteration, becausevalid()
does a!is_null($this->currentRow)
.So, definitely we do not want to set it to NULL, because the iteration end up. Removing the break is definitely right because there is no iteration there.
The only problem is that if I return
FALSE
on a migration classprepareRow()
method, the row is anyway processed.Not really sure about what to do.
Is this expected behaviour? (process row on FALSE return by a prepareRow() method)
Maybe this is another issue, if you think so, I will open another issue, but the break remove definitely belongs here.
Comment #7
mikeryanI see the problem now, my bad on my commit... What I did was copy the local logic from oracle.inc, which is testable for me (I don't have any MS SQL Server environment available) - but missed the larger picture, which is the Oracle next() contains a loop that continues until it finds a valid row. The MSSQL next() is counting on the Iterator class to do the looping, and as you saw there isn't really a way to tell it to skip a row, the looping logic has to come over from the Oracle implementation as well. Basically,
should become
with the rest of the logic inside the while loop. I'd take a blind shot at it, but it would best be done by someone who can actually test it:-).
Thanks.
Comment #8
mikeryanActually, today I do have a SQL Server, and need to make prepareRow work, so I'll get this done myself...
Thanks.
Comment #9
mikeryanCommitted - @marvil07, please re-open if you find a problem.
Thanks!
Comment #10
marvil07 CreditAttribution: marvil07 commentedThanks for the commit :-)
I found one logic bug: The
while
is validating the condition$this->currentRow
, but that value can be FALSE when we reach the end of the batch, and still we have values. The real problem is that themssql_fetch_batch
call is not near allmssql_fetch_object
calls. This patch do so.For a quick review(and reproduce the bug), try importing something with a batch limit less than the size of the whole import, so you need more than one
mssql_fetch_batch
.PS: BTW if you use
git am < some.patch
you can give real vcs attribution ;-)Comment #11
mikeryanI don't have time tonight to do it myself, but I'd like to simplify/consolidate a bit (I hate to see the same snippet of code twice!). I'd like to see that whole sequence (fetch_object/fetch_batch/fetch_object) in a getNextObject() method - this would also bring the structure closer to oracle.inc, the more similar they are the easier it will be to apply common changes across all the sources in the future.
Thanks!
Comment #12
mikeryanOK, committed your fix, plus additional work to bring the Oracle and SQL Server plugins in line with each other. First I used 'git am', worked nicely to give you authorship on D7 (I backported the whole batch to D6 as one). Hopefully we can put this one to rest now:-).
Comment #13
parallax CreditAttribution: parallax commentedI need to import from a MSSQL DB too. I am about to install ODBC driver on debian squeeze. May I see your Class?
Thank you!
:)