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.

Sign up for GitHub

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] no need to serialize collections, Guzzle does that #3984

Merged
merged 2 commits into from
Mar 4, 2020
Merged

[PHP] no need to serialize collections, Guzzle does that #3984

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4e1529
fix(php): only serialize collections if !explode, Guzzle handles the …
Sep 29, 2019
d50fe62
fix(php): update petstore samples
Sep 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,23 +196,26 @@ class ObjectSerializer
*
* @return string
*/
public static function serializeCollection(array $collection, $collectionFormat, $allowCollectionFormatMulti = false)
public static function serializeCollection(array $collection, $style, $allowCollectionFormatMulti = false)
{
if ($allowCollectionFormatMulti && ('multi' === $collectionFormat)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ($allowCollectionFormatMulti && ('multi' === $style))

($collectionFormat doesn't exist anymore ;) )

Copy link
Member

Choose a reason for hiding this comment

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

I'll fix it in a separate PR.

If no further feedback, I'll merge it on the coming Tuesday.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #5517

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, William!
Do you want me to create a PR for my other comment (regarding "form" support)? I've been using the suggested fix in my own fork, and it works pretty fine! :)

Copy link
Member

Choose a reason for hiding this comment

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

@thomasphansen yes please 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! #5519

// http_build_query() almost does the job for us. We just
// need to fix the result of multidimensional arrays.
return preg_replace('/%5B[0-9]+%5D=/', '=', http_build_query($collection, '', '&'));
}
switch ($collectionFormat) {
switch ($style) {
case 'pipeDelimited':
case 'pipes':
return implode('|', $collection);

case 'tsv':
return implode("\t", $collection);

case 'spaceDelimited':
case 'ssv':
return implode(' ', $collection);

case 'simple':
case 'csv':
// Deliberate fall through. CSV is default format.
default:
Expand Down
16 changes: 12 additions & 4 deletions modules/openapi-generator/src/main/resources/php/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,16 +472,24 @@ use {{invokerPackage}}\ObjectSerializer;
$multipart = false;

{{#queryParams}}

// query params
{{#collectionFormat}}
{{#isExplode}}
if (${{paramName}} !== null) {
$queryParams['{{baseName}}'] = ${{paramName}};
}
{{/isExplode}}
Comment on lines +477 to +481
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't properly support the 'form' style. If we look at other implementations (such as the Javascript one), when explode is true and style equals to 'form', the parameter should be expanded so that each key inside the parameter (which should then be an array) would result in a new query parameter. There is a very nice example of this in the accepted answer for this thread at stackoverflow.
So, to make this work, in my humble opinion this code might be changed to:

        if (${{paramName}} !== null) {
        {{#style}}
            if('form' === '{{style}}' && is_array(${{paramName}})) {
                foreach(${{paramName}} as $key => $value) {
                    $queryParams[$key] = $value;
                }
            }
            else {
                $queryParams['{{baseName}}'] = ${{paramName}};
            }
        {{/style}}
        {{^style}}
            $queryParams['{{baseName}}'] = ${{paramName}};
        {{/style}}
        }

The only problem here is: PR #4042 is unfortunately incomplete, and due to it, the style property is always empty. I've created a new PR #4640 to fix the issue...

Copy link
Contributor

Choose a reason for hiding this comment

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

EDIT: #4640 is merged now, which means the style property should work fine from now on! :)

{{^isExplode}}
if (is_array(${{paramName}})) {
${{paramName}} = ObjectSerializer::serializeCollection(${{paramName}}, '{{collectionFormat}}', true);
${{paramName}} = ObjectSerializer::serializeCollection(${{paramName}}, '{{#style}}{{style}}{{/style}}{{^style}}{{#collectionFormat}}{{collectionFormat}}{{/collectionFormat}}{{/style}}', true);
}
{{/collectionFormat}}
if (${{paramName}} !== null) {
$queryParams['{{baseName}}'] = ObjectSerializer::toQueryValue(${{paramName}});
$queryParams['{{baseName}}'] = ${{paramName}};
}
{{/isExplode}}

{{/queryParams}}

{{#headerParams}}
// header params
{{#collectionFormat}}
Expand Down