Skip to content

Accept arrays as arguments to collection parameters in PHP client.#1499

Merged
wing328 merged 2 commits intoswagger-api:masterfrom
arnested:collectionFormat
Jan 4, 2016
Merged

Accept arrays as arguments to collection parameters in PHP client.#1499
wing328 merged 2 commits intoswagger-api:masterfrom
arnested:collectionFormat

Conversation

@arnested
Copy link
Copy Markdown
Contributor

@arnested arnested commented Nov 3, 2015

This patch makes it possible to use arrays as arguments for parameters defined as arrays of strings -- and respects the collectionFormat.

Until now you had to implode the string to an array yourself before calling the API.

I've been thinking about also adding type hints to array for those parameters in method argument list. But since that would break backwards compatibility it should probably be hidden behind a command line option or something.

NB: This needs swagger-parser 1.0.12.

@arnested
Copy link
Copy Markdown
Contributor Author

arnested commented Nov 3, 2015

Tests running fine:

PHPUnit 5.0.0 by Sebastian Bergmann and contributors.

.............                                                     13 / 13 (100%)

Time: 6.49 seconds, Memory: 16.00Mb

OK (13 tests, 1102 assertions)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this handles the "multi" collectionFormat correctly. Assuming a parameter "foo" has an array value of 2 elements, "bar" and "baz", in the $collection here, then this is expected to be included in the URL: foo=bar&foo=baz.
However, the current implementation of serializeCollection seems to return string 0=bar&1=baz, which will be included into the URL via ApiClient.php and the URL would contains:
foo=0%3Dbar%261%3Dbaz

(see here in swagger spec for the above example of "multi" collectionFormat)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, @xhh, I haven't taken account for multidimensional arrays. That's a bug.

I'll look into -- either I have to do query string building myself or I'll postprocess the result of http_build_query() and weed out the encoded [].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI I just submitted PR #1509 to support collectionFormat in Ruby client, which is using Typhoeus to make HTTP requests.
Typhoeus handles array parameters in the same way with the multi collectionFormat. So the build_collection_param method returns the (array) parameter directly.
I've no idea how PHP handles it, but hope this helps.

@arnested
Copy link
Copy Markdown
Contributor Author

arnested commented Jan 3, 2016

I changed the pull request to handle @xhh comments about multidimensionale arrays.

The pull request has been rebased on HEAD of master and the petstore sample has been regenerated.

@fehguy
Copy link
Copy Markdown
Contributor

fehguy commented Jan 4, 2016

@wing328 please merge if this looks good to you...

@wing328 wing328 added this to the v2.1.5 milestone Jan 4, 2016
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 4, 2016

The change definitely looks good.

CI test result:

PHPUnit 4.8.21 by Sebastian Bergmann and contributors.

...............

Time: 7.33 seconds, Memory: 6.00Mb

OK (15 tests, 288 assertions)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:18 min
[INFO] Finished at: 2016-01-04T15:38:35+08:00
[INFO] Final Memory: 11M/245M

wing328 added a commit that referenced this pull request Jan 4, 2016
Accept arrays as arguments to collection parameters in PHP client.
@wing328 wing328 merged commit 563cabe into swagger-api:master Jan 4, 2016
@arnested arnested deleted the collectionFormat branch January 4, 2016 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants