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 5.x 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

I understand and after fixing the different returns this PR is more of an improvement than a bugfix.

It might be possible by checking if a denormalizer is available before trying to denormalize the scalar value, as done in this implementation.

How should I proceed:

• Keeping this PR for the improvement so that we kept the history and creating a new one for the bugfix (needs to update labels/milestone) ?
• Duplicate this branch to create another PR for the feature and forced push the bugfix only to this PR ?
• Close this PR and create two new ones, mentioning this one into the improvement for reference ?

Keeping this PR for the improvement so that we kept the history and creating a new one for the bugfix

I think, that's the best way.

Also if we fix the UnwrappingDenormalizer I think that its priority needs to be reviewed, it's the only one in symfony to be > 0. The highest is -890 right now, this one could be at -700, IMO it'd be more consistent then using 1000. I'm not sure how these priorities are decided though.

Symfony Container Services Tagged with "serializer.normalizer" Tag
==================================================================

 ----------------------------------------------------------- ---------- -----------------------------------------------------------------------------------------------------
  Service ID                                                  priority   Class name
 ----------------------------------------------------------- ---------- -----------------------------------------------------------------------------------------------------
  serializer.denormalizer.unwrapping                          1000       Symfony\Component\Serializer\Normalizer\UnwrappingDenormalizer
  serializer.normalizer.object                                -1000      Symfony\Component\Serializer\Normalizer\ObjectNormalizer
  serializer.normalizer.datetimezone                          -915       Symfony\Component\Serializer\Normalizer\DateTimeZoneNormalizer
  serializer.normalizer.dateinterval                          -915       Symfony\Component\Serializer\Normalizer\DateIntervalNormalizer
  serializer.normalizer.data_uri                              -920       Symfony\Component\Serializ er\Normalizer\DataUriNormalizer
  serializer.normalizer.datetime                              -910       Symfony\Component\Serializer\Normalizer\DateTimeNormalizer
  serializer.normalizer.json_serializable                     -900       Symfony\Component\Serializer\Normalizer\JsonSerializableNormalizer
  serializer.normalizer.problem                               -890       Symfony\Component\Serializer\Normalizer\ProblemNormalizer
  serializer.denormalizer.array                               -990       Symfony\Component\Serializer\Normalizer\ArrayDenormalizer
  serializer.normalizer.constraint_violation_list             -915       Symfony\Component\Serializer\Normalizer\ConstraintViolationListNormalizer
 ----------------------------------------------------------- ---------- -----------------------------------------------------------------------------------------------------

My guess is that the priority is so high because this denormalizer does not rely on the type and must always be called first.
I didn't check but assuming the default priority for any custom denormalizer is 0 then the UnwrappingDenormlizer must indeed have a priority > 0.

The bug that was reported occurred because scalar values are handled directly in the serializer and therefore it has the priority over the UnwrappingDenormalizer.

Little update, while trying to remove the duplication between the ScalarDenormalizer and the AbstractObjectNormalizer I realized there is a few small bugs here and there that makes it difficult so I was wondering if it might not be beneficial to solve them first and delay this feature until then.

First, the scalar denormalization is done directly in the Serializer and use a relatively naive approach: it does not handle the specific cases dealt with in the AbstractObjectNormalizer.
For example, deserializing '3' into a float type will fail but when decoding from JSON it should be accepted because '3' === json_encode(3.0) (there is other cases for XML and CSV).

Second, the AbstractObjectNormalizer only delegate the denormalization process when dealing with properties which types are object or object[]. For all other cases (builtin types) it uses the is_<type>() PHP function to validate it and if the data is valid returns it.
Which means it handles scalars and arrays internally, it's not a problem for scalar denormalization since it does a better job than the Serializer but it does not validate arrays properly.
If a property is of type int[] and the data to the denormalize is ['string'] the AbstractObjectNormalizer will consider it valid (it's an array) and will try to set the value onto the object.

To summarize there is two issues:

• The scalar denormalization in the Serializer is not smart enough
• The AbstractObjectNormalizer does not properly validate arrays

What we could do is:

1. A PR to fix the scalar denormalization bugs: duplicate the logic from the AbstractObjectNormalizer into the Serializer (target 5.1)
2. A PR to make the AbstractObjectNormalizer delegate all denormalization to the serializer (target 5.1 if consider a hotfix but do not deprecate using it without the delegation, 5.x if consider a feature and then add deprecation)
3. Update this PR to introduce the ScalarDenormalizer and deprecate the scalar denormalization within the Serializer (also deprecate using the fallback in the AbstractObjectNormalizer if step 2. was done as a bugfix)

But the step 2. would trigger two BC breaks:

• AbstractObjectNormalizer will not be usable without setting the serializer property, which is only required during denormalization to handle objects currently
• Denormalizing arrays without registering the ArrayDenormalizer won't work anymore

To void this we need to find a way to fallback to the current behavior if we can't denormalize because:

• The serializer property is not an instance of DenormalizerInterface
• There was nos denormalizer available: ArrayDenormalizer not register or for the builtin types iterable, resource and callable.

It should be doable by adding an new exception like NoDenormalizerFoundForValueException extending NotNormalizableValueException.
The problem with this solution is that we ends up duplicating the code responsible to handle scalar values 3 times.

I would like to validate how you would suggest to process before starting anything on my side.
Should I create a dedicated issue to talk about this ? If so what kind of issue should it be ?

Idk if this is relevant but you should also consider #30818 and the work around it.