Update ActiveDataProvider::prepareModels() to avoid SQL error with UNION queries

[cepaim]

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Fixed issues	#20239

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 64.93%. Comparing base (34d2396) to head (a1807f9).
Report is 27 commits behind head on master.

Files with missing lines	Patch %	Lines
framework/data/ActiveDataProvider.php	50.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20246      +/-   ##
============================================
- Coverage     64.94%   64.93%   -0.01%     
- Complexity    11390    11394       +4     
============================================
  Files           430      430              
  Lines         36912    36919       +7     
============================================
+ Hits          23972    23975       +3     
- Misses        12940    12944       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rob006]

I don't think this is a correct solution. IMO it should be fixed in QueryBuilder::build():

yii2/framework/db/QueryBuilder.php

Lines 225 to 269 in 34d2396

	public function build($query, $params = [])
	{
	$query = $query->prepare($this);
	$params = empty($params) ? $query->params : array_merge($params, $query->params);
	$clauses = [
	$this->buildSelect($query->select, $params, $query->distinct, $query->selectOption),
	$this->buildFrom($query->from, $params),
	$this->buildJoin($query->join, $params),
	$this->buildWhere($query->where, $params),
	$this->buildGroupBy($query->groupBy),
	$this->buildHaving($query->having, $params),
	];
	$sql = implode($this->separator, array_filter($clauses));
	$sql = $this->buildOrderByAndLimit($sql, $query->orderBy, $query->limit, $query->offset);
	if (!empty($query->orderBy)) {
	foreach ($query->orderBy as $expression) {
	if ($expression instanceof ExpressionInterface) {
	$this->buildExpression($expression, $params);
	}
	}
	}
	if (!empty($query->groupBy)) {
	foreach ($query->groupBy as $expression) {
	if ($expression instanceof ExpressionInterface) {
	$this->buildExpression($expression, $params);
	}
	}
	}
	$union = $this->buildUnion($query->union, $params);
	if ($union !== '') {
	$sql = "($sql){$this->separator}$union";
	}
	$with = $this->buildWithQueries($query->withQueries, $params);
	if ($with !== '') {
	$sql = "$with{$this->separator}$sql";
	}
	return [$sql, $params];
	}

$this->buildOrderByAndLimit() should be called after handing union queries.

[cepaim]

@rob006 You are right!

I don't know Yii2 so deeply, but if we fix this at the query level, it will be fixed for all its uses.

At the moment, my hack is working for me and I am now very busy. I'll try to find time to make another PR.

I think this PR can be rejected and closed.

[rob006]

@cepaim Does it really work for MySQL? Because only SQLite doesn't have parentheses, for all other DBMS generated query will look like (SELECT * FROM a) UNION (SELECT * FROM b LIMIT 10) which also does not look right (it should be (SELECT * FROM a) UNION (SELECT * FROM b) LIMIT 10?).

[cepaim]

@rob006
I haven't tested it on mysql, only on sqlite3.

I'll tell you when I test in on mysql.

[samdark]

Suggested change

	$has_union = $query->union && count($query->union);
	$hasUnion = $query->union && count($query->union);

[samdark]

Suggested change

	if ($has_union) {
	if ($hasUnion) {

[samdark]

Suggested change

	if ($has_union) {
	if ($hasUnion) {

[mtangoo]

At the moment, my hack is working for me and I am now very busy. I'll try to find time to make another PR.

So can we close this one and wait for the other one?