Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] Add support to only check first element of $data in ArrayDenormalizer#45337
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedFeb 7, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
cea8b69 to5329828Compare$data in `A…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| } | ||
| if ($this->checkFirstElement) { | ||
| $data = 0 === \count($data) ? null : reset($data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
nullas data, so they might throw or returnfalse. - If the original
$datais an empty array,ArrayDenormalizerwould not calldenormalize()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 activeand$data is an empty array, we should returntrue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 insupportsDenormalization has a use case that can't be solved in thedenormalize.
And incase we might want to introduce BC for 7.0 we can consider this option like already discussed in the issue itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
5329828 toac6828fCompareAssuming that all elements in the array/list are of the same type is a strong assumption, and it's not guaranteed at all. |
ac6828f to1dc335bCompare@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 theUuidNormalizer of ApiPlatform checks if Another option is to alter that specific denormalizer to move the datatype check to the |
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? |
| } | ||
| if ($this->checkFirstElement) { | ||
| $data = 0 === \count($data) ? null : reset($data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
| } | ||
| if ($this->checkFirstElement) { | ||
| $data = \is_array($data) ? $data : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here, ifcheckFirstElement is true and we are giving anint for example, we will convert the data to an empty array, which is quite odd to me.
What about returning false in that case?
Closing given there's no more activity here. Please feel free to resume the work, anyone. |
Uh oh!
There was an error while loading.Please reload this page.
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: