New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute default_scope defined by abstract class within the scope of subclass #23666

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
8 participants
@meinac
Contributor

meinac commented Feb 14, 2016

This commit will fix #23413 and #10658. In old behaviour it tries to execute block given by default_scope within the context of abstract class and in this case it can't find the table name. Now it tries to execute the block within subclass and everything works.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 14, 2016

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@meinac meinac force-pushed the meinac:fix_abstract_default_scope_bug branch Feb 14, 2016

@meinac

This comment has been minimized.

Contributor

meinac commented Feb 14, 2016

I've fixed the failing tests but travis is not working, maybe we need to close and reopen this.
Also WDYT about this change @sgrif?

@swrobel

This comment has been minimized.

Contributor

swrobel commented Feb 20, 2016

👏

@swrobel

This comment has been minimized.

Contributor

swrobel commented Feb 20, 2016

r? @sgrif

@rafaelfranca

View changes

activerecord/lib/active_record/scoping/default.rb Outdated
@@ -115,7 +115,10 @@ def build_default_scope(base_rel = nil) # :nodoc:
base_rel ||= relation
evaluate_default_scope do
default_scopes.inject(base_rel) do |default_scope, scope|
default_scope.merge(base_rel.scoping { scope.call })
scoped = base_rel.scoping do
scope.is_a?(Proc) ? instance_exec(&scope) : scope.call

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 1, 2016

Member

Is there any case where scope is not a Proc?

This comment has been minimized.

@meinac

meinac Mar 1, 2016

Contributor

Yes, any kind of object can be used in default scope if the object is callable. I saw this in Rails tests, for example in this case it uses OpenStruct.
But now I think that I have to handle the call method case also in correct context. Change is on the way.

@meinac meinac force-pushed the meinac:fix_abstract_default_scope_bug branch Mar 1, 2016

@meinac

This comment has been minimized.

Contributor

meinac commented Mar 1, 2016

@rafaelfranca I've changed the logic. If you have a look at the line you can see that we can use any kind of object which respond to call method as default scope and now we can handle it in correct context.
Thank you for looking into this pr.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 1, 2016

This looks good to me. r? @sgrif WDYT?

@rails-bot rails-bot assigned sgrif and unassigned pixeltrix Mar 1, 2016

@meinac meinac force-pushed the meinac:fix_abstract_default_scope_bug branch Mar 2, 2016

@sgrif

View changes

activerecord/lib/active_record/scoping/default.rb Outdated
@@ -115,7 +115,8 @@ def build_default_scope(base_rel = nil) # :nodoc:
base_rel ||= relation
evaluate_default_scope do
default_scopes.inject(base_rel) do |default_scope, scope|
default_scope.merge(base_rel.scoping { scope.call })
scope = scope.is_a?(Proc) ? scope : scope.method(:call)

This comment has been minimized.

@sgrif

sgrif Mar 7, 2016

Member

Should this be respond_to?(:to_proc)?

@sgrif

View changes

activerecord/lib/active_record/scoping/default.rb Outdated
@@ -115,7 +115,8 @@ def build_default_scope(base_rel = nil) # :nodoc:
base_rel ||= relation
evaluate_default_scope do
default_scopes.inject(base_rel) do |default_scope, scope|
default_scope.merge(base_rel.scoping { scope.call })
scope = scope.is_a?(Proc) ? scope : scope.method(:call)
default_scope.merge(base_rel.scoping{instance_exec(&scope)})

This comment has been minimized.

@sgrif

sgrif Mar 7, 2016

Member

Should this just be base_rel.instance_exec(&scope)?

@meinac meinac force-pushed the meinac:fix_abstract_default_scope_bug branch Mar 8, 2016

@meinac meinac force-pushed the meinac:fix_abstract_default_scope_bug branch to 0784dcc Mar 8, 2016

@meinac

This comment has been minimized.

Contributor

meinac commented Mar 8, 2016

@sgrif rebased & updated this.

sgrif added a commit that referenced this pull request Mar 8, 2016

Merge pull request #23666 from meinac/fix_abstract_default_scope_bug
Execute default_scope defined by abstract class within the scope of subclass

@sgrif sgrif merged commit da1fecc into rails:master Mar 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meinac meinac deleted the meinac:fix_abstract_default_scope_bug branch Mar 8, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Mar 10, 2016

@sgrif Should this be backported to 4-2-stable as per #23413 (comment) ?

rafaelfranca added a commit that referenced this pull request May 24, 2016

Merge pull request #23666 from meinac/fix_abstract_default_scope_bug
Execute default_scope defined by abstract class within the scope of subclass

rafaelfranca added a commit that referenced this pull request May 27, 2016

Revert "Merge pull request #23666 from meinac/fix_abstract_default_sc…
…ope_bug"

This reverts commit bd44967.

Reason: tests are broken after this change.

MichaelSp added a commit to MichaelSp/rails that referenced this pull request Nov 2, 2016

Merge pull request rails#23666 from meinac/fix_abstract_default_scope…
…_bug

Execute default_scope defined by abstract class within the scope of subclass

MichaelSp added a commit to MichaelSp/rails that referenced this pull request Nov 2, 2016

Revert "Merge pull request rails#23666 from meinac/fix_abstract_defau…
…lt_scope_bug"

This reverts commit bd44967.

Reason: tests are broken after this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment