Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Serializer] Add COLLECT_EXTRA_ATTRIBUTES_ERRORS and full deserialization path #46654

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

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
Loading
from

Conversation

NorthBlue333
Copy link

@NorthBlue333 NorthBlue333 commented Jun 12, 2022

Q A
Branch? 6.2
Bug fix? yes/no
New feature? yes
Deprecations? yes
Tickets Fix #46283, Fix #46284
License MIT
Doc PR symfony/symfony-docs#16872

The PartialDenormalizationException is used as unexpected extra attributes are, IMO, part of the denormalization exception.
This enables executing:

  try {
       $dto = $serializer->deserialize($request->getContent(), MyDto::class, 'json', [
            DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
            DenormalizerInterface::COLLECT_EXTRA_ATTRIBUTES_ERRORS => true,
            DenormalizerInterface::ALLOW_EXTRA_ATTRIBUTES => false,
        ]);
    } catch (PartialDenormalizationException $e) {
        $violations = new ConstraintViolationList();
        /** @var NotNormalizableValueException */
        foreach ($e->getErrors() as $exception) {
            $message = sprintf('The type must be one of "%s" ("%s" given).', implode(', ', $exception->getExpectedTypes()), $exception->getCurrentType());
            $parameters = [];
            if ($exception->canUseMessageForUser()) {
                $parameters['hint'] = $exception->getMessage();
            }
            $violations->add(new ConstraintViolation($message, '', $parameters, null, $exception->getPath(), null));
        };
        if (null !== $extraAttributesException = $e->getExtraAttributesError()) {
            foreach ($extraAttributesException->getExtraAttributes() as $extraAttribute) {
                $violations->add(new ConstraintViolation('This attribute is not allowed.', '', [], null, $extraAttribute, null));
            };
        }

        return $this->json($violations, 400);
    }

@NorthBlue333
Copy link
Author

Would ideally need #45861 first.
I can make the necessary changes if needed.

@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch 2 times, most recently from e285d19 to 7b8a73a Compare June 13, 2022 08:23
@NorthBlue333
Copy link
Author

NorthBlue333 commented Jun 13, 2022

Seems I cannot reproduce the failing tests in local in 8.2? Some help will be appreciated for this :)

@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch 2 times, most recently from 3328a3f to dac569d Compare June 13, 2022 11:18
@carsonbot
Copy link

Hey!

I think @mtarld has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch from dac569d to 58fc21e Compare October 8, 2022 13:09
@NorthBlue333
Copy link
Author

Any news for this?

@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch from 58fc21e to a202da2 Compare October 8, 2022 13:12
@NorthBlue333 NorthBlue333 requested a review from chalasr as a code owner October 8, 2022 13:12
@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch 2 times, most recently from daebc6d to adf10e3 Compare October 8, 2022 13:17
UPGRADE-6.2.md Outdated Show resolved Hide resolved
* returned. Otherwise, the concatenation of the two paths is returned,
* separated by a dot (".").
*/
public static function append(string $basePath, string $subPath): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update the PropertyPathInterface by adding a new @method phpdoc attribute?

Copy link
Member

Choose a reason for hiding this comment

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

As this is a static method, I don't think it makes sense to add in in the PropertyPathInterface.

src/Symfony/Component/Console/Helper/Table.php Outdated Show resolved Hide resolved
src/Symfony/Component/PropertyAccess/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Serializer/Tests/SerializerTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/Serializer/Tests/SerializerTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/Serializer/Tests/SerializerTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/Serializer/Util/PropertyPath.php Outdated Show resolved Hide resolved
src/Symfony/Component/PropertyAccess/PropertyPath.php Outdated Show resolved Hide resolved
@NorthBlue333
Copy link
Author

I think I am done with the changes here. 👍

@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch 2 times, most recently from b01db33 to 389804b Compare October 10, 2022 17:18
@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch from 389804b to 536fb94 Compare October 11, 2022 12:55
@NorthBlue333
Copy link
Author

Done 👍

Copy link
Contributor

@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

LGTM then, thank you @NorthBlue333!

public function getErrors(): array
{
return $this->errors;
return $this->getNotNormalizableValueErrors();
Copy link
Member

Choose a reason for hiding this comment

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

the method should throw a runtime deprecation
can you remind me why we deprecated the method?
deprecating always comes with a cost so it needs a good enough reason

Copy link
Author

Choose a reason for hiding this comment

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

Well this was because it might be used. Should I completely remove it / throw an exception instead of deprecating it?

src/Symfony/Component/Serializer/Serializer.php Outdated Show resolved Hide resolved
@NorthBlue333 NorthBlue333 force-pushed the feature/ticket_46283_ticket_46284 branch from 536fb94 to 2cda4a9 Compare October 23, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.