doctrine / doctrine/collections

ClosureExpressionVisitor::sortByField() fails when comparing Objects/Dates

Open
#136 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
6k
Forks
187
PR merge metrics
No merged PRs in 30d

Description

In method `ClosureExpressionVisitor::sortByField()` we have the following sort function...

```
return function ($a, $b) use ($name, $next, $orientation) {
$aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name);
$bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name);

if ($aValue === $bValue) {
return $next($a, $b);
}

return (($aValue > $bValue) ? 1 : -1) * $orientation;
};
```

... which does not work with Objects, especially DateTime objects. I do not know if it makes sense to compare objects in general if they are greater or lesser, but it definitely makes sense for DateTime objects.

The problem is, that equal DateTime objects will not be recognised by `$aValue === $bValue`. Instead `$aValue > $bValue` will evaluate to false in these cases. Not a big deal you may think, as the a element is put below the b element, which is fine, as they are equal. That's right, but $next will neither be executed.

In my use case, sorting by date is the first order criteria, sorting by ID is the second. That code will never evaluate the second criteria, though.

Possible fix:
```
if ($aValue instanceof \DateTime && $aValue == $bValue || $aValue === $bValue) {
return $next($a, $b);
}
```

You have to judge whether it makes sense to compare other object types. For DateTimes however, this is necessary, I suppose. Sorting by date with a second order by criteria is not possible otherwise.

Please have a look into the matter.

See https://github.com/doctrine/doctrine2/issues/6712 for original issue and PR demonstrating it.

Contributor guide

Open the contributing guide

Research direction

Start by locating ClosureExpressionVisitor::sortByField() and reproduce the comparator with equal DateTime values and a secondary ID criterion. Check the existing comparison behavior and verify that equal dates delegate to $next so the secondary ordering is applied.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.