fix: use value equality for sortByField tie detection

ClosureExpressionVisitor::sortByField() decided ties with a strict
"===" identity check before falling through to the next sort field.
For object-typed fields (e.g. two DateTimeImmutable instances that
represent the same point in time but are not the same instance),
this check never passes, so secondary/tertiary Criteria::orderBy()
fields were silently ignored and the resulting comparator was not
antisymmetric (swapping the input order of two such elements changed
the sort result).

Replace the "===" tie check with the same "<=>" comparison already
used to determine ordering, so a tie is now detected whenever the
values are equal under that comparison, matching value semantics for
both scalars and comparable objects.

Fixes #437
This commit is contained in:
dualfroz committed 2026-09-06 02:39:00 +02:00
1 parent dd6d913ae6
commit 2572a60593
2 files changed
+50 -2

No files matched your search

+4 -2
View File
@@ -170,11 +170,13 @@ class ClosureExpressionVisitor extends ExpressionVisitor
$aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name, $accessRawFieldValues); $aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name, $accessRawFieldValues);
$bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name, $accessRawFieldValues); $bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name, $accessRawFieldValues);
if ($aValue === $bValue) { $comparison = $aValue <=> $bValue;
if ($comparison === 0) {
return $next($a, $b); return $next($a, $b);
} }
return ($aValue > $bValue ? 1 : -1) * $orientation; return $comparison * $orientation;
}; };
} }
+46
View File
@@ -6,6 +6,7 @@ namespace Doctrine\Tests\Common\Collections;
use ArrayAccess; use ArrayAccess;
use ArrayIterator; use ArrayIterator;
use DateTimeImmutable;
use Doctrine\Common\Collections\Expr\ClosureExpressionVisitor; use Doctrine\Common\Collections\Expr\ClosureExpressionVisitor;
use Doctrine\Common\Collections\Expr\Comparison; use Doctrine\Common\Collections\Expr\Comparison;
use Doctrine\Common\Collections\Expr\CompositeExpression; use Doctrine\Common\Collections\Expr\CompositeExpression;
@@ -487,6 +488,51 @@ class ClosureExpressionVisitorTest extends TestCase
self::assertEquals('c', $objects[2]->getBar()); self::assertEquals('c', $objects[2]->getBar());
} }
public function testSortDelegateWithEqualButNotIdenticalObjectPrimaryField(): void
{
// Value-equal but not identical primary values must still fall through to
// the secondary field.
$sameInstant = static fn (): DateTimeImmutable => new DateTimeImmutable('2024-12-10 16:04:11');
$objects = [
new TestObject($sameInstant(), 5),
new TestObject($sameInstant(), 6),
];
self::assertNotSame($objects[0]->getFoo(), $objects[1]->getFoo());
self::assertEquals($objects[0]->getFoo(), $objects[1]->getFoo());
$sort = ClosureExpressionVisitor::sortByField('bar', -1, null, true);
$sort = ClosureExpressionVisitor::sortByField('foo', -1, $sort, true);
usort($objects, $sort);
self::assertEquals(6, $objects[0]->getBar());
self::assertEquals(5, $objects[1]->getBar());
}
public function testSortByFieldComparatorIsAntisymmetricForEqualButNotIdenticalObjects(): void
{
// Swapping the input order of two value-equal elements must not change
// which one sorts first.
$sameInstant = static fn (): DateTimeImmutable => new DateTimeImmutable('2024-12-10 16:04:11');
$element5 = new TestObject($sameInstant(), 5);
$element6 = new TestObject($sameInstant(), 6);
$sort = ClosureExpressionVisitor::sortByField('bar', -1, null, true);
$sort = ClosureExpressionVisitor::sortByField('foo', -1, $sort, true);
$orderedFromAscendingInput = [$element5, $element6];
usort($orderedFromAscendingInput, $sort);
$orderedFromDescendingInput = [$element6, $element5];
usort($orderedFromDescendingInput, $sort);
self::assertEquals(6, $orderedFromAscendingInput[0]->getBar());
self::assertEquals(6, $orderedFromDescendingInput[0]->getBar());
}
public function testArrayComparison(): void public function testArrayComparison(): void
{ {
$closure = $this->visitor->walkComparison($this->builder->eq('foo', 42)); $closure = $this->visitor->walkComparison($this->builder->eq('foo', 42));