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

[cepaim]

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

[codecov[bot]]

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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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():

https://github.com/yiisoft/yii2/blob/34d2396920a34534fb19cc342b5f4fe863c6e3ac/framework/db/QueryBuilder.php#L225-L269

$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.

[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?

[samdark]

@santilin do you maybe have a bit more time to finish it?

[Izumi-kun]

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

https://github.com/yiisoft/yii2/blob/34d2396920a34534fb19cc342b5f4fe863c6e3ac/framework/db/QueryBuilder.php#L225-L269

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

This will break this test case (actual count will be 2): https://github.com/yiisoft/yii2/blob/34d2396920a34534fb19cc342b5f4fe863c6e3ac/tests/framework/db/QueryTest.php#L339-L355

BTW, what result in the test is actually expected? :/