Follow-up to #10776 #19077
Merged
Follow-up to #10776 #19077
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
| 'ActiveModel::AttributeAssignment::UnknownAttributeError' | ||
| ) | ||
| # Raised when unknown attributes are supplied via mass assignment. | ||
| class UnknownAttributeError < ActiveModel::UnknownAttributeError |
There was a problem hiding this comment.
Rather than subclassing, let's just alias. There's no reason to have two separate exception classes.
UnknownAttributeError = ActiveModel::UnknownAttributeError
There was a problem hiding this comment.
Aliasing is a little confusing because you would expect error.class.to_s to return ActiveRecord exception but it is not. It will cause confusion at least in all bug notification services. I vote for deprecation.
There was a problem hiding this comment.
@bogdan : We decided to finally alias the constant as there was no big deal ; furthermore, we are already raising errors with constants nested under ActiveModel so having it in the output of error.class.to_s is not that unexpected.
The name `ActiveModel::AttributeAssignment::UnknownAttributeError` is too implementation specific so let's move the constant directly under the ActiveModel namespace. Also since this constant used to be under the ActiveRecord namespace, to make the upgrade path easier, let's avoid raising the former constant when we deal with this error on the Active Record side.
sgrif
added a commit
that referenced
this pull request
Mar 2, 2015
Move `UnknownAttributeError` to a more sane namespace
rthbound
added a commit
to rthbound/rails
that referenced
this pull request
Feb 9, 2016
- Corrects an incorrect exception message when using accepts_nested_attributes_for - Removes rescue/reraise behavior introduced in rails#19077 - Adds has_many & has_one, nested_attributes test case specifying the message that should be conveyed with an exception raised because one of the nested attributes provided is unknown
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.
Hello,
This pull request is a follow up to #10776.
The name
ActiveModel::AttributeAssignment::UnknownAttributeErroris too implementation specific so let's move the constant directly under the ActiveModel namespace.Also since this constant used to be under the ActiveRecord namespace, to make the upgrade path easier, let's avoid raising the former constant when we deal with this error on the Active Record side.
I don't know whether monkey-patching
_assign_attributeis the right solution to raise the correct constant so let me know if I should change it.Have a nice day.