collections icon indicating copy to clipboard operation
collections copied to clipboard

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

Open t11n opened this issue 7 years ago • 0 comments

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.

t11n avatar Nov 27 '17 10:11 t11n