Skip to content

Added support for a third deserialize parameter for the DateTime type #788

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

Merged
merged 2 commits into from
Jun 23, 2017

Conversation

bobvandevijver
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Doc updated yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets schmittjoh/JMSSerializerBundle#582
License Apache-2.0

This PR adds an third parameter to the DateTime format for the Type annotation, allowing to use a different deserialization type. This can be useful when only serializing dates in a custom format and you need to have the other fields all zeroed.

@bobvandevijver
Copy link
Contributor Author

Relates to #787, both will fix the problem I experience in schmittjoh/JMSSerializerBundle#582.

Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

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

Can you add some tests? without, this can't be merged

@@ -175,8 +175,8 @@ public function deserializeDateIntervalFromJson(JsonDeserializationVisitor $visi

private function parseDateTime($data, array $type, $immutable = false)
{
$timezone = isset($type['params'][1]) ? new \DateTimeZone($type['params'][1]) : $this->defaultTimezone;
$format = $this->getFormat($type);
$timezone = isset($type['params'][1]) && $type['params'][1] != "" ? new \DateTimeZone($type['params'][1]) : $this->defaultTimezone;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can just use !empty instead of isset

+----------------------------------------------------------+--------------------------------------------------+
| DateTime<'format', 'zone'> | PHP's DateTime object (custom format/timezone) |
+----------------------------------------------------------+--------------------------------------------------+
| DateTime<'format', 'zone', 'deserializeFormat'> | PHP's DateTime object (custom format/timezone, |
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you specify that is optional? (same for zone since we are editing it)

{
if ($deserialize && isset($type['params'][2])){
Copy link
Collaborator

Choose a reason for hiding this comment

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

space before { (PSR standards)

{
if ($deserialize && isset($type['params'][2])){
return $type['params'][2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

4 spaces indent

@goetas
Copy link
Collaborator

goetas commented Jun 17, 2017

Very appreciated this PR 😄 , I also had to use some workarounds to solve a similar issue

@bobvandevijver
Copy link
Contributor Author

Solved your review notes, and added a simple test. I'm not really experienced with tests, so when it's not sufficient, maybe you can take a look?

@goetas goetas merged commit 72d87dc into schmittjoh:master Jun 23, 2017
@goetas
Copy link
Collaborator

goetas commented Jun 23, 2017

@bobvandevijver thanks for your PR. (If you are interested have a look at my version of the testcase https://github.com/schmittjoh/serializer/blob/b92cf25987d0f2bae0817476a117f8b2811a54ef/tests/Handler/DateHandlerTest.php)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants