-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[PHP] Allow selecting Content-Type #13769
[PHP] Allow selecting Content-Type #13769
Conversation
After a bit of rethinking, I decided to remove the validation of the Content-Type. The OpenAPI Specification doesn't offer a way to describe parameters for Content-Type media types. A service may offer different options based on the content-type parameters, and they may not be described as separate content-types in the OpenAPI definition. For example, oData-based API's offer several parameters that can be used to limit the amount of data to be sent back to the client. Examples:
Some of these parameters may make sense in some contexts, but not in others: for example, if you are doing a full search in a big table, you may use "odata.metada=minimal" to reduce the amount of data to be retrieved, but if the search of for a specific subset and you will perform an update on it later, then you will need the full metadata information. By removing the validation on the field, we open a big door for Content-Type, allowing the client user to adjust its parameters on the fly - instead of relying on what is hard-coded in the OpenAPI definition. Hope you guys agree! 😄 |
{ | ||
$headers = []; | ||
|
||
$accept = $this->selectAcceptHeader($accept); | ||
if ($accept !== null) { | ||
$headers['Accept'] = $accept; | ||
} | ||
if(!$isMultipart) { | ||
if($contentType === '') { | ||
$contentType = 'application/json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. We can keep 'application/json' as the default for the time being if nothing is provided. For some other clients, we've removed such default as some servers do not expect "application/json" as the content-type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about what you want me to do here - the current code is doing exactly that: keeping application/json as default if nothing is provided... The "$isMultipart" check is just a rewrite of what the previous code used to do: if multipart is set, don't send any contentType header...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to do. More of an FYI that something we need to change later.
The current HeaderSelector class is handling Content-Type incorrectly:
The first one is critical when sending data to Microsoft Business Central: without it, sending a numeric (floating point) field through POST will result in 500 error.
In order to solve this issues, and after discussing them with @wing328, I'm presenting this PR to add the Content-Type as an extra parameter to the operation calls.
I thought about creating constants for the Content-Types, but normalizing them in order to create the constant names (like we do for field names and enum constants, for example) would require changes to the PhpClientCodegen. To avoid that, I chose to create a constant array to hold the possible Content-Types for each operation. In this way, we can provide a list of possible values for the $contentType parameter, and also validate it.
I've also discussed changes to the Accept header. They will come in a separate PR, to keep it small.
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.1.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@jebentier, @dkarlovi, @mandrean, @jfastnacht, @ybelenko, @renepardon