[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

CipherdevNL · Feb 7, 2022

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #20837
License	MIT
Doc PR	symfony/symfony-docs#16489

Based on an old discussion an implementation is made for option three/ four discussed in the ticket. With option four breaking changes will be prevented, so checking the first element will be an opt-in "feature".

To enable the check first element the following config can be used:

    Symfony\Component\Serializer\Normalizer\ArrayDenormalizer:
        class: Symfony\Component\Serializer\Normalizer\ArrayDenormalizer
        arguments: [true]

carsonbot · Feb 7, 2022

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

nicolas-grekas

As this is a new feature, please rebase to target 6.1.
Please also add a changelog entry in the component.

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

derrabus

Making the new behavior opt-in is a good way to avoid a BC break. I wonder if we should already deprecate not opting-in to this behavior. That way, we could remove the old odd behavior in 7.0.

derrabus · Feb 8, 2022

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

+        }
+
+        if ($this->checkFirstElement) {
+            $data = 0 === \count($data) ? null : reset($data);


This basically means that we would call the nested denormalizers with $data === null which is as odd for two reasons:

Denormalizers are not used to getting called with null as data, so they might throw or return false.

If the original $data is an empty array, ArrayDenormalizer would not call denormalize() on the nested denormalizers at all.

Since we're attempting to remove odd behevior, I'd like to get it right this time. 😉

Suggestion: If the new mode is active and $data is an empty array, we should return true.

Totally agree of getting it right directly :). You're right, passing null might indeed be odd for denomalizers and may introduce other unwanted issues. And incase $data is empty this denormalizer will basically do nothing.

The only side effect would be, when an unsupported type is given and [] is passed by it will not throw an error. But when data is given (e.g. ["x", "y", "z"]) it will fail because it couldn't find a denormalizer for the type.

Think this side effect is only preventable by deprecating/ removing $data from the method signature, so we don't have the issue of "what to pass when the array is empty?". But I don't have a clear picture if $data in supportsDenormalization has a use case that can't be solved in the denormalize.

And incase we might want to introduce BC for 7.0 we can consider this option like already discussed in the issue itself.

I totally agree with @derrabus there.
Actually, if the array is empty, there is no need to check the first element as it doesn't exist.
Therefore, why not just resume the "default" behavior when the array is empty?

dunglas · Feb 9, 2022

Assuming that all elements in the array/list are of the same type is a strong assumption, and it's not guaranteed at all.

…yDenormalizer`

CipherdevNL · Feb 9, 2022

@dunglas , I agree its a strong assumption, but i think it would be a valid assumption. As far as I know it's not possible to denormalize an array with mixed types besides creating a custom denormalizer that will act as a middleware.

We can check all elements in the array, but when considering performance I don't think it would be a good option.

Think the question is more if we want to support arrays of mixed types, or that the data inside the array must always be of one type. Personally I would prefer the second because it "enforce" good programming practices, but I don't have any experience with maintaining OSS / libraries, so it could be a bluntly choose from that point of view.

Personally I came across this issue when using the ApiPlatform library and using uuids inside an array. Apparently the UuidNormalizer of ApiPlatform checks if $data is a string, thus resulting in not finding the correct denormalizer for arrays of uuids.

Another option is to alter that specific denormalizer to move the datatype check to the denormalize method. But then the same question as in the mentioned issue arises, what is the benefit of having $data inside supportsDenormalization if it shouldn't be used for data checking?

derrabus · Feb 14, 2022

I have layed out the different options that we have in #20837 (comment) already. This PR implements option 3. What would be your favorite, @dunglas?

mtarld · Oct 7, 2022

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

+        }
+
+        if ($this->checkFirstElement) {
+            $data = 0 === \count($data) ? null : reset($data);


I totally agree with @derrabus there.
Actually, if the array is empty, there is no need to check the first element as it doesn't exist.
Therefore, why not just resume the "default" behavior when the array is empty?

mtarld · Oct 7, 2022

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php

+        }
+
+        if ($this->checkFirstElement) {
+            $data = \is_array($data) ? $data : [];


Here, if checkFirstElement is true and we are giving an int for example, we will convert the data to an empty array, which is quite odd to me.
What about returning false in that case?

chalasr · Mar 29, 2025

Closing given there's no more activity here. Please feel free to resume the work, anyone.

CipherdevNL requested a review from dunglas as a code owner February 7, 2022 11:15

carsonbot added Status: Needs Review Bug Serializer labels Feb 7, 2022

carsonbot added this to the 5.4 milestone Feb 7, 2022

CipherdevNL force-pushed the 20837-array-denormalizer-supports-array branch 4 times, most recently from cea8b69 to 5329828 Compare February 7, 2022 13:01

nicolas-grekas changed the title ~~[Serializer] Add support to only check first element of $data in `A…~~ [Serializer] Add support to only check first element of $data in ArrayDenormalizer Feb 7, 2022

nicolas-grekas reviewed Feb 8, 2022

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php Outdated Show resolved Hide resolved

src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php Show resolved Hide resolved

derrabus reviewed Feb 8, 2022

View reviewed changes

derrabus modified the milestones: 5.4, 6.1 Feb 8, 2022

derrabus added Feature and removed Bug labels Feb 8, 2022

derrabus changed the base branch from 5.4 to 6.1 February 8, 2022 14:54

derrabus requested review from chalasr, wouterj, xabbuh and yceruto as code owners February 8, 2022 14:54

derrabus changed the base branch from 6.1 to 5.4 February 8, 2022 14:55

derrabus changed the base branch from 5.4 to 6.1 February 8, 2022 15:00

derrabus removed request for wouterj, xabbuh, yceruto and chalasr February 8, 2022 15:01

CipherdevNL force-pushed the 20837-array-denormalizer-supports-array branch from 5329828 to ac6828f Compare February 9, 2022 11:16

CipherdevNL mentioned this pull request Feb 9, 2022

[Serializer] Add documentation for new feature in ArrayDenormalizer of Symfony Serializer symfony/symfony-docs#16489

Open

[Serializer] Add the ability to only check the first element in `Arra…

1dc335b

…yDenormalizer`

CipherdevNL force-pushed the 20837-array-denormalizer-supports-array branch from ac6828f to 1dc335b Compare February 9, 2022 12:39

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

mtarld requested changes Oct 7, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 7, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

chalasr closed this Mar 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

CipherdevNL commented Feb 7, 2022 •

edited

Loading

carsonbot commented Feb 7, 2022

nicolas-grekas left a comment

derrabus left a comment

derrabus Feb 8, 2022

CipherdevNL Feb 8, 2022

mtarld Oct 7, 2022

dunglas commented Feb 9, 2022

CipherdevNL commented Feb 9, 2022

derrabus commented Feb 14, 2022

mtarld Oct 7, 2022

mtarld Oct 7, 2022

chalasr commented Mar 29, 2025

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

[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

Conversation

CipherdevNL commented Feb 7, 2022 • edited Loading

carsonbot commented Feb 7, 2022

nicolas-grekas left a comment

Choose a reason for hiding this comment

derrabus left a comment

Choose a reason for hiding this comment

derrabus Feb 8, 2022

Choose a reason for hiding this comment

CipherdevNL Feb 8, 2022

Choose a reason for hiding this comment

mtarld Oct 7, 2022

Choose a reason for hiding this comment

dunglas commented Feb 9, 2022

CipherdevNL commented Feb 9, 2022

derrabus commented Feb 14, 2022

mtarld Oct 7, 2022

Choose a reason for hiding this comment

mtarld Oct 7, 2022

Choose a reason for hiding this comment

chalasr commented Mar 29, 2025

CipherdevNL commented Feb 7, 2022 •

edited

Loading