$db, SELECT returning numeric indexes

[lat9]

Zen Cart 1.5.5e. Running queries and I noticed some "funky" output conditions.

Take the following sample (I place the file in the root of my test site and run it directly):

<?php
require 'includes/application_top.php';

$check = $db->Execute(
    "SELECT * FROM " . TABLE_COUNTRIES . " WHERE countries_name LIKE 'a%' LIMIT 2"
);
foreach ($check as $key => $value) {
    if (gettype($value) == 'array') {
        echo "$key => (Array)<br />";
        foreach ($value as $k2 => $v2) {
            echo "$k2 => $v2<br />";
        }
    } else {
        echo "$key => $value<br />";
    }
    echo '<br />';
}

require DIR_WS_INCLUDES . 'application_bottom.php';

The output of that script contains numerically-indexed and non-updating additional values:

0 => (Array)
countries_id => 1
countries_name => Afghanistan
countries_iso_code_2 => AF
countries_iso_code_3 => AFG
address_format_id => 1
status => 1
0 => 1
1 => Afghanistan
2 => AF
3 => AFG
4 => 1
5 => 1

1 => (Array)
countries_id => 240
countries_name => Åland Islands
countries_iso_code_2 => AX
countries_iso_code_3 => ALA
address_format_id => 1
status => 1
0 => 1
1 => Afghanistan
2 => AF
3 => AFG
4 => 1
5 => 1

I believe that the numerical-indexes should not be part of the output.

[mc12345678]

That would likely be because of the highlighted code found in the Move method of the queryFactoryResult class located in includes/classes/db/mysql/query_factory.php (with similar issue in the ExecuteRandomMulti method):

/**
   * Moves the cursor to the specified row. If the row is not valid,
   * the cursor will be moved past the last row and EOF will be set false.
   *
   * @param int $zp_row the row to move to
   */
  public function Move($zp_row) {
    global $db;
    if ($this->is_cached) {
      if($zp_row >= sizeof($this->result)) {
        $this->cursor = sizeof($this->result);
        $this->EOF = true;
      } else {
        while(list($key, $value) = each($this->result[$zp_row])) {
          $this->fields[$key] = $value;
        }
        $this->cursor = $zp_row;
        $this->EOF = false;
      }
    } else if (@mysqli_data_seek($this->resource, $zp_row)) {
      $zp_result_array = @mysqli_fetch_array($this->resource);
      while (list($key, $value) = each($zp_result_array)) {
        $this->fields[$key] = $value;
      }
      $this->cursor = $zp_row;
      $this->EOF = false;
    } else {
      $this->EOF = true;
      $db->set_error(mysqli_errno($this->link), mysqli_error($this->link), $db->dieOnErrors);
    }
  }

As identified in the PHP Manual for the mysqli_fetch_array function, When performing a mysqli_fetch_array with no further resulttype identifier, then both the numeric as well as associative result(s) are provided. In cases/uses outside of the Move method and ExecuteRandomMulti method, the numeric index is prevented from being added to the result, which also excludes the possibility of a field being identified as a number (which is possible with mysql if the field, when used/referenced, is backquoted (`` not '').

Therefore, if it is in fact considered that no field/index should be numeric only then the following changes would be expected with line numbers provided from the github tracked ZC 1.5.5f version (centrally, the issue affects all of ZC 1.5.X (at least up to 1.5.5f and then 1.5.6) as well 1.6.0).

The reason that this is now so "visible" is because the queryFactoryResult class implements the Iterator class to support the foreach operation and centrally its use of the Move(0) call within the Rewind method which is called as part of the Iterator class.

Beginning at line 711 changing:

    } else if (@mysqli_data_seek($this->resource, $zp_row)) {
      $zp_result_array = @mysqli_fetch_array($this->resource);
      while (list($key, $value) = each($zp_result_array)) {
        $this->fields[$key] = $value;
      }

to:

    } else if (@mysqli_data_seek($this->resource, $zp_row)) {
      $zp_result_array = @mysqli_fetch_array($this->resource);
      while (list($key, $value) = each($zp_result_array)) {
        if (!preg_match('/^[0-9]/', $key)) { // mc12345678 prevent numeric index result from being stored.
          $this->fields[$key] = $value;
        }
      }

and beginning at line 327 changing:

          $zp_result_array = @mysqli_fetch_array($zp_db_resource);
          if ($zp_result_array) {
            $obj->result[$zp_ii] = array();
            while (list($key, $value) = each($zp_result_array)) {
              $obj->result[$zp_ii][$key] = $value;
            }
          } else {

to:

          $zp_result_array = @mysqli_fetch_array($zp_db_resource);
          if ($zp_result_array) {
            $obj->result[$zp_ii] = array();
            while (list($key, $value) = each($zp_result_array)) {
              if (!preg_match('/^[0-9]/', $key)) { // mc12345678 prevent numeric index result from being stored for use.
                $obj->result[$zp_ii][$key] = $value;
              }
            }
          } else {

[carlwhat]

wow! real curious bug. and good resolution of problem, mc.

however, as a minor point, i would take issue with your comment:

// mc12345678 prevent numeric index result from being stored for use.

those numeric index results are clearly wrong. if someone wanted to use the numeric index result, this class would have to be fixed. this is clearly a bug, as the values associated with the numeric index never get incremented and stay with the first record part of the selection.

in addition, the class needs to be updated for use in php7.2. (each is deprecated...)

and are we not moving to laravel framework for zc 1.6? which would hopefully make the db query_factory class obsolete except for legacy plugins and customizations.

[mc12345678]

So the lack of increment is because of the implementation. The MoveNext method does not support the numeric index and is the method called to iterate through the results.

For "legacy" applications that desire the numeric index (which technically could replace a field's result if it was numeric and not properly positioned in the list of fields to be returned) I would recommend that the Move method have an additional parameter added that would default to support returning the numeric index but implementing the iterator class such that the numeric index is not returned... OR for users of the foreach implementation to not then "blindly" iterate over all results of the results and to instead recognize that the foreach be used as it has thus far been described of using/referencing the field(s) known or expected to be returned and as necessary to update/modify the associated query_factory.php file to support whichever method is to be employed. Not a small task in either way though it should be noted that ZC default tables do not have numeric only fields and therefore supporting numeric fields in such a customized store would not impact the ability of ZC to operate, though it would continue to double each foreach loop as it currently is doing.

Just some additional food for thought.

[lat9]

As @carlwhat indicated, nice solution @mc12345678; are you going to submit a PR?

[mc12345678]

While writing the below saw the question about submitting a PR. Plan to to include the below for those that have intentionally used the integer index as provided by the Move method AND to support the new foreach iterator to provide just the fields results that one would expect in a replacement of the old while(!EOF) MoveNext loop.

To implement the switch supporting the continued use/receipt of the numeric index at least for the Move($zp_row) method, could see the following changes to what I previously provided:

 public function Move($zp_row, $int_key = true) {

 if ($int_key === true ? true : !preg_match('/^[0-9]/', $key)) {

Or:

 $add_val = ($int_key === true ? true : !preg_match('/^[0-9]/', $key));
 if ($add_val) {

Then in the method rewind of the queryFactoryResult class change:

$this->Move(0);

To:

$this->Move(0, false);

And that would adjust the foreach to only return field=>val type results when combined with the above and would still permit the use of results from the move method where the result includes a numeric key.

Regarding the deprecation statement of PHP 7.2, yes the use of list(x, y) = each(data) is deprecated, to be modified to the likes of foreach (data as x => y) {, doesn't apply to ZC 1.5.5x (as max php version identified as applicable is 7.1.x), and is in the process of being changed in ZC 1.5.6 in some form of "step" process. That said, the issue of results from executing the move() method exists in all stated versions providing a numeric key for each field and could be adjusted for use using the above code (though won't find the rewind method implemented until ZC 1.5.5).

Why the issue with the move method has been beat around the bush for so long is beyond me, but it has been brought up a number of times as not supporting the desired results.

[mc12345678]

Submitted pull requests for this to cover:
ZC 1.5.6: https://github.com/zencart/zencart/pull/1633
ZC 1.6.0: https://github.com/zencart/zencart/pull/1634
And 1.5.5: https://github.com/zencart/zencart/pull/1635

The pull for 1.5.5 may be rejected (based on previous response to pull requests to 1.5.5) as currently it is reported that pull requests for 1.5.5 are being supported for bugs only. Unfortunately as the condition provides some potentially odd behavior, from my perspective it doesn't appear that a *true* problem has been identified as in what has been affected by this occurrence. Not that I've looked at all of the code to see where the use case provided in the OP exists (or would exist when one of the while loops was changed to a foreach loop).

[lat9]

Submitted pull requests for this to cover:
ZC 1.5.6: https://github.com/zencart/zencart/pull/1633
ZC 1.6.0: https://github.com/zencart/zencart/pull/1634
And 1.5.5: https://github.com/zencart/zencart/pull/1635

The pull for 1.5.5 may be rejected (based on previous response to pull requests to 1.5.5) as currently it is reported that pull requests for 1.5.5 are being supported for bugs only. Unfortunately as the condition provides some potentially odd behavior, from my perspective it doesn't appear that a *true* problem has been identified as in what has been affected by this occurrence. Not that I've looked at all of the code to see where the use case provided in the OP exists (or would exist when one of the while loops was changed to a foreach loop).

Thanks, @mc12345678! It's best to squish this before the foreach approach becomes widely adopted; I wouldn't want a repeat of the double ->Move(0) in the current implementation.

[mc12345678]

Thanks, @mc12345678! It's best to squish this before the foreach approach becomes widely adopted; I wouldn't want a repeat of the double ->Move(0) in the current implementation.

I'm totally onboard and actually wish that the "extended" fix had been implemented in earlier versions so that didn't/don't have to deal with the result of that static integer array portion.

[carlwhat]

The pull for 1.5.5 may be rejected (based on previous response to pull requests to 1.5.5) as currently it is reported that pull requests for 1.5.5 are being supported for bugs only. Unfortunately as the condition provides some potentially odd behavior, from my perspective it doesn't appear that a *true* problem has been identified as in what has been affected by this occurrence.

#1 - good job!
#2 - i completely disagree with the above comment. it is a bug. period. the array is wrong.

it looks like your PR removes the numeric index to the array. now someone could take issue with that approach, ie, lets correct the numeric index part of the array as opposed to removing it completely. but i would completely disagree with anyone stating this is not a bug.

best.