Put plural inverse association inference behind a configuration flag #50883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Forgot to cc @seanpdoyle and @davidstosik |
seanpdoyle
reviewed
Jan 26, 2024
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt
Outdated
Show resolved
Hide resolved
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt
Outdated
Show resolved
Hide resolved
casperisfine
force-pushed
the
infer-inverse-of-deprecation
branch
3 times, most recently
from
841d9a3
to
0e8c2ce
Compare
casperisfine
force-pushed
the
infer-inverse-of-deprecation
branch
from
0e8c2ce
to
1416392
Compare
seanpdoyle
reviewed
Mar 21, 2024
seanpdoyle
reviewed
Mar 21, 2024
seanpdoyle
reviewed
Mar 21, 2024
casperisfine
force-pushed
the
infer-inverse-of-deprecation
branch
from
1416392
to
b8166e3
Compare
Edouard-chin
approved these changes
Mar 22, 2024
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_2.rb.tt
Outdated
Show resolved
Hide resolved
4 tasks
seanpdoyle
reviewed
Mar 24, 2024
| @@ -11,6 +11,7 @@ module Reflection # :nodoc: | |||
| class_attribute :_reflections, instance_writer: false, default: {} | |||
| class_attribute :aggregate_reflections, instance_writer: false, default: {} | |||
| class_attribute :automatic_scope_inversing, instance_writer: false, default: false | |||
| class_attribute :automatically_invert_plural_associations, instance_writer: false, default: false | |||
There was a problem hiding this comment.
@casperisfine #50280 also involves automatic inversion (for delegated type associations).
If it were to be merged, would it make sense to put it behind a configuration flag of its own? Would it be appropriate to make this PR's configuration flag general enough to also apply for both scenarios?
There was a problem hiding this comment.
Ideally anything that could cause backward compatibility issues should be behind a flag to allow to deprecation and selective upgrade, so yes.
As for sharing the same flag, I think it would be weird because the names wouldn't match.
Ref: rails#50284 While having the inverse association configured it generally positive as it avoid some extra queries etc, infering it may break legecy code, as evidenced by how it broke `ActiveStorage::Blob` in rails#50800 As such we can't just enable this behavior immediately, we need to provide and upgrade path for users.
casperisfine
force-pushed
the
infer-inverse-of-deprecation
branch
from
b8166e3
to
7a8e58b
Compare
4 tasks
tagliala
added a commit
to ifad/chronomodel
that referenced
this pull request
Mar 29, 2024
Ref: - rails/rails@7a8e58b - rails/rails#50883 Close #278
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #50284
While having the inverse association configured it generally positive as it avoid some extra queries etc, infering it may break legecy code, as evidenced by how it broke
ActiveStorage::Blobin #50800As such we can't just enable this behavior immediately, we need to provide and upgrade path for users.
At this stage this PR is just a quick draft to discuss how exactly we should gate this. We can just make it a regular framework default, but perhaps emitting a deprecation warning when we would have infered the inverse relation would help users upgrade? cc @rafaelfranca as you generally have great insights on this kind of new framework default.