From 2572a60593984f91afbb9e8e3772fcfb07d63986 Mon Sep 17 00:00:00 2001 From: DualFroz Date: Fri, 4 Sep 2026 06:21:06 +0200 Subject: [PATCH] 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 --- src/Expr/ClosureExpressionVisitor.php | 6 ++-- tests/ClosureExpressionVisitorTest.php | 46 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/Expr/ClosureExpressionVisitor.php b/src/Expr/ClosureExpressionVisitor.php index a397b4b..fdf76eb 100644 --- a/src/Expr/ClosureExpressionVisitor.php +++ b/src/Expr/ClosureExpressionVisitor.php @@ -170,11 +170,13 @@ class ClosureExpressionVisitor extends ExpressionVisitor $aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name, $accessRawFieldValues); $bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name, $accessRawFieldValues); - if ($aValue === $bValue) { + $comparison = $aValue <=> $bValue; + + if ($comparison === 0) { return $next($a, $b); } - return ($aValue > $bValue ? 1 : -1) * $orientation; + return $comparison * $orientation; }; } diff --git a/tests/ClosureExpressionVisitorTest.php b/tests/ClosureExpressionVisitorTest.php index 182f783..764b304 100644 --- a/tests/ClosureExpressionVisitorTest.php +++ b/tests/ClosureExpressionVisitorTest.php @@ -6,6 +6,7 @@ namespace Doctrine\Tests\Common\Collections; use ArrayAccess; use ArrayIterator; +use DateTimeImmutable; use Doctrine\Common\Collections\Expr\ClosureExpressionVisitor; use Doctrine\Common\Collections\Expr\Comparison; use Doctrine\Common\Collections\Expr\CompositeExpression; @@ -487,6 +488,51 @@ class ClosureExpressionVisitorTest extends TestCase 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 { $closure = $this->visitor->walkComparison($this->builder->eq('foo', 42));