Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Throw exception when extra attributes are used during an object denor…#19958
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| private$propertyTypeExtractor; | ||
| private$attributesCache =array(); | ||
| private$extraAttributes =array(); |
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.
A service should not have global states like this one. Here it is worst because it creates a bug: if thedenormalize method fails one time because of an extra attribute, all successive calls will throw an error (even if there is no extra attribute) because$this->extraAttributes isn't reseted to an empty array.
Here I think that it's enough to use a local variable to store the list of extra attributes. If you want to make information available in nesteddenormalize calls, you should use the$context parameter.
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.
Humm yes... sorry for that.
| } | ||
| if (($allowedAttributes !==false && !in_array($attribute,$allowedAttributes)) || !$this->isAllowedAttribute($class,$attribute,$format,$context)) { | ||
| if (isset($context['extra_attributes']) && !$context['extra_attributes']) { |
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.
The form component has a similar option calledallow_extra_fields. I propose to rename the key toallow_extra_attributes for consistency.
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 had a lot of hesitation. Updated.
| } | ||
| if (!empty($this->extraAttributes)) { | ||
| thrownewExtraAttributesException(sprintf('Extra attributes are not allowed ("%s" given).',implode('", "',$this->extraAttributes))); |
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.
Extra attributes are not allowed ("%s" are unknown).?
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.
It's more simple with no plurial, but that's good for me. Updated.
4cd4e13 to8444b21Comparedunglas commentedSep 18, 2016
👍, travis error not related. Status: reviewed |
juliendidier commentedSep 18, 2016
Doc PR added. |
| useSymfony\Component\Serializer\Exception\LogicException; | ||
| useSymfony\Component\Serializer\Exception\UnexpectedValueException; | ||
| useSymfony\Component\PropertyInfo\PropertyTypeExtractorInterface; | ||
| useSymfony\Component\PropertyInfo\Type; |
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.
use statements reorganization should reverted.
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.
Alright. done.
8430c61 todca221bCompare| } | ||
| if (!empty($extraAttributes)) { | ||
| thrownewExtraAttributesException(sprintf('Extra attributes are not allowed ("%s" are unknown).',implode('", "',$extraAttributes))); |
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.
Given you have specific exception type, it makes sense to put such logic into the exception, i.e.throw new ExtraAttributesException($extraAttributes) (seeSymfony\Component\DependencyInjection\Exception\ServiceNotFoundException for example).
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 don't really like doing that. And it's not the current behavior for serializer exceptions (like theCircularReferenceException).
However, I pushed this change.
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.
It makes exception API stricter. It is harder to misuse it, it decreases possible inconsistency between messages in different places if an exception has many usages in the code. It makes easier to unit test the exception if the message-forming logic becomes tricky.
d485036 to313fca9Compare313fca9 to565a984Comparejuliendidier commentedSep 22, 2016
Rebased from master to fix builds. |
dunglas commentedNov 11, 2016
I'll merge this PR in 3.3. Thanks@juliendidier |
dunglas commentedDec 5, 2016
Thanks@juliendidier for working on this feature, this is much appreciated. |
… used during an object denor… (juliendidier)This PR was merged into the 3.3-dev branch.Discussion----------[Serializer] Throw exception when extra attributes are used during an object denor…| Q | A || --- | --- || Branch? | "master" || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | no || Tests pass? | yes || Fixed tickets |#19948 || License | MIT || Doc PR | [#6975](symfony/symfony-docs#6975) |I will update the doc if you're ok with this PR.Commits-------565a984 throw exception when extra attributes are used during an object denormalization
juliendidier commentedDec 6, 2016
@dunglas thx, don't forget the documentation ;)symfony/symfony-docs/pull/6975 |
dunglas commentedDec 6, 2016
I cannot merge the doc but I'm sure that the @symfony/team-symfony-docs will handle it soon! |
…juliendidier)This PR was merged into the master branch.Discussion----------[Serializer] documentation for allow_extra_attributesDoc PR for [symfony/symfony#19958](symfony/symfony#19958)Commits-------28dcc94 [Serializer] documentation for allow_extra_attributes
Uh oh!
There was an error while loading.Please reload this page.
I will update the doc if you're ok with this PR.