[Serializer] Return null and handle as InvalidArgumentException instead of ValueError if the data doesn't belong to a backed enum

[alli83]

Q	A
Branch?	from 5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #46977 [Serializer] Collect deserialization errors on enum cause TypeError
License	MIT
Doc PR

BackedEnumNormalizer : Return null and handle as InvalidArgumentException instead of ValueError if the data doesn't belong to a backed enum

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[chalasr]

/cc @alexandre-daubois as you are the author of that normalizer

[fabpot]

Thank you @alli83.

[issei-m]

This pull-request doesn't seem to fix the targeted issue. AFAIK, the issue author (@elliotbruneel) would've wanted BackedEnumNormalizer to throw the PartialDenormalizationException in case a value not corresponding to the enum is attempted to be de-normalized.

While I understood the following argument of @alli83,

By just trying to collect the error we do not prevent the object from being instantiated. But this action is bound to fail because we already know that the data is not valid (checked by the BackedEnumNormalizer).

BackedEnumNormalizer can throw the PartialDenormalizationException as long as the DTO doesn't have the constructor but only state their properties like:

<?php

class SerializerIssueReport extends \PHPUnit\Framework\TestCase
{
    public function testCollectErrorsOnEnum()
    {
        $serializer = new Symfony\Component\Serializer\Serializer(
            [
                new \Symfony\Component\Serializer\Normalizer\BackedEnumNormalizer(),
                new \Symfony\Component\Serializer\Normalizer\ObjectNormalizer(),
            ],
            ['json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder()]
        );

        $this->expectException(\Symfony\Component\Serializer\Exception\PartialDenormalizationException::class);

        $serializer->deserialize('{"test": "invalid"}', DummyObject2::class, 'json', [
            \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true
        ]);
    }
}

// Original one, for this class the pull-req is legit.
class DummyObject {
    public function __construct(public TestEnum $test) {}
}

// For this, PartialDenormalizationException can be thrown.
class DummyObject2 {
    public TestEnum $test;
}

enum TestEnum: String
{
    case Test = "test";
}

PartialDenormalizationException is preferred to InvalidArgumentException under the situation we can use it.

[alli83]

Indeed, we could distinguish the type of exception thrown depending on whether there is a constructor or not.

• if constructor invalid, the object being not possible to instantiate we could still throw the InvalidArgumentException
• but throw a NotNormalizableValueException in the case you present above and would be collected as a PartialDenormalizationException.

maybe when calling the instantiateObject from the AbstractNormalizer class we could easily add to the context to identify the presence of a constructor and direct the type of exception to throw ?

thanks for your feedback 🙏

[issei-m]

@alli83 Thank you for clarification. Could you kindly open a pull-req for it?

[alli83]

@issei-m for sure. I will also take into account this PR https://github.com/symfony/symfony/pull/48821

[TerjeBr]

Hi @alli83 Are you going to make a new PR to correct this? As it is now, we cannot have enums in our Dto's because we get InvalidArgumentException instead of PartialDenormalizationException that is expected when we specify collect_denormalization_errors

[alli83]

@TerjeBr PR https://github.com/symfony/symfony/pull/48821 doesn't solve your issue ? I can have a look at it :) sorry for the late reply

[TerjeBr]

No. My project is locked to symfony 5.4 and PR https://github.com/symfony/symfony/pull/48821 is a symfony 6.3 thing.

The thing is that this PR undid the feature done by https://github.com/symfony/symfony/pull/42502

[alli83]

indeed. I'm glad to have a look at it then for 5.4 !

[TerjeBr]

Thanks

[TerjeBr]

To provide some context where I come from, I first viewed https://github.com/symfony/symfony/pull/43047#issuecomment-921077295 Then I was happy that may be https://github.com/symfony/symfony/pull/40830 + https://github.com/symfony/symfony/pull/42502 would be a solution to my problem. But then I discovered that it was not working any more because of this change.

(Just to give you the background.)

[alli83]

Would the added test case correspond to your case in https://github.com/symfony/symfony/pull/50192 ?