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 (d146307) to head (30bc262).

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20311      +/-   ##
============================================
+ Coverage     64.84%   64.86%   +0.01%     
- Complexity    11433    11435       +2     
============================================
  Files           431      431              
  Lines         37187    37197      +10     
============================================
+ Hits          24115    24126      +11     
+ Misses        13072    13071       -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]

Do we need a clone here?

[Izumi-kun]

Tests pass successfully without clone. I think it can be removed.

[Izumi-kun]

But this kind of test will fail without clone.

$provider = new ActiveDataProvider([
    'query' => Item::find(),
]);
$pagination = $provider->getPagination();
$pagination->pageSize = 2;
$provider->prepare();
$this->assertCount(2, $provider->getModels());

$provider->setPagination(false);
$provider->prepare(true);
$this->assertCount(5, $provider->getModels());

[samdark]

Makes sense.