handle non lambda scope properly #10528

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@neerajdotname
Member

neerajdotname commented May 9, 2013

This PR is only for 4-0-stable since in master scope without lambda is no longer supported.

I will add tests and change log if the fix looks ok.

fixes #10421

If @post.comments.approved_only_without_lambda.count is invoked
then the result is equivalent to calling
Comment.approved_only_without_lambda.

This is because when scope is defined without lambda then scope
stores an already built relation.

@post.comments.approved_only_without_lambda invokes the method
using scoping.

scoping changes the current_scope so that the relation that is
to built can make use of the current_scope.

However if scope is defined without lambda then the relation is
already built and I could not find anyway to apply current_scope
on an already built relation.

If I use merge then last where condition wins and that was removing
conditions from current_scope.

Hence I added a way to merge two relations without anyone clobbering
on another.

All existing tests are passing.

/cc @jonleighton

Neeraj Singh added some commits May 8, 2013

Neeraj Singh
extracted piece of code into a method
In order to fix #10421 I need to enable merge to take an option
so that relations could be merged without making the last where
condition to win.

That fix would forever reside in 4-0-stable branch and would not be
merged to master since using scope without lambda has been deprecated.

In this commit I have extracted code into a method and I think it
makes code look better. Hence the request to merge it in both
master and 4-0-stable.

If there is any concern then this code can be merged only in
4-0-stable and that would be fine too.
Neeraj Singh
handle non lambda scope
fixes #10421

If `@post.comments.approved_only_without_lambda.count` is invoked
then the result is equivalent to calling
`Comment.approved_only_without_lambda`.

This is because when `scope` is defined without lambda then `scope`
stores an already built relation.

`@post.comments.approved_only_without_lambda` invokes the method
using `scoping`.

`scoping` changes the `current_scope` so that the relation that is
to built can make use of the `current_scope`.

However if `scope` is defined without lambda then the relation is
already built and I could not find anyway to apply current_scope
on an already built relation.

If I use `merge` then last where condition wins and that was removing
conditions from `current_scope`.

Hence I added a way to merge two relations without anyone clobbering
on another.
@beerlington

This comment has been minimized.

Show comment
Hide comment
@beerlington

beerlington Aug 7, 2013

Contributor

@neerajdotname I would recommend writing tests for this to demonstrate the issue in code.

Contributor

beerlington commented Aug 7, 2013

@neerajdotname I would recommend writing tests for this to demonstrate the issue in code.

@robin850 robin850 added this to the 4.0.6 milestone May 2, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.0.10, 4.1.7 Aug 21, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.1.9, 4.0.13 Nov 19, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 29, 2014

Member

Closing since this PR seems to be stale. Also, with the 4.2 release Rails 4.0 will not accept any bug fix. As scopes without lambdas are deprecated the best way to fix this issue is stoping to use it.

Member

rafaelfranca commented Dec 29, 2014

Closing since this PR seems to be stale. Also, with the 4.2 release Rails 4.0 will not accept any bug fix. As scopes without lambdas are deprecated the best way to fix this issue is stoping to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment