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] Normalize constraint violation parameters#29130
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
[Serializer] Normalize constraint violation parameters#29130
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ogizanagi commentedNov 8, 2018
Note: adding this by default might break CI testing API endpoint responses, but I don't see much more reasons not to do so. So, better keep adding by default, or add a context option for this? |
xabbuh commentedNov 8, 2018
it may be better to make this behaviour opt-in |
dunglas commentedNov 8, 2018
We don't guarantee that the output of the normalizers will never change. Especially, we don't guarantee that will not add fields. I'm in favor of adding it by default, but only if at least one parameter exists. |
| $violationEntry =array( | ||
| 'propertyPath' =>$propertyPath, | ||
| 'title' =>$violation->getMessage(), | ||
| 'parameters' =>$violation->getParameters(), |
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 should only add this key if at least one parameter exists:
if ($parameters =$violation->getParameters()) {$violationEntry['parameters'] =$parameters;}
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.
May I ask what is the motivation behind this?
From my experience, this would just introduce additional complexity/confusion on the client side in the way would be consumed the data structure, only to save a few bytes.
dunglas left a comment
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.
Let's merge it as is. Can you rebase please?
ogizanagi commentedFeb 24, 2019
Now rebased :) |
lyrixx commentedMar 16, 2019
this one is OK too. let's merge it. /cc@dunglas (easy pick) |
fabpot commentedMar 17, 2019
Thank you@ogizanagi. |
… (ogizanagi)This PR was merged into the 4.3-dev branch.Discussion----------[Serializer] Normalize constraint violation parameters| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/A?Adding violation constraints' parameters to the normalized data, as these are valuable for an API client.I used `parameters` for now, as it's the name used in `ConstraintViolationInterface::getParameters()`, but what about `placeholders` or `context`?Commits-------32c90eb [Serializer] Normalize constraint violation parameters
…eters (ogizanagi)This PR was merged into the 4.4 branch.Discussion----------[Validator] Add compared value path to violation parameters| Q | A| ------------- | ---| Branch? | 4.4 <!-- see below -->| Bug fix? | no| New feature? | yes <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AWhile it's not really useful to use as a placeholder in the violation message template (compared to hard-coding it into the message. Nor it is really user-friendly),it becomes handy in conjunction with#29130 for any mapping logic on client-side.Commits-------2da226a [Validator] Add compared value path to violation parameters
Uh oh!
There was an error while loading.Please reload this page.
Adding violation constraints' parameters to the normalized data, as these are valuable for an API client.
I used
parametersfor now, as it's the name used inConstraintViolationInterface::getParameters(), but what aboutplaceholdersorcontext?