Removed where_values_hash from AR::NullRelation #11496

Merged
merged 1 commit into from Sep 29, 2013

Conversation

Projects
None yet
7 participants
@pftg
Contributor

pftg commented Jul 18, 2013

In order to build associated records for owners which has not been saved
need to get where values to use as default attributes.
But for new record owner uses ActiveRecord::NullRelation which
override where_values_hash to return empty hash stub.

where_values_hash is not used to invoke any sql query, but good to
build others chains (even will be never executed) like:

  post          = Post.new
  admin_comment = post.admin_comments.build

  assert_equal 'Admin', admin_comment.author

Closes #11376, #11676 and #11675

@pftg

This comment has been minimized.

Show comment
Hide comment
Contributor

pftg commented Jul 18, 2013

@nathanvda

This comment has been minimized.

Show comment
Hide comment
@nathanvda

nathanvda Aug 8, 2013

👍 Nice work, thank you!

👍 Nice work, thank you!

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Aug 13, 2013

Contributor

Thanks to @beerlington and @Nthalk for their tests and CHANGELOG

Contributor

pftg commented Aug 13, 2013

Thanks to @beerlington and @Nthalk for their tests and CHANGELOG

@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Aug 13, 2013

Contributor

@rafaelfranca may you check this one, thanks in advance.

Contributor

pftg commented Aug 13, 2013

@rafaelfranca may you check this one, thanks in advance.

@jarl-dk

This comment has been minimized.

Show comment
Hide comment
@jarl-dk

jarl-dk Sep 20, 2013

Could someone please merge this, it seems to resolve more than one bug report.

jarl-dk commented Sep 20, 2013

Could someone please merge this, it seems to resolve more than one bug report.

@Maxim-Filimonov

This comment has been minimized.

Show comment
Hide comment
@Maxim-Filimonov

Maxim-Filimonov Sep 24, 2013

Run into the same issue today with Rails 4. Is it gonna be merged for 4.1.0 ?

Run into the same issue today with Rails 4. Is it gonna be merged for 4.1.0 ?

@leiyangyou

This comment has been minimized.

Show comment
Hide comment

+1

Removed where_values_hash from AR::NullRelation
In order to build associated records for owners which has not been saved
need to get where values to use as default attributes.
But for new record owner uses `ActiveRecord::NullRelation` which
override `where_values_hash` to return empty hash stub.

`where_values_hash` is not used to invoke any sql query, but good to
build others chains (even will be never executed) like:

```ruby
  post          = Post.new
  admin_comment = post.admin_comments.build

  assert_equal 'Admin', admin_comment.author
```

Closes #11376, #11676, #11675
@pftg

This comment has been minimized.

Show comment
Hide comment
@pftg

pftg Sep 28, 2013

Contributor

Rebased!

Contributor

pftg commented Sep 28, 2013

Rebased!

rafaelfranca added a commit that referenced this pull request Sep 29, 2013

Merge pull request #11496 from jetthoughts/11376_has_many_assoc_respe…
…ct_scope_on_build

Removed where_values_hash from AR::NullRelation

@rafaelfranca rafaelfranca merged commit 8771a56 into rails:master Sep 29, 2013

1 check passed

default The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 29, 2013

Member

Merged. Sorry for the delay guys

Member

rafaelfranca commented Sep 29, 2013

Merged. Sorry for the delay guys

rafaelfranca added a commit that referenced this pull request Sep 29, 2013

Merge pull request #11496 from jetthoughts/11376_has_many_assoc_respe…
…ct_scope_on_build

Removed where_values_hash from AR::NullRelation
Conflicts:
	activerecord/CHANGELOG.md

@pftg pftg deleted the jetthoughts:11376_has_many_assoc_respect_scope_on_build branch Sep 29, 2013

@jarl-dk

This comment has been minimized.

Show comment
Hide comment
@jarl-dk

jarl-dk Sep 29, 2013

Thanks... Any plans for a 4.0.1 release?

jarl-dk commented Sep 29, 2013

Thanks... Any plans for a 4.0.1 release?

@nathanvda

This comment has been minimized.

Show comment
Hide comment

Thanks @rafaelfranca! 👍

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Sep 30, 2013

Member

@jarl-dk : Yes, the pull request has been backported to the 4-0-stable branch so it will be in 4.0.1 which will be released soon. There is still issues which need to be resolved to get 4.0.1 out unfortunately.

Member

robin850 commented Sep 30, 2013

@jarl-dk : Yes, the pull request has been backported to the 4-0-stable branch so it will be in 4.0.1 which will be released soon. There is still issues which need to be resolved to get 4.0.1 out unfortunately.

@jarl-dk

This comment has been minimized.

Show comment
Hide comment
@jarl-dk

jarl-dk Oct 1, 2013

@robin850: I hope this bug (#12417) is one of those that need to be resolved in 4-0-stable.

jarl-dk commented Oct 1, 2013

@robin850: I hope this bug (#12417) is one of those that need to be resolved in 4-0-stable.

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Oct 1, 2013

Member

@jarl-dk : Most of the time, the issues are put under a "milestone" on this tracker. As you can see, this is under the 4.0.1 milestone so this should be. :-)

Member

robin850 commented Oct 1, 2013

@jarl-dk : Most of the time, the issues are put under a "milestone" on this tracker. As you can see, this is under the 4.0.1 milestone so this should be. :-)

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