-
Bug
-
Resolution: Fixed
-
Minor
-
3.2, 3.3
-
MOODLE_32_STABLE, MOODLE_33_STABLE
-
MOODLE_32_STABLE, MOODLE_33_STABLE
-
MDL-59173-master -
-
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.