PersistentCollection::matching() merges orderings incorrectly when association has default sorting #6315

Closed
opened 2026-01-22 15:30:45 +01:00 by admin · 3 comments
Owner

Originally created by @paxuclus on GitHub (Sep 27, 2019).

Originally assigned to: @paxuclus on GitHub.

Bug Report

When PersistentCollection::matching() is called with orderings, they are not correctly merged together with the default sortings of an association.

Q A
BC Break yes/no
Version 2.6.4

Summary

When a relationship has a default sorting configured using @ORM\OrderBy({"lastName" = "ASC", "firstName" = "ASC"}), the orderings are not correctly merged together with the orderings of the $criteria in PersistentCollection::matching().

Current behavior

The orderings will be sorted like the array given to the @ORM\OrderBy annotation, leading to unexpected results when passing another orderBy to the $criteria of PersistentCollection::matching().

How to reproduce

  • Define a relationship with @ORM\OrderBy({"lastName" = "ASC", "firstName" = "ASC"}) (sort by at least 2 properties)
  • Call $relationship->matching(Criteria::create()->orderBy(['firstName' => 'DESC'])) on the PersistentCollection of that relationship

Expected behavior

The collection should be ordered by firstName DESC, lastName ASC, but instead it will be ordered by lastName ASC, firstName DESC.

Origin of the bug

Original Issue: #7767
The bug was introduced in this PR: https://github.com/doctrine/orm/pull/7766/files#diff-108586f774fc8acb02163ed709e05e86R675

Using $criteria->orderBy(array_merge($criteria->getOrderings(), $this->association['orderBy'] ?? [], $criteria->getOrderings())); there would fix the ordering.

Originally created by @paxuclus on GitHub (Sep 27, 2019). Originally assigned to: @paxuclus on GitHub. ### Bug Report When PersistentCollection::matching() is called with orderings, they are not correctly merged together with the default sortings of an association. | Q | A |------------ | ------ | BC Break | yes/no | Version | 2.6.4 #### Summary When a relationship has a default sorting configured using `@ORM\OrderBy({"lastName" = "ASC", "firstName" = "ASC"})`, the orderings are not correctly merged together with the orderings of the `$criteria` in PersistentCollection::matching(). #### Current behavior The orderings will be sorted like the array given to the `@ORM\OrderBy` annotation, leading to unexpected results when passing another orderBy to the `$criteria` of PersistentCollection::matching(). #### How to reproduce * Define a relationship with `@ORM\OrderBy({"lastName" = "ASC", "firstName" = "ASC"})` (sort by at least 2 properties) * Call `$relationship->matching(Criteria::create()->orderBy(['firstName' => 'DESC']))` on the PersistentCollection of that relationship #### Expected behavior The collection should be ordered by `firstName DESC, lastName ASC`, but instead it will be ordered by `lastName ASC, firstName DESC`. #### Origin of the bug Original Issue: #7767 The bug was introduced in this PR: https://github.com/doctrine/orm/pull/7766/files#diff-108586f774fc8acb02163ed709e05e86R675 Using `$criteria->orderBy(array_merge($criteria->getOrderings(), $this->association['orderBy'] ?? [], $criteria->getOrderings()));` there would fix the ordering.
admin added the Bug label 2026-01-22 15:30:45 +01:00
admin closed this issue 2026-01-22 15:30:45 +01:00
Author
Owner

@SenseException commented on GitHub (Sep 29, 2019):

Thank you for reporting this issue. If you like, you can create a PR with a fix and test(s) to this issue.

As far as I understand you describe a regression between two versions/commits for the orderBy behavior. But I'm not sure if there's an intended or defined behavior for a use case like this.

When I read $relationship->matching(Criteria::create()->orderBy(['firstName' => 'DESC'])) shouldn't it override the OrderBy annotation instead of mixing it? I would expect that firstName DESC will be the only order in here.

If you want to create a PR, you should aim to restore the previous known behavior you described, but I wonder if we should change it for the next major release.

@SenseException commented on GitHub (Sep 29, 2019): Thank you for reporting this issue. If you like, you can create a PR with a fix and test(s) to this issue. As far as I understand you describe a regression between two versions/commits for the orderBy behavior. But I'm not sure if there's an intended or defined behavior for a use case like this. When I read `$relationship->matching(Criteria::create()->orderBy(['firstName' => 'DESC']))` shouldn't it override the `OrderBy` annotation instead of mixing it? I would expect that `firstName DESC` will be the only order in here. If you want to create a PR, you should aim to restore the previous known behavior you described, but I wonder if we should change it for the next major release.
Author
Owner

@paxuclus commented on GitHub (Oct 1, 2019):

Hey @SenseException,

thinking about this again I agree with you that "orderBy()" should override the default sorting.

Previously (before #7766 was merged), the "->matching()" would completely ignore the OrderBy annotation, even if there were no orderings in the $criteria.

I think the PersistentCollection should only use the OrderBy annotation if the $criteria does not contain any orderings.

I will try to create a PR this week. 👍

@paxuclus commented on GitHub (Oct 1, 2019): Hey @SenseException, thinking about this again I agree with you that "orderBy()" should override the default sorting. Previously (before #7766 was merged), the "->matching()" would completely ignore the `OrderBy` annotation, even if there were no orderings in the `$criteria`. I think the PersistentCollection should only use the `OrderBy` annotation if the `$criteria` does not contain any orderings. I will try to create a PR this week. 👍
Author
Owner

@lcobucci commented on GitHub (Oct 2, 2019):

Handled by #7850

@lcobucci commented on GitHub (Oct 2, 2019): Handled by #7850
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6315