search

lox / pheasant

A lightweight data mapper designed to take advantage of PHP 5.3+
http://getpheasant.com
MIT License
101 stars 21 forks source link

Fix broken support for chained orderBy(). Fixes issue #65 #68

Closed bjornpost closed 10 years ago

bjornpost commented 11 years ago

Also added a new testcase which test the chaining of orderBy() calls.

lox commented 11 years ago

The only problem I have with this is that it becomes impossible to change the ordering. To match the where and andWhere semantics, it would probably make sense to add an andOrderBy method. Thoughts?

bjornpost commented 11 years ago

The only problem I have with this is that it becomes impossible to change the ordering.

Not sure if I understand this. What do you mean by change the ordering? Why would you want to change those after defining them? Maybe I'm missing your point here :-)

\Model\Post::all()->orderBy('date DESC')->orderBy('title ASC');

To match the where and andWhere semantics, it would probably make sense to add an andOrderBy method. Thoughts?

Makes sense, I'll fix my pull request.

bjornpost commented 11 years ago

Not sure if I understand this. What do you mean by change the ordering? Why would you want to change those after defining them? Maybe I'm missing your point here :-)

ping @lox :-), I'd like to get this fixed asap.

bjornpost commented 11 years ago

ping @lox ?

bjornpost commented 11 years ago

Hi @lox,

I've added the andOrderBy() method: https://github.com/bjornpost/pheasant/commit/eb83004fd3b860aa4e888c4779273806ff0a564f

Can you pull this in? :-)

lox commented 11 years ago

I'm confused @bjornpost, shouldn't the semantics of orderBy() be that it replaces, whereas andOrderBy should append an additional column to the existing orderBy?

bjornpost commented 11 years ago

Ah, ofcourse. Updated: https://github.com/bjornpost/pheasant/commit/09d76489dfa397a4fb63d898e949aad990c3b93d

bjornpost commented 10 years ago

Hey @lox, just noticed this pull request wasn't fully pulled for some reason. The changes as per https://github.com/bjornpost/pheasant/commit/09d76489dfa397a4fb63d898e949aad990c3b93d aren't on master right now. Can you pull those in by hand?

lox commented 10 years ago

If you were to create a pull request, I'd happily merge it.

On Wed, Dec 4, 2013 at 11:26 PM, Bjorn Post notifications@github.comwrote:

Hey @lox https://github.com/lox, just noticed this pull request wasn't fully pulled for some reason. The changes as per bjornpost@09d7648https://github.com/bjornpost/pheasant/commit/09d76489dfa397a4fb63d898e949aad990c3b93daren't on master right now. Can you pull those in by hand?

— Reply to this email directly or view it on GitHubhttps://github.com/lox/pheasant/pull/68#issuecomment-29800630 .