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:
1 parent
f0274aca89
commit
12401b54e3
2 files changed
+58
-2
No files matched your search
@@ -170,11 +170,19 @@ 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) {
|
// Use the same comparison that determines the ordering below (<=>, which is
|
||||||
|
// built from the same <, ==, > semantics as the "$aValue > $bValue" check) to
|
||||||
|
// detect ties. A strict "===" identity check misses value-equal-but-not-identical
|
||||||
|
// objects (e.g. two DateTimeImmutable instances for the same point in time),
|
||||||
|
// which silently skips the next sort field and can make the comparator
|
||||||
|
// non-antisymmetric.
|
||||||
|
$comparison = $aValue <=> $bValue;
|
||||||
|
|
||||||
|
if ($comparison === 0) {
|
||||||
return $next($a, $b);
|
return $next($a, $b);
|
||||||
}
|
}
|
||||||
|
|
||||||
return ($aValue > $bValue ? 1 : -1) * $orientation;
|
return $comparison * $orientation;
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -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,53 @@ class ClosureExpressionVisitorTest extends TestCase
|
|||||||
self::assertEquals('c', $objects[2]->getBar());
|
self::assertEquals('c', $objects[2]->getBar());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function testSortDelegateWithEqualButNotIdenticalObjectPrimaryField(): void
|
||||||
|
{
|
||||||
|
// Two distinct DateTimeImmutable instances representing the same point in time are
|
||||||
|
// value-equal ("==") but not identical ("==="). The primary field tie must still fall
|
||||||
|
// through to the secondary field instead of being reported as a strict mismatch.
|
||||||
|
$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
|
||||||
|
{
|
||||||
|
// A comparator that only falls through to the next field on a strict "===" tie is not
|
||||||
|
// antisymmetric for value-equal objects: swapping the input order of two equal-but-not-
|
||||||
|
// identical elements must not change which element 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));
|
||||||
|
|||||||
Reference in new issue
Block a user