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

feat: support collect denormalization errors #5170

Merged
merged 2 commits into from Jan 13, 2023

Conversation

natacha-h
Copy link
Contributor

@natacha-h natacha-h commented Nov 7, 2022

Q A
Branch? main
Tickets #4793
License MIT
Doc PR api-platform/docs#1643

Related to symfony/symfony#21239 and symfony/symfony#42502.

Add property collectDenormalizationErrors in Operation (set to false by default), setting this property to truewill enable collecting denormalization type errors to return them as constraint violations.

]
}
"""
And the header "Content-Type" should be equal to "application/ld+json; charset=utf-8"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Maybe this line should be before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we check the response so its ok

@@ -419,7 +419,7 @@ protected function setAttributeValue(object $object, string $attribute, mixed $v
*
* @throws InvalidArgumentException
*/
protected function validateType(string $attribute, Type $type, mixed $value, string $format = null): void
protected function validateType(string $attribute, Type $type, mixed $value, string $format = null, array &$context): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: It's a BC break (add a default value?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't have to be a reference also

@@ -226,7 +226,6 @@ private function normalizeDefaults(array $defaults): array
foreach ($rc->getConstructor()->getParameters() as $param) {
$publicProperties[$param->getName()] = true;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Please revert these changes.

Comment on lines 20 to 22
/**
* DummyWithCollectDenormalizationErrors.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Can be removed.

if (!$exception instanceof NotNormalizableValueException) {
continue;
}
$message = sprintf('The type of the "%s" attribute must be "%s", "%s" given.', $exception->getPath(), implode(', ', $exception->getExpectedTypes() ?? []), $exception->getCurrentType());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to use by default a message like in Symfony Validator componen: This value should be of type {{ type }}. so it would be translated if validator and translation components are dependencies of application?

@alanpoulain alanpoulain marked this pull request as ready for review January 11, 2023 14:53
@alanpoulain alanpoulain force-pushed the issue-4793 branch 4 times, most recently from de28527 to aaf4452 Compare January 11, 2023 15:18
@alanpoulain
Copy link
Member

@norkunas and @lyrixx (because you made the related PR in Symfony 😉), it would be great if you could review this PR!

@alanpoulain alanpoulain changed the title feat: support collect dernormalization errors feat: support collect denormalization errors Jan 11, 2023
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job!

Copy link
Contributor

@norkunas norkunas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🙂

@soyuka soyuka merged commit 10da65f into api-platform:main Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants