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

[t11n]

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.

[lcobucci]

@t11n comparing DateTime objects is actually fine, but maybe we should be using the space ship operator instead of ($aValue > $bValue) ? 1 : -1. But regardless of that, could you send a PR with a failing functional test? That would be really helpful!

You can find examples on https://github.com/doctrine/doctrine2/tree/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket

[t11n]

@lcobucci Thanks for looking into the issue.

I prepared three tests, that demonstrate the problem: doctrine/orm#6851
(branched from v2.5, since I do not have a php7.1 setup running, yet)

First test shows sorting by scalar values, that works as expected. The other two tests sort by fields, where first ordered by field is a DateTime object and a general stdClass.

It is debatable whether sorting by any other objects, than DateTime makes sense. But since php does have a logic for that to evaluate, it is probably good to adopt it.

Something like that in ClosureExpressionVisitor::sortByField() makes the tests green:

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

It is the identical comparison, that prevents $next from being evaluated, whenever two objects are not identical.

[beberlei]

The ExpressionBuilder has this class level comment about only "scalar" values being supported for interoperability. Obviously a comment is well hidden and hard to spot. We could throw an exception if values are not a scalar value to enforce this, for one big whopping BC break. But given the amount of issues about this it might be worthwhile.

[mpdude]

I may be wrong, but I believe this issue belongs to the doctrine/collections repo?

/cc @greg0ire