search

nextras / orm

Orm with clean object design, smart relationship loading and powerful collections.
https://nextras.org/orm
MIT License
309 stars 59 forks source link

Remove ORDER BY when counting collection items #529

Closed mrceperka closed 1 week ago

mrceperka commented 3 years ago

Describe the bug Calling $collection->countStored() should remove ORDER BY clause

To Reproduce

// SomeMapper.php

$builder->addOrderBy('id');
$builder->limitBy(10, 0);
// ...
$collection = $this->toCollection($builder);

// counting with ORDER BY, which is weird, because order does not matter
$total = $collection->countStored();

// hotfix
$collection  = $this->toCollection((clone $builder)->orderBy(null));
// counting without ORDER BY
$total = $collection->countStored();

Expected behavior ORDER BY clause should be removed

Versions:

TODO

mrceperka commented 3 years ago

@hrach can this if statement be removed? https://github.com/nextras/orm/blob/82d0cfc277828ad71cbe66c02fb9de0577a7d74c/src/Collection/DbalCollection.php#L311

hrach commented 3 years ago

Probably. Not fully sure now what was the reason for it. You may try it and see if any test "needs it".

mrceperka commented 3 years ago

Probably. Not fully sure now what was the reason for it. You may try it and see if any test "needs it".

I've found it :(

-- FAILED: integration/Collection/collection.phpt dataprovider=sqlsrv|/home/runner/work/orm/orm/tests/cases/integration/Collection/../../../databases.ini method=testCountStoredDbalWithoutOrderByClause
   Exited with error code 255 (expected 0)
   Nextras\Dbal\Drivers\Exception\QueryException: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Incorrect syntax near 'OFFSET'.
hrach commented 3 years ago

We could workaround it and keep the ordering only for MSSQL.