Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: 3.4
from

Conversation

@mkrauser
Copy link
Contributor

mkrauser commented Oct 4, 2019

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

@mkrauser mkrauser force-pushed the mkrauser:3.4 branch from 164bf56 to a9a50b9 Oct 4, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 4, 2019
@mkrauser mkrauser force-pushed the mkrauser:3.4 branch from a9a50b9 to bbbac21 Oct 4, 2019
$data = (int) $data;
} elseif ('NaN' === $data) {
return NAN;
} elseif ('INF') {

This comment has been minimized.

Copy link
@nicolas-grekas

This comment has been minimized.

Copy link
@warslett

warslett Oct 21, 2019

also doesn't hurt

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 5, 2019

Member

let's do it then?

Copy link

warslett left a comment

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.

@@ -252,6 +254,54 @@ private function validateAndDenormalize($currentClass, $attribute, $data, $forma
$data = [$data];
}
// In XML and CSV all basic datatypes are represented as strings, it is e.g. not possible to determine,

This comment has been minimized.

Copy link
@teohhanhui

teohhanhui Oct 22, 2019

Contributor

For XML, isn't it better to do XSD validation in the encoder instead?

For CSV, all bets are off. We can't reasonably know how to convert anything here.

This comment has been minimized.

Copy link
@warslett

warslett Oct 22, 2019

Would that mean that developers would need to create an XSD document for each and every entity they want to deserialize in order to work with XML (having already defined type information separately in the Entity)? Is that practical? XSD would be a nice feature for validation but at the moment XML deserialization is essentially unusable for any data that includes floats and integers and there aught to be a way of dealing with that. XML APIs will typically interpret values set on boolean properties as booleans. Validation is a separate concern to that.

@mkrauser mkrauser force-pushed the mkrauser:3.4 branch from b9257ed to a6157bd Oct 24, 2019
@mkrauser mkrauser force-pushed the mkrauser:3.4 branch from a6157bd to 98105f5 Oct 24, 2019
@mkrauser mkrauser requested a review from nicolas-grekas Oct 25, 2019
'context' => $context,
'ignored' => $this->ignoredAttributes,
'camelized' => $this->camelizedAttributes,
]));

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 5, 2019

Member

change looks unrelated

if (
\is_string($data) &&
(XmlEncoder::FORMAT === $format || CsvEncoder::FORMAT === $format)
) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 5, 2019

Member

should be on one line (same below - our CS is to prefer long lines)

case Type::BUILTIN_TYPE_INT:
if (
ctype_digit($data) ||
'-' == $data[0] && ctype_digit(substr($data, 1))

This comment has been minimized.

Copy link
@nicolas-grekas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.