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

[Sortable] SortableListener uses wrong compareTo check if Comparable is used #2541

Closed
AndreasA opened this issue Nov 30, 2022 · 0 comments · Fixed by #2542
Closed

[Sortable] SortableListener uses wrong compareTo check if Comparable is used #2541

AndreasA opened this issue Nov 30, 2022 · 0 comments · Fixed by #2542

Comments

@AndreasA
Copy link
Contributor

In https://github.com/doctrine-extensions/DoctrineExtensions/blob/v3.10.0/src/Sortable/SortableListener.php#L266

the sortable listener uses compareTo to find out if the object of the group is matching.

However, the corresponding interface https://github.com/doctrine/common/blob/3.4.3/src/Comparable.php

says it should return 0 upon a match and 1 / -1 according to sorting as that interface could also be used for sorting.

In combination with https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/src/Sortable/SortableListener.php#L275 the sortable listener expects $matches to be a boolean value (or at least 1 and not 0).

see also:

$matches = $gr == $value;

There was a previous PR #2185 but that was never merged - probably as the one comment was never fixed.

@AndreasA AndreasA changed the title SortableListener uses wrong compareTo check if Comparable is used [Sortable] SortableListener uses wrong compareTo check if Comparable is used Nov 30, 2022
phansys pushed a commit that referenced this issue Dec 1, 2022
rotdrop pushed a commit to rotdrop/DoctrineExtensions that referenced this issue Apr 15, 2023
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

Successfully merging a pull request may close this issue.

1 participant