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

Serialized DateTime instances are not valid ISO-8601 #713

Closed
xelan opened this issue Feb 10, 2017 · 7 comments
Closed

Serialized DateTime instances are not valid ISO-8601 #713

xelan opened this issue Feb 10, 2017 · 7 comments

Comments

@xelan
Copy link

xelan commented Feb 10, 2017

The default format of JMS\Serializer\Handler\DateHandler is \DateTime::ISO8601, which is however not fully compatible with ISO 8601. Specificially, the time zone delimiter is missing.

Example:
2016-04-06T14:39:32+0200
should be:
2016-04-06T14:39:32+02:00

As a result, the serialized string can't be validated via XML schema (xs:dateTime) and the interoperability with other software is also limited.
The PHP documentation suggests to use DateTime::ATOM instead of DateTime::ISO8601 for ISO 8601 compatibility.

In my opinion, the fix would technically be a breaking change, as the serialized XML/JSON would be different from that of previous versions. The deserialization would need to support both variants for backwards compatibility, newly serialized documents should use the standard-compliant DateTime format as default.

What do you think?

@xelan
Copy link
Author

xelan commented Feb 10, 2017

Deserialization already seems to work with both DateTime::ISO8601 and DateTime::ATOM using JSON and XML. (Tested with jms/serializer 1.4.2)

xelan added a commit to xelan/serializer that referenced this issue Feb 10, 2017
@mvrhov
Copy link

mvrhov commented Feb 10, 2017

This can also fixed when DateHandler instance is created. And the bundle already supports this as you can set the format in your configuration.

@xelan
Copy link
Author

xelan commented Feb 10, 2017

Thanks for your fast response, @mvrhov. What is the recommended way to set the DateHandler for the serializer? I don't use the bundle.

To fix the issue and provide a standards-compliant default, I created a patch which sets the default to DateTime::ATOM and adapted the test suite accordingly.

@goetas
Copy link
Collaborator

goetas commented Feb 10, 2017

If you are not using the bundle, then probably you are using the serialization builder.

$builder->configureHandlers(function($h) {
        $h->registerSubscribingHandler(new DateHandler(YOUR_DEFAULT));
        $h->registerSubscribingHandler(new PhpCollectionHandler()); // omit if not using 
        $h->registerSubscribingHandler(new ArrayCollectionHandler()); 
        $h->registerSubscribingHandler(new PropelCollectionHandler()); // omit if not using propel
});
$serializer = $builder->build();

@xelan
Copy link
Author

xelan commented Feb 10, 2017

Yes, I'm using the SerializerBuilder, so your suggested solution should work. Thank you very much, @goetas!
Is there still interest in changing the default to conform with ISO-8601 later on (e.g. with serializer 2.0)? If so, I could submit my PR.

@goetas
Copy link
Collaborator

goetas commented Feb 10, 2017

yes, i'm interested, so a PR (with tests 😄) is welcome.

The only thing is that a release date for 2.0 is really not yet defined, so if can take many months before it gets released.
For now i'm just adding ideas and requirements for the 2.0, but haven't started yet to write code..

@goetas
Copy link
Collaborator

goetas commented Feb 10, 2017

closing the issue since it looks resolved

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

No branches or pull requests

3 participants