[PHP-Symfony] Fixes #14930 #14933
Merged
[PHP-Symfony] Fixes #14930 #14933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes parts of OpenAPITools#14930. Without this patch a parameter declared as date-time is validated against Symfony's "DateTime" constraint, which always fails. Because this constraint expects a string with format "Y-m-d H:i:s". It fails because the generated code performs the check after the deserialization, so the variable checked is not a string anymore. Besides this, even if we performed that validation on the string, that would not work well because OpenApi specification expects date-time to conform to RFC 3339 and that "Y-m-d H:i:s" would reject RFC 3339 compliant dates. With this change we ensure that the string provided by the web user could be parsed by PHP to DateTime, which solves both issues. (Note however that it means that it now accepts more formats than just RFC 3339 compliant ones for those parameters (it would accept all formats accepted by PHP DateTime). That being said it's compliant with the guideline ""be conservative in what you send, be liberal in what you accept")
6 tasks
This fixes one of the issue described on OpenAPITools#14930, namely that the deserializeString method of the generated class JsmSerializer returns null for other types of string, but not for date-time. Instead it returns a DateTime which represents "now" (because that what `new DateTime(null)` does). Consequently when an API declares a date-time parameter as non-required and when that endpoint is called without that parameters, then the user code would end up having "now" instead of "null" for this parameter.
|
thanks for the PR. cc @jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ybelenko (2018/07), @renepardon (2018/12) |
|
LGTM |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the issues described on #14930 , ie: it fixes the parsing of date-time parameter, as well as non-required date-time parameters.
All the details are either on the bug report and on commit messages.
This PR can be tested by setting up a Symfony app with autogenerated code using the openapi file described on #14930 and performing the queries:
(since this is PHP I guess I should ping @jebentier, @dkarlovi , @mandrean, @jfastnacht , @ybelenko @renepardon (sorry in advance for the noise if I misunderstood that guideline))
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.For Windows users, please run the script in Git BASH.
master(6.3.0) (minor release - breaking changes with fallbacks),7.0.x(breaking changes without fallbacks)