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

Use PHP's DateTime::ATOM for true ISO-8601 support #2385

Merged
merged 2 commits into from
Mar 16, 2016
Merged

Use PHP's DateTime::ATOM for true ISO-8601 support #2385

merged 2 commits into from
Mar 16, 2016

Conversation

arnested
Copy link
Contributor

Currently we use PHP's DateTime::ISO8601 for the date-time properties but according to http://php.net/manual/en/class.datetime.php#datetime.constants.iso8601 it is actually not compatible with ISO-8601.

Instead we should use DateTime::ATOM.

@wing328
Copy link
Contributor

wing328 commented Mar 16, 2016

@arnested thanks for the PR. I don't see change in the object serializer mustache template. I only found changes to the PHP Petstore sample.

Would you please take a look and rebase on the latest master?

@wing328 wing328 added this to the v2.1.6 milestone Mar 16, 2016
@arnested
Copy link
Contributor Author

Yeah, I apparently got something wrong there. Rebasing and fixing right now. New push in a minute.

Sorry for the trouble, @wing328.

@wing328
Copy link
Contributor

wing328 commented Mar 16, 2016

@arnested no worry. Please take your time.

Currently we use PHP's DateTime::ISO8601 for the date-time properties
but according to
http://php.net/manual/en/class.datetime.php#datetime.constants.iso8601
it is actually not compatible with ISO-8601.

Instead we should use DateTime::ATOM.
@arnested
Copy link
Contributor Author

Just a minor glitch -- fixed and push now.

*UserApi* | [**loginUser**](docs/UserApi.md#loginuser) | **GET** /user/login | Logs user into the system
*UserApi* | [**logoutUser**](docs/UserApi.md#logoutuser) | **GET** /user/logout | Logs out current logged in user session
*UserApi* | [**updateUser**](docs/UserApi.md#updateuser) | **PUT** /user/{username} | Updated user
*PetApi* | [**addPet**](PetApi.md#) | **POST** /pet | Add a new pet to the store
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using a customized version of PhpClientCodegen and DefaultCodegen somehow? e.g. {{operationIdLowerCase}} seems not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After rebasing I forgot to mvn clean package so this was probably generated with an older / somehow other version of codegen.

I've rebuild codege, regenerated the petstore sample, and run the phpunit test suite now.

CI is currently building.

Sorry for the noise again and thank you for catching it.

wing328 added a commit that referenced this pull request Mar 16, 2016
Use PHP's DateTime::ATOM for true ISO-8601 support
@wing328 wing328 merged commit 4d19da4 into swagger-api:master Mar 16, 2016
@arnested arnested deleted the php_date_time_iso8601_vs_atom branch March 16, 2016 12:41
@wing328
Copy link
Contributor

wing328 commented Mar 16, 2016

Upgrade note from 2.1.5 to 2.1.6

Starting from 2.1.6, DateTime::ATOM will be used instead of DateTime::ISO8601. The change is based on the

Note: This (DateTime::ISO8601) format is not compatible with ISO-8601, but is left this way for backward compatibility reasons. Use DateTime::ATOM or DATE_ATOM for compatibility with ISO-8601 instead.

Ref: http://php.net/manual/en/class.datetime.php#datetime.constants.iso8601

If this breaks your application after upgrade, please reply to let us know and we'll share a workaround.

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