[PHP] Correctly format JSON in headers #4024
Conversation
`ObjectSerializer::toHeaderValue()` in the generated PHP code calls
`toString()` on the values, which formats JSON with the `JSON_PRETTY_PRINT`
option. This will result in a multi-line header which cannot be parsed
since linebreaks aren't allowed by RFC 7230.
In my case I have a header schema called `UpdateUser` which I had hoped
would be serialized as `{"type":"staff","id":123}`.
Every single `__toString()` in the generator does the same thing, so I
figured it's safe to change `toHeaderValue()` to convert to JSON directly,
without `JSON_PRETTY_PRINT`. This fix works for me.
|
It seems there are some cases I didn't cover. Let me fix that up real quick... |
| return $value->toHeaderValue(); | ||
| } | ||
|
|
||
| return self::toString($value); |
There was a problem hiding this comment.
How about this instead of adding toHeaderValue() to model_generic.mustache ? 💡
// Strip newlines and spaces which comes from JSON_PRETTY_PRINT
return str_replace([PHP_EOL, ' '], '', (string)$value);This will reduce the if condition.
if (method_exists($value, 'toHeaderValue')) { return $value->toHeaderValue(); }
There was a problem hiding this comment.
There's no need to remove spaces, as they are allowed in a header value (just not at the beginning/end, but JSON wouldn't begin/end with spaces anyway). But yeah, otherwise this approach would work I suppose.
There was a problem hiding this comment.
I pushed this change. It is simpler, but now there are extra indents in the JSON since it was pretty-printed, which doesn't really make sense for a single line.
There was a problem hiding this comment.
Yes, that is why I removed spaces in the sample. 💡
There was a problem hiding this comment.
Yeah, I see. We should still allow spaces inside strings in the JSON. Would you prefer the previous solution then, with toHeaderValue() on the model, or keep it like this?
There was a problem hiding this comment.
Oh... sorry for my oversight... I would prefer the previous solution, which makes clean output.
|
The circle CI failure will be fixed via #4041 . nothing to do with this PR. I've checked that unit tests of PHP client has passed locally. $ pwd
/Users/akihito1/src/github.com/ackintosh/openapi-generator-1
$ php vendor/bin/phpunit tests
PHPUnit 7.5.16 by Sebastian Bergmann and contributors.
................................................................. 65 / 70 ( 92%)
..... 70 / 70 (100%)
Time: 12.44 seconds, Memory: 10.00 MB
OK (70 tests, 412 assertions) |
|
@hinrik thanks for the PR, which has been included in the v4.1.3 release: https://twitter.com/oas_generator/status/1180123829626003456 |
* master: (35 commits) [haskell-http-client] update samples (#4073) [haskell-http-client] Bump deps to LTS 14.7 (#4068) update release for 4.2.0 [typescript-axios] Fix api generating incorrect seralization type check (#4051) prepare 4.1.3 release (#4052) typescript-node: form data file (#3967) Add a link to blog post on vertx and openapi (#4048) better wording for apiNameSuffix option description (#4045) [Ruby] fix ruby test, update error message (#4041) [PHP] Correctly format JSON in headers (#4024) [haskell-http-client] add dateTimeParseFormat cli option - overrides the format string used to parse a datetime (#4037) Add frankyjuang to the C# technical committee (#4036) Feature/api name suffix (#3918) [F#] minor improvements to the generators (#3968) Repaired Checkstyle (#4029) mockito 3.1.0 (#4035) typescript-fetch: fix return type of primitive value (#4028) [typescript][node]: Add accept header if produces is not empty (#3966) [haskell-http-client] disable unused import warning in Core.hs (#4020) Add a link to the tutorial in http4k (#4019) ...
ObjectSerializer::toHeaderValue()in the generated PHP code callstoString()on the values, which formats JSON with theJSON_PRETTY_PRINToption. This will result in a multi-line header which cannot be parsed
since linebreaks aren't allowed by RFC 7230.
In my case I have a header schema called
UpdateUserwhich I had hopedwould be serialized as
{"type":"staff","id":123}.This PR adds a
toHeaderValue()method to models which doesn't useJSON_PRETTY_PRINT. Works for me.FYI @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko @renepardon