Unbreak update_column/update_columns for the primary key attribute. #8053
Conversation
| @@ -224,11 +224,11 @@ def update_columns(attributes) | |||
| verify_readonly_attribute(key.to_s) | |||
| end | |||
|
|
|||
| self.class.where(self.class.primary_key => id).update_all(attributes) == 1 | |||
There was a problem hiding this comment.
This change is modifying the return value of update_column. We should probably add a test to ensure it returns a boolean.
There was a problem hiding this comment.
Yeah, I just noticed this. Should I go ahead and fix? Any preference between amend/force push vs. multiple commits in a feature branch?
There was a problem hiding this comment.
Generally we prefer commits to be squashed, so amend/force push.
There was a problem hiding this comment.
Alright, pushed an amended commit: henrik@cd0efa5
Just tested that it returns true. Good enough? Seems the return value is undocumented anyway, and I'm not sure what it'd take to make it return false. Multiple records with the same id or for the query to somehow update nothing.
|
@henrik I think it's "the query updating nothing", in any case, it's just better to not change the result value to avoid possible breakage (and it'd be weird to return the attributes enumerable anyways). I made small comments, but overall it looks good, thanks! |
|
@henrik thanks. Github says it cannot be merged anymore, can you rebase it from master please? It's probably the changelog. |
|
And since you're touching the changelog, how about using backticks like this instead of \ escaping: " |
|
Do you mean this? |
|
Yes. |
|
Good idea - used backticks, rebased against master. |
|
Oops, a merge commit snuck in there. Fixing. |
Didn't work before because it updated the model-in-memory first, so the DB query couldn't find the record.
|
And there we go. |
Unbreak update_column/update_columns for the primary key attribute.
|
@henrik awesome, thanks. |
|
Wow, that was faster and less ceremony than expected. Thanks a lot, guys! |
|
Thank you |
Unbreak update_column/update_columns for the primary key attribute. Conflicts: activerecord/CHANGELOG.md activerecord/lib/active_record/persistence.rb activerecord/test/cases/persistence_test.rb
Didn't work before because it updated the model-in-memory first, so the DB query couldn't find the record.