Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

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

Already on GitHub? # to your account

fix issue #199: "array" type ignoring DateTime format #201

Merged
merged 11 commits into from
Feb 5, 2014

Conversation

lukey78
Copy link

@lukey78 lukey78 commented Dec 9, 2013

fixes issue #199

Need feedback for XML deserialization of type("array<string,DateTime>) with XmlKeyValuePairs. Currently the test is failing.

deserialization of named DateTime array from XML currently fails!
@@ -99,7 +99,8 @@ public function visitArray($data, array $type, Context $context)
}

foreach ($data as $k => $v) {
$v = $this->navigator->accept($v, isset($type['params'][1]) ? $type['params'][1] : null, $context);
$typeArray = isset($type['params'][0]) ? (isset($type['params'][1]) && is_array($type['params'][1]) ? $type['params'][1] : $type['params'][0]) : null;
Copy link
Owner

Choose a reason for hiding this comment

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

How about renaming this to elementType?

Copy link
Owner

Choose a reason for hiding this comment

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

Can you also rewrite this condition, it's getting too long. Maybe move this to a separate method getElementType($arrayType).

@schmittjoh
Copy link
Owner

As I said on the other issue XmlKeyValuePairs is only working for serialization. You can remove the test-case for the deserialization part.

@lukey78
Copy link
Author

lukey78 commented Dec 13, 2013

Thanks for your comments. I tried to implement it that way. Hope it's ok.

@lukey78
Copy link
Author

lukey78 commented Feb 4, 2014

ping!

@schmittjoh
Copy link
Owner

Sorry for the wait.

I remember that there were a few coding style issues here (nothing big). You often use short if/foreach statements, could you make sure that their body is always enclosed in { }?

Could you also rebase/merge latest master so that your PR can be tested/analyzed?

Please also add a comment if everything goes green so I can merge right away :)

Jens Hassler added 7 commits February 4, 2014 17:45
deserialization of named DateTime array from XML currently fails!
refactored tests to exclude deserializing XmlKeyValuePairs
Conflicts:
	src/JMS/Serializer/AbstractVisitor.php
	tests/JMS/Serializer/Tests/Serializer/BaseSerializationTest.php
	tests/JMS/Serializer/Tests/Serializer/JsonSerializationTest.php
@lukey78
Copy link
Author

lukey78 commented Feb 5, 2014

Ok, had some minor issues with integrating latest master, but should be ok now. Tests are inside and passing green.

Thanks!

schmittjoh added a commit that referenced this pull request Feb 5, 2014
fix issue #199: "array" type ignoring DateTime format
@schmittjoh schmittjoh merged commit c93adad into schmittjoh:master Feb 5, 2014
@schmittjoh
Copy link
Owner

Great, thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants