Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged
fabpot merged 1 commit intosymfony:masterfrommkrauser:3.4
Sep 2, 2020

Conversation

@mkrauser
Copy link
Contributor

@mkrausermkrauser commentedOct 4, 2019•
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#33849
LicenseMIT
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 theXmlUtils-Class of the Config-Component

Devristo and gpenverne reacted with thumbs up emoji
$data = (int)$data;
}elseif ('NaN' ===$data) {
returnNAN;
}elseif ('INF') {

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

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
ContributorAuthor

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

Copy link
Contributor

@warslettwarslett 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 likeprotected function castStringToBuiltInType(string $builtInType, string $value) it will make this easier to fix in API Platform without further duplication.

dunglas reacted with thumbs up emoji
$data = (int)$data;
}elseif ('NaN' ===$data) {
returnNAN;
}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

'context' =>$context,
'ignored' =>$this->ignoredAttributes,
'camelized' =>$this->camelizedAttributes,
]));

Choose a reason for hiding this comment

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

change looks unrelated

Copy link
ContributorAuthor

@mkrausermkrauser 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 likeprotected 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?

teohhanhui reacted with confused emoji
break;
case Type::BUILTIN_TYPE_FLOAT:
if (is_numeric($data)) {
return'0x' ===$data[0].$data[1] ?hexdec($data) : (float)$data;
Copy link
ContributorAuthor

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

teohhanhui reacted with confused emoji
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

$data = (int)$data;
}elseif ('NaN' ===$data) {
returnNAN;
}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.

@fabpot
Copy link
Member

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

Copy link
Member

@dunglasdunglas 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

Merging as new feature though.

@fabpotfabpot changed the base branch from3.4 tomasterSeptember 2, 2020 05:44
@fabpot
Copy link
Member

Thank you@mkrauser.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

+2 more reviewers

@teohhanhuiteohhanhuiteohhanhui left review comments

@warslettwarslettwarslett left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp