This project is archived and is in readonly mode.
named_scope doesn't override default_scope's :order key
There's a problem of not overriding deafult_scope's keys that don't merge (:order for instance) by same keys of named_scopes
class Post
default_scope :order => 'posts.created_at asc'
named_scope :recent, :order => 'posts.created_at desc'
end
Post.recent # sorts posts in ascending order
This behavior appeared as a result of resolving #1960
Here's the the patch to fix it Patch also fixes test that was broken too (see comments on #1960)
Added code checks if scoping that was current on creation of Scope object is in scoped_methods stack on execution of scope, and if so - doesn't apply it second time (as it's been already implicitly accounted for during merging named scope's scoping with current_scoped_methods). This eliminates a problem of overriding keys of named scope by default scope
-
Max Lapshin+1 Fix this, please.
-
Yaroslav Markin+1 since 2.3 named_scope merging is a problem for all folks using default_scope
-
Jérôme
+1
-
Anton Ageev+1
-
-
igonef
+1
-
-
Eloy DuranI'm gonna be the party pooper here, but simply saying "+1" does nothing good for the ticket. Please see the contribution guide linked in the help text =>.
The part about commenting:
Reviewing Changes Are you happy with the tests, can you follow what they're testing, is there anything missing Does the documentation still seem right to you Do you like the implementation, can you think of a nicer or faster way to implement a part of their change Once you're happy it's a good change, comment on the lighthouse ticket indicating your approval. Your comment should indicate that you like the change and what you like about it. Something like: I like the way you've restructured that code in generate_finder_sql, much nicer. The tests look good too. If your comment simply says +1, then odds are other reviewers aren't going to take it too seriously. Show that you took the time to review the patch. Once three people have approved it, add the verified tag. This will bring it to the attention of a committer who'll then review the changes looking for the same kinds of things.
-
Yaroslav Markin
@Eloy AFAIR old Rails Trac used to have something like "+1 only if you've tested it yourself", and I think this is the case here — tests pass, works as expected, "works for me".
When someone comes with "something's wrong with this", well, different story.
-
Eloy Duran
@Eloy AFAIR old Rails Trac used to have something like "+1 only if you've tested it yourself", and I think this is the case here — tests pass, works as expected, "works for me".
Well I don't know about the past, I'm just reading what it currently says:
Your comment should indicate that you like the change and what you like about it. Something like: I like the way you've restructured that code in generate_finder_sql, much nicer. The tests look good too. If your comment simply says +1, then odds are other reviewers aren't going to take it too seriously. Show that you took the time to review the patch.
So in both cases (good/problem) only a +/- 1 won't be taken too seriously. Which I can understand as it's basically not constructive criticism. Note that I'm also not claiming it's deconstructive criticism, it's neither :)
-
PratikI agree with Eloy. Being the asignee here, I wouldn't take seriously the massive influx of +1s in a very short time.
That is not to say there is anything wrong with the patch itself. Patch looks ok and I'll apply the next time I am applying pending patches.
-
Yaroslav Markin
- Tag changed from 2.3, activerecord, bug, default_scope, named_scope, patch to 2.3, activerecord, bug, default_scope, named_scope, patch, verified
-
Repository
- State changed from new to committed
(from [db0bfe4ede3cdfc2e4ccdb2a89525a914e6d0913]) Default scope :order should be overridden by named scopes.
[#2346 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
-
Repository
(from [628b4ad679b1971427a20461e8c2332d492e4655]) Default scope :order should be overridden by named scopes.
[#2346 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
-
JonI'm not so sure this bug is fixed. Product.my_named_scope works now, but I'm losing the order key on something like @store.products.my_named_scope. So a scoped query isn't respecting the :order key of a named_scope when a default_scope is present or an order clause of a parent model is present.
I made a gist of some of my app's output here: http://gist.github.com/136140
Probably related to ticket #2253
-
jack dempsey (jackdempsey)Not working for me as well. Following a couple of the links, starting with Jon's to ticket #2253, lead to what looks like a good solution. Will try the patch later if I can, but for now wanted to give another "ain't working here".
-
Jon
Thanks Jack. In my experimentation, some of the other keys weren't working either. Primarily :select. This is a pretty dangerous bug.
-
Jamie Hill- Tag changed from 2.3, activerecord, bug, default_scope, named_scope, patch, verified to 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified
Sorry to resurrect an old ticket but this is definitely still an issue, default_scope becomes pretty much unusable without being able to override the :order etc. in named scopes.
-
JeroenAs far as I can see this ticket is still open.
I just ran against this problem and it seems I'm still encountering it (2.3.5 and 2.3.8).
Am I overlooking another ticket on this?
-
Santiago Pastorino
- State changed from committed to open
-
Jan Xie- Tag changed from 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified to rails 3.0.0.beta4, 2.3, 2.3.5, activerecord, bug, default_scope, named_scope, patch, verified
I just met the same problem today, I'm using edge rails (version string = Rails 3.0.0.beta4). Should I create a different ticket?
-
Santiago Pastorino
Jan add the patch for 2-3-stable here
-
Neeraj Singh- Milestone changed from 2.x to 2.3.9
- Importance changed from to
-
Santiago Pastorino
Can someone check if this is still an issue on 2-3-stable and provide a backport please.
Thanks. -
Lars Kuhnt
I am still having this Problem on 2-3-stable with polymorphic has_many associations.
-
-
-
Ryan Bigg
Automatic cleanup of spam.
-
Ryan Bigg
Automatic cleanup of spam.
-
Steve PurcellAs of the 3.0.3 release, this bug is now present in the latest Rails. I can't comment on the proposed patch, but I will say that I haven't found a tidy way to work around this problem.
-
Neeraj Singh
@Steve the behavior you are seeing in Rails 3.0.3 is not a bug.
class Car < ActiveRecord::Base default_scope :order => 'id desc' scope :recent, :order => 'name desc' end Car.recent #=> SELECT "cars".* FROM "cars" ORDER BY id desc, name descIn the above case because of default_scope order has 'id desc' at the first. That's okay.
What you might argue is a bug is that if you try User.unscoped.recent then alos you see 'id desc'. I have documented that. In this case you need to use block form of unscoped.
User.unscoped { recent } #=> SELECT "cars".* FROM "cars" ORDER BY name desc