Fix #20239: fix yii\data\ActiveDataProvider to avoid unexpected pagination results with UNION queries

[Izumi-kun]

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

Related PR: #20246

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.86%. Comparing base (4859c8f) to head (efb8b5e).

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20311      +/-   ##
============================================
+ Coverage     64.85%   64.86%   +0.01%     
- Complexity    11435    11437       +2     
============================================
  Files           431      431              
  Lines         37193    37203      +10     
============================================
+ Hits          24120    24131      +11     
+ Misses        13073    13072       -1     

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

[samdark]

Let's exclude composer.lock changes.

[samdark]

@rob006 would you please take a look?

[samdark]

Can we return $this->query if the condition isn't met right away?

if (!$wrapper instanceof Query || empty($wrapper->union)) {
    return $this->query;
}

[bizley]

Rather return $wrapper;

[Izumi-kun]

I decided to keep the original logic (not reusing the original query):

yii2/framework/data/ActiveDataProvider.php

Lines 98 to 104 in b0b7832

	protected function prepareModels()
	{
	if (!$this->query instanceof QueryInterface) {
	throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
	}
	$query = clone $this->query;
	if (($pagination = $this->getPagination()) !== false) {

yii2/framework/data/ActiveDataProvider.php

Lines 162 to 168 in b0b7832

	protected function prepareTotalCount()
	{
	if (!$this->query instanceof QueryInterface) {
	throw new InvalidConfigException('The "query" property must be an instance of a class that implements the QueryInterface e.g. yii\db\Query or its subclasses.');
	}
	$query = clone $this->query;
	return (int) $query->limit(-1)->offset(-1)->orderBy([])->count('*', $this->db);

Should I change it?

[rob006]

There might be more properties that needs to be reset. For example ActiveQuery has on and joinWith (and sql?) properties, that may leak to wrapper. Developer may extend these classes and create own properties with the same problem. Query needs some kind of reset() method, that would be responsible for resting query state.

Or we cold use new Query(), similar to ActiveQuery::queryScalar():

yii2/framework/db/ActiveQuery.php

Line 355 in b0b7832

$command = (new Query())->select([$selectExpression])

[Izumi-kun]

Using a new Query() will break prepareModels() itself if query is an ActiveQuery and the expected return value of ActiveQuery::all() is array of objects rather then an array of arrays. I'll think about the solution a little more.

[Izumi-kun]

I don't really like the implementation, it looks like a workaround. The design of the query build doesn't allow for a more proper solution without breaking BC. Or am I missing something?

[samdark]

Moved to next milestone for now.

[samdark]

@Izumi-kun any idea about better solution?

[Izumi-kun]

@Izumi-kun any idea about better solution?

No any new solutions without BC.