Skip to content

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
* 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.

@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?

@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