Skip to content
This repository has been archived by the owner on Nov 11, 2020. It is now read-only.

Fix default sort order in Query Builder #139

Closed
wants to merge 1 commit into from
Closed

Fix default sort order in Query Builder #139

wants to merge 1 commit into from

Conversation

gerryvdm
Copy link
Contributor

@gerryvdm gerryvdm commented Oct 3, 2013

cursor.sort() expects a value of 1 for ascending or -1 for descending. Calling Builder::sort($field) would result in a sort with a value of 0.

Resorting to a default ascending order when not having specified a sort order.

cursor.sort() expects a value of 1 for ascending or -1 for descending. Calling Builder::sort($field) would result in a sort with a value of 0.

Resorting to a default ascending order when not having specified a sort order.
@jmikola
Copy link
Member

jmikola commented Oct 4, 2013

@gerryvdm: I think proper behavior here would be to throw an InvalidArgumentException, as the method should either be called with either a single array of field/order pairs or two field and order arguments. I'm not keen on defaulting to a particular order based because a required (by the documentation) argument is missing.

Something like "Single field requires an order argument" as the exception message would suffice, if you'd like to revise this.

@gerryvdm
Copy link
Contributor Author

gerryvdm commented Oct 5, 2013

Wouldn't throwing an exception break a lot of exisiting code in projects?

I also don't think it's unreasonable to have a default ascending sort order?

@jmikola
Copy link
Member

jmikola commented Oct 16, 2013

@gerryvdm: Good point on BC. I didn't consider that.

It looks like doctrine/dbal also defaults to ascending mode if the $order parameter is omitted, so this seems like a suitable default behavior.

jmikola added a commit that referenced this pull request Oct 16, 2013
@jmikola
Copy link
Member

jmikola commented Oct 16, 2013

Manually merged. Thanks!

@jmikola jmikola closed this Oct 16, 2013
@gerryvdm gerryvdm deleted the patch-1 branch October 17, 2013 16:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants