Skip to content
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

Opt into has many inverse #37429

Merged
merged 1 commit into from Oct 15, 2019

Conversation

@gmcgibbon
Copy link
Member

gmcgibbon commented Oct 10, 2019

Summary

Followup on #34533, #37413

Make has_many inversing support available through an opt-in config variable. This behaviour is likely to break existing applications, but it is correct behaviour.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:opt_into_has_many_inverse branch 5 times, most recently from 77068b8 to 6293ff0 Oct 10, 2019
@gmcgibbon gmcgibbon requested a review from rafaelfranca Oct 11, 2019
@@ -67,6 +68,12 @@ class Railtie < Rails::Railtie # :nodoc:
require "active_record/base"
end

initializer "active_record.has_many_inversing" do |app|
ActiveSupport.on_load(:active_record) do
self.has_many_inversing = app.config.active_record.has_many_inversing

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Oct 11, 2019

Member

You don't need this intializer. The setting config initializer will take care of it.

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Oct 11, 2019

Author Member

Fixed. CI should also pass now

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:opt_into_has_many_inverse branch from 6293ff0 to 083dc69 Oct 11, 2019
Make has_many inversing support available through an opt-in config
variable. This behaviour is likely to break existing applications, but
it is correct behaviour.
@gmcgibbon gmcgibbon force-pushed the gmcgibbon:opt_into_has_many_inverse branch from 083dc69 to 74201c3 Oct 11, 2019
@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Oct 13, 2019

We'd gradually improved automatic inverse_of detection without opt-in configuration (e.g. #28808, #28830).
Is that change (#34533) broken any app in Shopify?

@gmcgibbon

This comment has been minimized.

Copy link
Member Author

gmcgibbon commented Oct 14, 2019

@kamipo Yes, it did introduce breakages. I think it was related to two different cases:

  1. Code that doesn't expect the inverse to be set on has_many relations (I think this counts as a behaviour change in Active Record, so I'm choosing to make it opt-in in this PR)
  2. Associations that use before_add or after_add (fixed in #37413)

See #34533 (comment)

@eileencodes

This comment has been minimized.

Copy link
Member

eileencodes commented Oct 15, 2019

@gmcgibbon and @kamipo is this ready to be merged? We're hitting this issue at GitHub too 😄

@gmcgibbon gmcgibbon requested a review from rafaelfranca Oct 15, 2019
@gmcgibbon

This comment has been minimized.

Copy link
Member Author

gmcgibbon commented Oct 15, 2019

@eileencodes Yep, it should be ready to merge. I'm just waiting for a review.

@gmcgibbon gmcgibbon merged commit 41bbd7c into rails:master Oct 15, 2019
2 checks passed
2 checks passed
buildkite/rails Build #64293 passed (18 minutes, 50 seconds)
Details
codeclimate All good!
Details
@gmcgibbon gmcgibbon deleted the gmcgibbon:opt_into_has_many_inverse branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.