Add mysqli_fetch_all to core?

[zipurman]

Not sure if you'd want to add this to the core or not? I am using it in my code and am not sure why the core is using single sql calls and while (tep_db_fetch_array) which seams to be extra overhead?

Add the following to core?

function tep_db_fetch_all( $db_query ) { return mysqli_fetch_all( $db_query, MYSQLI_ASSOC ); }

[gburton]

I see no reason why not...

[ecartz]

It would create a hard dependency on mysqlnd.

Also, for large results, mysqli_fetch_all uses more memory than mysqli_fetch_assoc, as it has to load all the results into memory before processing them. In particular, it doesn't combine well with using wildcard columns.

[zipurman]

Agree on both. It would be handy in many cases for smaller data sets, what about something like this as an option to clean up code on short lists etc.

function tep_db_fetch_all( $db_query ) {

    $results = [];
    while ( $result = tep_db_fetch_array($db_query) ) {
        $results[] = $result;
    }
    return $results;
}

I realize that this still creates a large memory heap if loading large datasets, but it gets around the issue of requiring mysqlnd.

[ecartz]

It's also worth noting that if you are sure of having mysqlnd, you don't need a separate function. The following should work:

$all_results = $db_query->fetch_all(MYSQLI_ASSOC);

I'm considering the possibility of

    public function tep_db_fetch_all($db_query) {
      if (!($db_query instanceof mysqli_result) && is_string($db_query)) {
        $db_query = tep_db_query($db_query);
      }

      if (method_exists($db_query, 'fetch_all')) {
        return $db_query->fetch_all(MYSQLI_ASSOC);
      }

      $results = [];
      while ($result = $db_query->fetch_assoc()) {
        $results[] = $result;
      }

      return $results;
    }

Which will use mysqlnd if available. If not, it falls back to a loop.

One reason why I am reluctant to make a change like this is that I don't really like the current approach to database handling. In general, I would prefer to move from procedural functions to object-oriented methods. And I think that the current approach of writing a new function for every mysqli_ function that we want to use is rather silly. How does

  function tep_db_free_result($db_query) {
    return mysqli_free_result($db_query);
  }

help anyone? After all, they could just $db_query->free_result() if they felt the need.

You can see more of the possible future direction at https://github.com/ecartz/CE-Phoenix/commit/aa05a2256a89448ba90ea3d0ecffa5d52e490ac9

[zipurman]

I like the idea of your tep_db_fetch_all that would work in both cases.

I also like the idea of moving to OOP PDO. Of course this would break all kinds of things but what if you created a second database class that the core started to adopt using OOP while leaving the old one in place for existing mods to use until they can be upgraded? That would also allow the core to be migrated to OOP over time.

I use OOP PDO on all my projects and am very familiar with it. I also use PHPStorm as my IDE which allows for quick code changes ... I'd be happy to help in that area if you wanted some help. I also usually create PHP Unit Testing for my projects to spot coding issues before releasing new code ... not sure if your interested in going that route?

[ecartz]

If you have unit tests (although wouldn't they have to be integration tests?) for the functions in includes/functions/database.php, I would be interested in seeing them.

[zipurman]

I usually create a test database with specific info in it, like a few orders, taxes, tax codes, shipping zones, etc. Then the unittests load a custom db class to connect to the test database and then run all code against it. That way if a coding change breaks something, the unit tests pick it up right away. I do this for my large projects of 300+ database tables and it works great ... couldn't code confidently without it. It would take some time, but I could do it if you were interested? Then I'd just distribute the test database to anyone coding the core project and once it was loaded into a localhost on your dev server, the unittests would be able to run. Then any changes to that database going forward would have patches so that the dev team stays current with the test db.

Another way of doing it is for the test class to load arrays of data as if they came from the db. That would be easier, but it's not a true test of all of the code.

Thoughts?

[ecartz]

https://github.com/CE-PhoenixCart/PhoenixCart/commit/02b21e898b80fb700e10056a77a954a9275dd11e

[ecartz]

This was released with 1.0.8.1. Closing.