Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
t11n opened this issue Nov 27, 2017 · 0 comments
Open

Comments

@t11n
Copy link

t11n commented Nov 27, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant