Change to $db Move(0)/MoveNext() behavior breaks plugins

[lat9]

In Zen Cart 1.5.5, someone noticed that when trying to reset a query (using $the_query->Move (0)) didn't work, so they "fixed" the behavior.

Unfortunately, that fix now breaks existing plugins (a couple of mine, that I'll need to hunt up, and others) which have "known" that to reset a query you had to:

$the_query->Move (0);
$the_query->MoveNext ();

to accomplish that feat. Now, any plugin that requires a query-reset is going to have to check the Zen Cart version to see whether that MoveNext () call is required.

I put together a teeny script to illustrate the issue:

<?php
include ('includes/application_top.php');
$list = $db->Execute ("SELECT * FROM " . TABLE_ORDERS_STATUS . " ORDER BY orders_status_id ASC");
echo "First pass iteration:<br />";
while (!$list->EOF) {
    echo sprintf ("Orders status id = %u, name = %s<br />", $list->fields['orders_status_id'], $list->fields['orders_status_name']);
    $list->MoveNext ();
}
echo "<br />Second pass iteration:<br />";
$list->Move (0);
$list->MoveNext ();
while (!$list->EOF) {
    echo sprintf ("Orders status id = %u, name = %s<br />", $list->fields['orders_status_id'], $list->fields['orders_status_name']);
    $list->MoveNext ();
}

and then ran that script on a "fresh" Zen Cart 1.5.4 installation, yielding:

First pass iteration:
Orders status id = 1, name = Pending
Orders status id = 2, name = Processing
Orders status id = 3, name = Shipped
Orders status id = 4, name = Update

Second pass iteration:
Orders status id = 1, name = Pending
Orders status id = 2, name = Processing
Orders status id = 3, name = Shipped
Orders status id = 4, name = Update

That same script, run on a "fresh" Zen Cart 1.5.5 (3-29) installation yields:

First pass iteration:
Orders status id = 1, name = Pending
Orders status id = 2, name = Processing
Orders status id = 3, name = Delivered
Orders status id = 4, name = Update

Second pass iteration:
Orders status id = 2, name = Processing
Orders status id = 3, name = Delivered
Orders status id = 4, name = Update

See how the 2nd-pass output is missing the first orders-status record? That's what's going to happen to all those plugins that were just "doing the right thing" when they're run without change on Zen Cart 1.5.5.

Please note that I'm not asking for the behavior to be modified, since there are now Zen Cart 1.5.5 installations with this functionality and it will just further compound the issue if the 1.5.5 behavior is changed mid-stream. I'm posting to let other plugin authors know that they're going to need to check their plugins, too.

[Ajeh]

Have you tried using:

$blah->rewind();

[DrByte]

You're right. But the real bug is that the Move(0) should never have needed to be followed by MoveNext().
I think my advice was always to never rely on using Move(0) because I knew this bug existed but hadn't been fixed.
Regardless, you're right: plugins that relied on using Move(0) will have unexpected results.

The better way, moving forward, is to use the Iterator approach instead of the old "while(!EOF) { // work; MoveNext() }" approach.

OLD WAY:

$result = $db->Execute($sql);
while (!$result->EOF) { 
  $data = $result->fields['filename'];
  //foo
  $result->MoveNext();
}

NEW WAY:

$result = $db->Execute($sql);
foreach ($result as $row) {
  $bar = $row['fieldname'];
}
// To reset and loop again, use:
reset($result);

Note that with the foreach() approach, MoveNext() is dropped altogether.
And reset() is used instead of Move(0) (although Move(0) will indeed do the same as reset() now)

I suppose for compatibility, you could try testing whether method_exists($result, 'rewind') to determine if your plugin needs to add a MoveNext() after the old Move(0).

[lat9]

Have you tried using:

$blah->rewind();

Nope, that function was added in Zen Cart 1.5.5.

[Ajeh]

Sorry since this was marked v155 I thought you were looking for a v155 method ...

DrByte's method works any which way ...

[lat9]

You're right. But the real bug is that the Move(0) should never have needed to be followed by MoveNext().
I think my advice was always to never rely on using Move(0) because I knew this bug existed but hadn't been fixed.
Regardless, you're right: plugins that relied on using Move(0) will have unexpected results.

The better way, moving forward, is to use the Iterator approach instead of the old "while(!EOF) { // work; MoveNext() }" approach.

OLD WAY:

$result = $db->Execute($sql);
while (!$result->EOF) { 
  $data = $result->fields['filename'];
  //foo
  $result->MoveNext();
}

NEW WAY:

$result = $db->Execute($sql);
foreach ($result as $row) {
  $bar = $row['fieldname'];
}
// To reset and loop again, use:
reset($result);

Note that with the foreach() approach, MoveNext() is dropped altogether.
And reset() is used instead of Move(0) (although Move(0) will indeed do the same as reset() now)

I suppose for compatibility, you could try testing whether method_exists($result, 'rewind') to determine if your plugin needs to add a MoveNext() after the old Move(0).

I'm thinking that I'll just check for earlier than Zen Cart v1.5.5 to issue the additional MoveNext ()

[rbarbour]

Maybe I'm a little thick here,

I'm creating new and (updating) old plugins for 1.5.5 and the "NEW WAY" isn't working for $db->ExecuteRandomMulti($sql), when a constant ", MAX_DISPLAY" is set.

What am I missing?

You're right. But the real bug is that the Move(0) should never have needed to be followed by MoveNext().
I think my advice was always to never rely on using Move(0) because I knew this bug existed but hadn't been fixed.
Regardless, you're right: plugins that relied on using Move(0) will have unexpected results.

The better way, moving forward, is to use the Iterator approach instead of the old "while(!EOF) { // work; MoveNext() }" approach.

OLD WAY:

$result = $db->Execute($sql);
while (!$result->EOF) { 
  $data = $result->fields['filename'];
  //foo
  $result->MoveNext();
}

NEW WAY:

$result = $db->Execute($sql);
foreach ($result as $row) {
  $bar = $row['fieldname'];
}
// To reset and loop again, use:
reset($result);

Note that with the foreach() approach, MoveNext() is dropped altogether.
And reset() is used instead of Move(0) (although Move(0) will indeed do the same as reset() now)

I suppose for compatibility, you could try testing whether method_exists($result, 'rewind') to determine if your plugin needs to add a MoveNext() after the old Move(0).

[mc12345678]

Maybe I'm a little thick here,

I'm creating new and (updating) old plugins for 1.5.5 and the "NEW WAY" isn't working for $db->ExecuteRandomMulti($sql), when a constant ", MAX_DISPLAY" is set.

What am I missing?

Haven't tested this condition, but by code review, MAX_DISPLAY doesn't appear to be an existing ZC 1.5.5 constant, does it work if a number is briefly put in place of the constant? If so, then it would appear that the constant is not defined prior to the execution of ExecuteRandomMulti sql.

Does the same problem occur if instead of ExecuteRandomMulti you try a standard Execute? (Ie. basically trying both types of execution to validate that the "new method" (Assume referring to use of foreach on an array instead of while(!$obj->EOF) type loop) works).

[rbarbour]

"MAX_DISPLAY" is not existing, was an example per se.

I guess better examples would be the center-box "loops".

$db->ExecuteRandomMulti($specials_index_query, MAX_DISPLAY_SPECIAL_PRODUCTS_INDEX);
$db->ExecuteRandomMulti($new_products_query, MAX_DISPLAY_NEW_PRODUCTS);
$db->ExecuteRandomMulti($featured_products_query, MAX_DISPLAY_SEARCH_RESULTS_FEATURED);
$db->ExecuteRandomMulti(sprintf(SQL_ALSO_PURCHASED, (int)$_GET['products_id'], (int)$_GET['products_id']), MAX_DISPLAY_ALSO_PURCHASED); 

I'm mainly updating plugins for 1.5.5 but using the "NEW" way doesn't respect the constant. Yes the standard execute works but you no longer get random products on page reload or revisit.

Haven't tested this condition, but by code review, MAX_DISPLAY doesn't appear to be an existing ZC 1.5.5 constant, does it work if a number is briefly put in place of the constant? If so, then it would appear that the constant is not defined prior to the execution of ExecuteRandomMulti sql.

Does the same problem occur if instead of ExecuteRandomMulti you try a standard Execute? (Ie. basically trying both types of execution to validate that the "new method" (Assume referring to use of foreach on an array instead of while(!$obj->EOF) type loop) works).

[lat9]

... I'm mainly updating plugins for 1.5.5 but using the "NEW" way doesn't respect the constant ...

Just a note to readers of this thread: Using the "new" way will only work for plugins that support only Zen Cart 1.5.5 or later since prior Zen Cart versions don't know the "new" way!