Support multiple order by statements in the discussion model (breaks backwards compat of discussion sorting) #3523
Conversation
Add Score to allowed sort fields.
|
|
||
| $this->EventArguments['SortField'] = &$SortField; | ||
| $this->EventArguments['SortDirection'] = c('Vanilla.Discussions.SortDirection', 'desc'); | ||
| $this->EventArguments['OrderFields'] = &$orderFields; |
There was a problem hiding this comment.
This breaks backwards compatibility for any plugin manipulating this, and I don't see any good reason for that.
There was a problem hiding this comment.
This pull request is to support multiple order by clauses. Keeping the single string-type sort field as an argument means that we would need to build extra logic (i.e., if the OrderFields exist, use that, otherwise default to the SortField... etc.) Consolidating the sort fields and directions into one array is simpler and more explicit in that we know which direction belongs to which field.
I've done a search to see whether these arguments are even being used in Vanilla and it came up empty. I think it's a pretty safe enhancement.
There was a problem hiding this comment.
It's gonna break some open source plugins for sure. There was a whole conversation about using this hook a couple weeks back. Super conflicted about this.
There was a problem hiding this comment.
To be clear: conflicted about whether we need some logic here to restore backwards compat, not the need for the change.
linc
changed the title
linc
added
the
Status: WIP
linc
changed the title
|
I feel like maybe we should merge this into #3528 as 1 big pull request since we're now destroying the old discussion sorting behavior and wholesale replacing it. |
|
Merged into #3528 |
Add a function that checks order by values.
Add order by values in a loop to support multiple order by values.
Adds Score to allowed sort fields.
Gives events the ability to override these values.