-
Notifications
You must be signed in to change notification settings - Fork 879
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
Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468
Fixes Rails 7.1 raise_on_assign_to_attr_readonly #1468
Conversation
22b411a
to
af8073b
Compare
af8073b
to
bddd0b1
Compare
@@ -92,7 +92,7 @@ def init_unversioned_attrs(attrs, model) | |||
# @api private | |||
def reify_attribute(k, v, model, version) | |||
if model.has_attribute?(k) | |||
model[k.to_sym] = v | |||
model[k.to_sym] = v unless model.class.readonly_attribute?(k.to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty dangerous since it silently throws away assignments of readonly attributes without the developer knowing about it. This is the exact problem that config.active_record.raise_on_assign_to_attr_readonly was looking to address.
I see some viable options:
- We consider paper_trail's reify operation as a "dangerous" operation that automatically disable
attr_readonly
declarations (somehow) and forces the attributes to be rewritten. - We take the stance you can't have both worlds. As in, you can't reify a record if it's going to set a readonly attribute. This is basically not changing anything.
- We only set the value iff it's a different value. This allows the reify to work in some cases and fail (loudly) in others. This seems a little odd to me, but still, it's an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only set the value iff it's a different value. This allows the reify to work in some cases and fail (loudly) in others. This seems a little odd to me, but still, it's an option.
This is the approach in #1473. Any concerns?
Should we close this in favor of #1473? |
Yes, I agree. |
Fixes #1467
Check the following boxes:
master
(if not - rebase it).code introduces user-observable changes.
and description in grammatically correct, complete sentences.
I manually tested this PR with these commands: