Skip to content

Conversation

@tchiotludo
Copy link
Contributor

@tchiotludo tchiotludo commented Sep 4, 2017

The serialize method don't handle correctly \DateTimeImmutable

The unserialize method expect a string (\DateTimeImmutable constructor)

        } elseif (strcasecmp($type, '\datetimeimmutable') === 0) {
            $val = new \DateTimeImmutable($val);

But the serialize method serialize '\DateTimeImmutable' as object
This throw an DateTimeImmutable::__construct() expects parameter 1 to be string, object given on calling ->populateFromArray((array) $serialize)

@tchiotludo
Copy link
Contributor Author

Any update ?

Copy link
Collaborator

@haphan haphan left a comment

Choose a reason for hiding this comment

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

LGTM. Please also re-base with master. Thank you
@tchiotludo

if ($val instanceof Serializable) {
return $val->serialize();
} else if ($val instanceof \DateTimeImmutable) {
return $val->format('Y-m-d H:i:s.u');
Copy link
Collaborator

Choose a reason for hiding this comment

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

use ISO 8601 instead of hard-code date time format $val->format('c')

@tchiotludo
Copy link
Contributor Author

done & thanks 👍

@haphan
Copy link
Collaborator

haphan commented Feb 16, 2018

Please fix travis error (PSR2) @tchiotludo then we are good to go.

@haphan haphan merged commit 9210914 into php-opencloud:master Feb 16, 2018
@haphan
Copy link
Collaborator

haphan commented Feb 16, 2018

thanks @tchiotludo

@tchiotludo
Copy link
Contributor Author

np @haphan, good to see my fork disappear 😄

@haphan
Copy link
Collaborator

haphan commented Feb 16, 2018

@tchiotludo As we are trying to maintain branch 2.x till end of March 2018, can you also back-port this fix to branch 2.0

Thanks.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants