Skip to content
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

[Serializer] Normalize constraint violation parameters #29130

Merged
merged 1 commit into from Mar 17, 2019

Conversation

@ogizanagi
Copy link
Member

ogizanagi commented Nov 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets N/A
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?

@ogizanagi ogizanagi requested a review from dunglas as a code owner Nov 7, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:constraint_violation_normalizer_parameters branch from 9519027 to 4816f25 Nov 7, 2018

@ogizanagi ogizanagi removed the Deprecation label Nov 7, 2018

@dunglas

dunglas approved these changes Nov 7, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 8, 2018

@ogizanagi

This comment has been minimized.

Copy link
Member Author

ogizanagi commented Nov 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

This comment has been minimized.

Copy link
Member

xabbuh commented Nov 8, 2018

it may be better to make this behaviour opt-in

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Nov 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.

@dunglas

dunglas approved these changes Nov 8, 2018

@@ -49,6 +49,7 @@ public function normalize($object, $format = null, array $context = array())
$violationEntry = array(
'propertyPath' => $propertyPath,
'title' => $violation->getMessage(),
'parameters' => $violation->getParameters(),

This comment has been minimized.

@dunglas

dunglas Nov 8, 2018

Member

We should only add this key if at least one parameter exists:

if ($parameters = $violation->getParameters()) {
    $violationEntry['parameters'] = $parameters;
}

This comment has been minimized.

@ogizanagi

ogizanagi Nov 9, 2018

Author Member

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.

@lyrixx

lyrixx approved these changes Nov 9, 2018

@dunglas
Copy link
Member

dunglas left a comment

Let's merge it as is. Can you rebase please?

@ogizanagi ogizanagi force-pushed the ogizanagi:constraint_violation_normalizer_parameters branch from 4816f25 to 32c90eb Feb 24, 2019

@ogizanagi

This comment has been minimized.

Copy link
Member Author

ogizanagi commented Feb 24, 2019

Now rebased :)

@lyrixx

lyrixx approved these changes Mar 13, 2019

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Mar 16, 2019

this one is OK too. let's merge it. /cc @dunglas (easy pick)

@fabpot

fabpot approved these changes Mar 17, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 17, 2019

Thank you @ogizanagi.

@fabpot fabpot merged commit 32c90eb into symfony:master Mar 17, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 17, 2019

feature #29130 [Serializer] Normalize constraint violation parameters…
… (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     <!-- see https://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

@ogizanagi ogizanagi deleted the ogizanagi:constraint_violation_normalizer_parameters branch Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.