Skip to content

[Serializer] fix denormalization of basic property-types in XML and CSV #33850

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 2, 2020
Merged

[Serializer] fix denormalization of basic property-types in XML and CSV #33850

merged 1 commit into from
Sep 2, 2020

Conversation

mkrauser
Copy link
Contributor

@mkrauser mkrauser commented Oct 4, 2019 •
edited
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #33849
License MIT
Doc PR

Like I explained in the Issue, the serializer cannot de-serialize non-string basic properties (int, float, bool). This PR add's some logic to cast to the expected types.

Similar logic is already present in the XmlUtils-Class of the Config-Component

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 4, 2019
nicolas-grekas
nicolas-grekas reviewed Oct 7, 2019
View reviewed changes
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated
$data = (int) $data;
} elseif ('NaN' === $data) {
return NAN;
} elseif ('INF') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inf too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also doesn't hurt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do it then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is broken (should be 'INF' === $data). Also, a switch will perform better than a succession of is/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is implemented using switch now

warslett
warslett reviewed Oct 21, 2019
View reviewed changes
Copy link
Contributor

@warslett warslett left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution will not solve the problem in the API Platform because API Platform detects types differently. If we can extract this logic into a protected function something like protected function castStringToBuiltInType(string $builtInType, string $value) it will make this easier to fix in API Platform without further duplication.

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated
$data = (int) $data;
} elseif ('NaN' === $data) {
return NAN;
} elseif ('INF') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also doesn't hurt

@warslett warslett mentioned this pull request Oct 22, 2019
Closed
@mkrauser mkrauser force-pushed the 3.4 branch 2 times, most recently from a6157bd to 98105f5 Compare October 24, 2019 12:19
nicolas-grekas
nicolas-grekas reviewed Nov 5, 2019
View reviewed changes
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated
'context' => $context,
'ignored' => $this->ignoredAttributes,
'camelized' => $this->camelizedAttributes,
]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change looks unrelated

@mkrauser mkrauser requested a review from dunglas as a code owner November 12, 2019 19:06
mkrauser
mkrauser commented Nov 12, 2019
View reviewed changes
Copy link
Contributor Author

@mkrauser mkrauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution will not solve the problem in the API Platform because API Platform detects types differently. If we can extract this logic into a protected function something like protected function castStringToBuiltInType(string $builtInType, string $value) it will make this easier to fix in API Platform without further duplication.

@nicolas-grekas Would that be ok?

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated
break;
case Type::BUILTIN_TYPE_FLOAT:
if (is_numeric($data)) {
return '0x' === $data[0].$data[1] ? hexdec($data) : (float) $data;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, if we should include hex decoding here. XSD does allow hex for numeric types

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XSD does allow hex for numeric types

Source? I can't find anything about that in the XML Schema spec: https://www.w3.org/TR/xmlschema-2

dunglas
dunglas requested changes Jan 22, 2020
View reviewed changes
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php Outdated
$data = (int) $data;
} elseif ('NaN' === $data) {
return NAN;
} elseif ('INF') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is broken (should be 'INF' === $data). Also, a switch will perform better than a succession of is/else.

@mkrauser mkrauser force-pushed the 3.4 branch 2 times, most recently from bef9507 to 7e4fcc7 Compare January 22, 2020 15:32
@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

@teohhanhui @dunglas @nicolas-grekas Can you review again this PR?

dunglas
dunglas approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that this logic should be extracted in a dedicated class but it's not a blocker.

@fabpot
Copy link
Member

fabpot commented Sep 2, 2020

Merging as new feature though.

@fabpot fabpot changed the base branch from 3.4 to master September 2, 2020 05:44
@fabpot
Copy link
Member

fabpot commented Sep 2, 2020

Thank you @mkrauser.

@fabpot fabpot mentioned this pull request Sep 2, 2020
Closed
@fabpot fabpot merged commit 4753e4d into symfony:master Sep 2, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@lyrixx lyrixx mentioned this pull request Nov 12, 2020
Closed
@alanpoulain alanpoulain mentioned this pull request Mar 19, 2021
Merged
@mkrauser mkrauser mentioned this pull request Jan 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teohhanhui teohhanhui teohhanhui left review comments

@warslett warslett warslett left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Bug Serializer Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.

Serializer does not map basic non-string types correctly in XML and CSV
7 participants
@mkrauser @fabpot @dunglas @nicolas-grekas @teohhanhui @warslett @carsonbot