[Php] Convert boolean value for query string (additional option) by ksvirkou-hubspot · Pull Request #11880 · OpenAPITools/openapi-generator

ksvirkou-hubspot · 2022-03-15T14:47:33Z

This PR creates an additional property "booleanFormatForQueryString"
User can choose the format for conversation of boolean to the query string
user can choose int (param=1 or param=0) or string (param=true or param=false)
int is default format because guzzle/psr7 converts boolean to int (param=1 or param=0)

it will fix #11683

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
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.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

update master

ksvirkou-hubspot · 2022-03-15T20:05:20Z

@jebentier , @dkarlovi , @mandrean , @jfastnacht , @ackintosh , @ybelenko , @renepardon

ksvirkou-hubspot · 2022-03-17T08:27:00Z

Hi Everyone
Could anyone check this pr, please?
@jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko @renepardon
@wing328

ybelenko · 2022-03-18T01:26:23Z

Ohh, you're fixing the same part of generator that I do right now... I changed toQueryValue method significantly at #11225 because we need to support many styles of query serialization including deepObject.

openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php

Lines 159 to 215 in 920cfe5

    
               /** 
        
                * Take query parameter properties and turn it into an array suitable for 
        
                * native http_build_query or GuzzleHttp\Psr7\Query::build. 
        
                * 
        
                * @param mixed  $value       Parameter value 
        
                * @param string $paramName   Parameter name 
        
                * @param string $openApiType OpenAPIType eg. array or object 
        
                * @param string $style       Parameter serialization style 
        
                * @param bool   $explode     Parameter explode option 
        
                * 
        
                * @return array 
        
                */ 
        
               public static function toQueryValue( 
        
                   $value, 
        
                   string $paramName, 
        
                   string $openApiType = 'string', 
        
                   string $style = 'form', 
        
                   bool $explode = true 
        
               ): array { 
        
                   // return empty string 
        
                   if (empty($value)) return ["{$paramName}" => '']; 
        
                   $query = []; 
        
                   $value = (in_array($openApiType, ['object', 'array'], true)) ? (array)$value : $value; 
        
                   // since \GuzzleHttp\Psr7\Query::build fails with nested arrays 
        
                   // need to flatten array first 
        
                   $flattenArray = function ($arr, $name, &$result = []) use (&$flattenArray, $style, $explode) { 
        
                       if (!is_array($arr)) return $arr; 
        
                       foreach ($arr as $k => $v) { 
        
                           $prop = ($style === 'deepObject') ? $prop = "{$name}[{$k}]" : $k; 
        
                           if (is_array($v)) { 
        
                               $flattenArray($v, $prop, $result); 
        
                           } else { 
        
                               if ($style !== 'deepObject' && !$explode) { 
        
                                   // push key itself 
        
                                   $result[] = $prop; 
        
                               } 
        
                               $result[$prop] = $v; 
        
                           } 
        
                       } 
        
                       return $result; 
        
                   }; 
        
                   $value = $flattenArray($value, $paramName); 
        
                   if ($openApiType === 'object' && ($style === 'deepObject' || $explode)) { 
        
                       return $value; 
        
                   } 
        
                   // handle style in serializeCollection 
        
                   $query[$paramName] = ($explode) ? $value : self::serializeCollection((array)$value, $style); 
        
                   return $query; 
        
               }

We can't merge this PR before #11225. I can integrate your changes into new method later, don't worry. If you want to help, you can write unit tests at samples/client/petstore/php/OpenAPIClient-php/tests/ObjectSerializerTest.php. Wait... seems like you can't because you need to test updated toQueryValue function which you don't have right now. Anyway, this changes will be published in next major release 6.0.0 which scheduled for the end of the March, so hold on, we won't forget your PR.

ybelenko · 2022-03-18T01:28:34Z

It cannot be merged without at least few unit tests, so please describe some test cases, so we can cover your feature.

ksvirkou-hubspot · 2022-03-18T12:00:24Z

@ybelenko
I'll be waiting until you merge and then I'll update this PR and create some tests like these

merge master

# Conflicts: # modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache # modules/openapi-generator/src/main/resources/php/api.mustache # samples/client/petstore/php/OpenAPIClient-php/lib/Api/FakeApi.php # samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php # samples/client/petstore/php/OpenAPIClient-php/lib/Api/UserApi.php # samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php

samples/client/petstore/php/OpenAPIClient-php/tests/ObjectSerializerTest.php

ybelenko · 2022-03-24T02:42:59Z

docs/generators/php.md

 |allowUnicodeIdentifiers|boolean, toggles whether unicode identifiers are allowed in names or not, default is false| |false|
 |apiPackage|package for generated api classes| |null|
 |artifactVersion|The version to use in the composer package version field. e.g. 1.2.3| |null|
+|booleanFormatForQueryString|Specifies the format for converting boolean to query string. The default is 'int'.(e.g. int param=1 or param=0 string param=true or param=false)|<dl><dt>**int**</dt><dd>int</dd><dt>**string**</dt><dd>string</dd></dl>|int|


Additional codegen option, really? Are you sure there are many users who need this?
Beside why not to add that option to modules/openapi-generator/src/main/resources/php/Configuration.mustache instead? That way you can change bool serialization without regeneration of client SDK.

Additional option is better then option in Configuration.mustache
Because no necessary for an user to set up it because it will be common behavior for all requests

no, I think it is necessary for a small number of users (one more reason for using Additional option)

In my opinion it is better to have an Additional option then have option in Configuration.mustache because it will be like “excessive option in Configuration.mustache”
but If for you better option in Configuration.mustache you can say and I’ll rewrite it
What do you think?
@ybelenko

@wing328 As far as I remember you also against adding dozens of options to codegen. In current situation I think Configuration php class is better place to put this option.

@ksvirkou-hubspot Your suggested codegen option changes one single line of code, I understand that it's convinient to you, but client SDK users always ask to consider their use case as one and only important. 😅

Ok
it was only thought
waiting decision and will do it

merge master

ybelenko · 2022-05-03T21:27:17Z

@ksvirkou-hubspot I don't clearly remember did you add that fix in #12120 or not. You should rebase to latest master and solve conflicts if you need this merged.

# Conflicts: # modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache # samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php # samples/client/petstore/php/OpenAPIClient-php/tests/ObjectSerializerTest.php

ksvirkou-hubspot · 2022-05-04T15:37:41Z

@ksvirkou-hubspot I don't clearly remember did you add that fix in #12120 or not. You should rebase to latest master and solve conflicts if you need this merged.

I ve merged master
Short information
I create an additional codegen option "booleanFormatForQueryString"
for setting format for converting boolean for query string
int (param=1 or param=0) or string (param=true or param=false)
we discussed it above
Could you check it one more time, please?
I am ready to rewrite or update something
CC @ybelenko

ybelenko · 2022-05-06T11:21:57Z

As I said I would vote against codegen option in favour to config option. I think you can wait for somebody who share your vision to approve. Otherwise you can rewrite it a bit to use config option and I'll be pleased to merge.

merge master

ksvirkou-hubspot · 2022-05-17T14:01:38Z

This PR creates an additional property "booleanFormatForQueryString" User can choose the format for conversation of boolean to the query string user can choose int (param=1 or param=0) or string (param=true or param=false) int is default format because guzzle/psr7 converts boolean to int (param=1 or param=0)

it will fix #11683

HI @ybelenko
I ve created new PR
Could you check it, please?

ksvirkou-hubspot · 2022-05-23T09:15:34Z

Any updates?
@ybelenko @wing328

ybelenko · 2022-05-25T02:05:23Z

I would close this in favour of #12385

ksvirkou-hubspot and others added 5 commits March 10, 2022 16:19

Merge pull request #1 from OpenAPITools/master

5b9e72a

update master

apply toQueryValue for all values for query string

f162e25

add additional property booleanFormatForQueryString

8cc741b

fix default value and generate samples

d72c6bd

export docks

2e36eb7

ksvirkou-hubspot changed the title ~~[Php] add an additional property "booleanFormatForQueryString"~~ [Php] Convert boolean value for query string Mar 17, 2022

ybelenko self-requested a review March 18, 2022 01:05

ybelenko added Client: PHP Issue: Bug labels Mar 18, 2022

ksvirkou-hubspot and others added 4 commits March 22, 2022 19:34

Merge pull request #2 from OpenAPITools/master

d96e3a9

merge master

fix merge

c2ed5d2

fix merge

146a60b

ksvirkou-hubspot mentioned this pull request Mar 23, 2022

[php] use http_build_query for deepObject support #11225

Merged

ksvirkou-hubspot added 4 commits March 23, 2022 17:09

rewrite templates

eaa95b7

update samples

dcf6863

fix if (empty)

650c52c

regenerate samples

d2ccac3

ybelenko reviewed Mar 24, 2022

View reviewed changes

ksvirkou-hubspot requested a review from ybelenko March 24, 2022 11:58

ksvirkou-hubspot added 2 commits April 8, 2022 11:51

Merge pull request #4 from OpenAPITools/master

13dd735

merge master

Merge pull request #5 from OpenAPITools/master

e30d9ef

merge master

ksvirkou-hubspot mentioned this pull request Apr 13, 2022

[Php] Exclude query params when they're not required #12120

Merged

4 tasks

Merge pull request #7 from OpenAPITools/master

4e10e79

merge master

Merge pull request #9 from OpenAPITools/master

37954c4

merge master

ksvirkou-hubspot changed the title ~~[Php] Convert boolean value for query string~~ [Php] Convert boolean value for query string (additional option) May 23, 2022

wing328 closed this May 25, 2022

ksvirkou-hubspot deleted the feature/queryParamValues branch May 25, 2022 10:27

Uh oh!

Conversation

ksvirkou-hubspot commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

ksvirkou-hubspot commented Mar 15, 2022

Uh oh!

ksvirkou-hubspot commented Mar 17, 2022

Uh oh!

ybelenko commented Mar 18, 2022

Uh oh!

ybelenko commented Mar 18, 2022

Uh oh!

ksvirkou-hubspot commented Mar 18, 2022

Uh oh!

Uh oh!

ybelenko Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

ksvirkou-hubspot Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ybelenko Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

ksvirkou-hubspot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

ybelenko commented May 3, 2022

Uh oh!

ksvirkou-hubspot commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ybelenko commented May 6, 2022

Uh oh!

ksvirkou-hubspot commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksvirkou-hubspot commented May 23, 2022

Uh oh!

ybelenko commented May 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ksvirkou-hubspot commented Mar 15, 2022 •

edited

Loading

ksvirkou-hubspot Mar 24, 2022 •

edited

Loading

ksvirkou-hubspot commented May 4, 2022 •

edited

Loading

ksvirkou-hubspot commented May 17, 2022 •

edited

Loading