Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-59173

table_sql - Null instead of array() leads to empty params in query_db

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.2, 3.3
    • Fix Version/s: 3.2.4, 3.3.1
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Take the error_table.php from the attachments and place it in the root folder of your moodle installation. Browse it and use the initialfilters.

       

      Depending on your error reporting level you get either the error:

      ERROR: Incorrect number of query parameters. Expected 1, got 0.

      Or:

      Warning: array_merge(): Argument #1 is not an array in /var/www/public/moodlemaster/lib/tablelib.php on line 1578

       

      The error should not happen, after the test is applied!

      Show
      Take the error_table.php from the attachments and place it in the root folder of your moodle installation. Browse it and use the initialfilters.   Depending on your error reporting level you get either the error: ERROR: Incorrect number of query parameters. Expected 1, got 0. Or: Warning: array_merge(): Argument #1 is not an array in /var/www/public/moodlemaster/lib/tablelib.php on line 1578   The error should not happen, after the test is applied!
    • Workaround:
      Hide

      If no $params are needed initialize them in set_sql() with array() and override the default!

      Show
      If no $params are needed initialize them in set_sql() with array() and override the default!
    • Affected Branches:
      MOODLE_32_STABLE, MOODLE_33_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE, MOODLE_33_STABLE
    • Pull Master Branch:
      MDL-59173-master

      Description

      When using the table_sql the usage pattern is usually to first call set_sql(...) to initialize the sql query. Afterwards out(...) can be called to print the table. out(...) calls query_db(), which builds the query and executes it. Thereby, query_db() makes some implications on the values set by set_sql(), especially $params which is by default null.

      $this->sql->params is first stored in $this->countparams in line 1568. Later $this->sql->params and $this->countparams are both used within array_merge(), However, when null is passed to array_merge() the result will be null independent of the value of the other parameter!

      The problem only occurs, if a table extending table_sql uses no params for the set_sql(), but uses the initial filters functionality. Then the $wparams, comming from get_sql_where() in line 1575, may contain params for i_last or i_first. These params are returned as null by the array_merge statements in query_db().

      I came up with to possible solutions to this problem. The first one is changing the default of the parameter $params in set_sql() (see referenced branches). The other one is to add null checks within the query_db such as:

       

      @@ -1575,10 +1576,18 @@ class table_sql extends flexible_table {
                   list($wsql, $wparams) = $this->get_sql_where();
                   if ($wsql) {
                       $this->countsql .= ' AND '.$wsql;
      -                $this->countparams = array_merge($this->countparams, $wparams);
      +                if ($this->countparams === null) {
      +                    $this->countparams = $wparams;
      +                } else {
      +                    $this->countparams = array_merge($this->countparams, $wparams);
      +                }
       
                       $this->sql->where .= ' AND '.$wsql;
      -                $this->sql->params = array_merge($this->sql->params, $wparams);
      +                if ($this->sql->params  === null) {
      +                    $this->sql->params  = $wparams;
      +                } else {
      +                    $this->sql->params @@ -1575,10 +1576,18 @@ class table_sql extends flexible_table {
      list($wsql, $wparams) = $this->get_sql_where();
      if ($wsql) {
      $this->countsql .= ' AND '.$wsql;
      - $this->countparams = array_merge($this->countparams, $wparams);
      + if ($this->countparams === null) {
      + $this->countparams = $wparams;
      + } else {
      + $this->countparams = array_merge($this->countparams, $wparams);
      + }
       
      $this->sql->where .= ' AND '.$wsql;
      - $this->sql->params = array_merge($this->sql->params, $wparams);
      + if ($this->sql->params === null) {
      + $this->sql->params = $wparams;
      + } else {
      + $this->sql->params = array_merge($this->sql->params, $wparams);
      + }
       
      $total = $DB->count_records_sql($this->countsql, $this->countparams);
      } else {
      = array_merge($this->sql->params, $wparams);
      +                }
       
                       $total  = $DB->count_records_sql($this->countsql, $this->countparams);
                   } else {
      

      The second option is more error-tolerant, but adds much more code and therefore complexity, so I currently decided on the first version.

       

        Attachments

          Activity

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Jul/17