prepareRow() is really useful :-)

CommentFileSizeAuthor
#10 0001-Issue-1195802-follow-up-by-marvil07-Added-mssql_fetc.patch2.71 KBmarvil07
#4 0001-Issue-1195802-follow-up-by-marvil07-Set-current-row-.patch686 bytesmarvil07
#3 0001-Issue-1195802-follow-up-by-marvil07-Remove-unnecessa.patch703 bytesmarvil07
0001-Add-prepareRow-call-on-MigrateSourceMSSQL-next.patch1.14 KBmarvil07

Comments

mikeryan’s picture

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

Strangely, it was half there in the D6 version. Committed for both D6 and D7, thanks!

Status: Fixed » Closed (fixed)

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

marvil07’s picture

marvil07 commented
Status: Closed (fixed) » Needs review
StatusFileSize
new 0001-Issue-1195802-follow-up-by-marvil07-Remove-unnecessa.patch703 bytes

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

marvil07’s picture

marvil07 commented
StatusFileSize
new 0001-Issue-1195802-follow-up-by-marvil07-Set-current-row-.patch686 bytes

Actually, I think you wanted to add a

 $this->currentRow = NULL;

Instead of the break.

I think that is needed to avoid processing of the row.

So, new patch :-)

marvil07’s picture

marvil07 commented

Sorry, actually both last two patches are necessary.

marvil07’s picture

marvil07 commented
Status: Needs review » Needs work

I am confused.

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.

mikeryan’s picture

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

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, 

$this->currentRow = mssql_fetch_object($this->result);

should become

while ($this->currentRow = mssql_fetch_object($this->result)) {

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.

mikeryan’s picture

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

Actually, today I do have a SQL Server, and need to make prepareRow work, so I'll get this done myself...

Thanks.

mikeryan’s picture

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

Committed - @marvil07, please re-open if you find a problem.

Thanks!

marvil07’s picture

marvil07 commented
Status: Fixed » Needs review
StatusFileSize
new 0001-Issue-1195802-follow-up-by-marvil07-Added-mssql_fetc.patch2.71 KB

Thanks 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 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 ;-)

mikeryan’s picture

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

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.

Thanks!

mikeryan’s picture

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

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

parallax’s picture

parallax commented

I 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!
:)

Status: Fixed » Closed (fixed)

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