[PHP] API client using guzzle6 & psr7 instead of curl #5190

baartosz · 2017-03-24T19:28:44Z

PR checklist

Read the contribution guidelines.
Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Finally finished #1482. Looks massive but without generated samples its roughly 10 files changed plus few new test classes. Large feature anyway.
Important points:

decided not to follow suggestion and to remove ApiClient altogether and make generated API classes call Httplug's Client interface directly. Otherwise It would be messy abstraction (PetApi -> ApiClient -> HttpClient -> Guzzle/Curl/whatever) plus impossible to leverage fancy new features curl was missing (like streams in responses)
deleted generated autoload.php as it make no sense to use it when its necessary to install httplug and guzzle with composer and use vendor/autoload.php
tested with guzzle client (php-http/guzzle6-adapter + guzzlehttp/guzzle) and curl client (php-http/curl-client). Works nice with both of them. Only thing not working currently with curl client was file upload. I left it for another iteration/separate ticket. Tests are currently ran only with guzzle.
ObjectSerializer had some methods static some not. I tried to make all methods not static and inject it to Api object but ran into issue here https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/php/SwaggerClient-php/lib/Model/Pet.php#L390. Finally decided to make it all static for consistency and rework it later
BC breaks:
- new dependencies (obviously)
- initialization of the API class is slightly changed (client adapter has to be passed as argument) (no way around that)
- removed options from Configuration class (client-specific options go to client now)
- headers returned as array from $response->getHeader($name); (its how psr7 expects it to work)

Please let me know if its better to make it optional like --library=httplug or something.

wing328 · 2017-03-25T06:24:48Z

@baartosz thanks for the contribution. We'll review and let you know if we've any question.

cc @arnested @abcsum

wing328 · 2017-03-25T06:27:50Z

cc @dkarlovi , @scop as well since they also filed several PRs before to improve the PHP API client.

git checkout -b baartosz-php_httplug2 2.3.0
git pull https://github.com/baartosz/swagger-codegen.git php_httplug2

dkarlovi · 2017-03-25T07:53:37Z

I'm all for dropping the shipped client as it will make the generated code much more simple, focus of this projects is not to generate HTTP clients.

The problem I see is relying directly on Guzzle interface directly, the whole point of PSR-7 is NOT to do that. Generated code should rely on PSR-7 exclusively and enable me to use whatever compatible implementation. This would make integrating the SDK much harder than it should be.

For example, you might already have a PSR-7 compatible client in your app pre-configured, you should be able to just inject it and use it. This means generated code should rely on functionality provided by the interface, for example Streams.

dkarlovi · 2017-03-25T08:03:14Z

Also: having this behind an option seems reasonable, but I think it would make supporting other libraries messy. What I feel would be a much cleaner approach is to do a "base" PSR-7 only implementation which wouldn't be able to work without injecting the client at all.

Then, --library=httplug would mean we generate an additional class which modifies composer.json and preconfigures everything to be ready to use (so, libraries only add the implementation to use, not modify anything).

baartosz · 2017-03-25T08:54:23Z

@dkarlovi i see your point. So basically instead of httplug provide own interface for the client and optional --library to have one already there?

guzzle/psr7 now is used to provide helpers e.g. To build multipart requests. Are you suggesting writing it from scratch instead?

dkarlovi · 2017-03-25T12:10:26Z

@baartosz pretty much, yes. If you have a functional PSR-7 client enabled, you can just inject it and have it work with the generated SDK. For example, your HTTP client might have a logger set up which stores all requests to Elastic search, you don't want to have another client which does the same thing, but lacks integration to your environment.

I don't suggest writing anything from scratch, no, the whole point of removing the HTTP client is avoid doing that. :) Can you point me to where exactly you're constructing multi-part messages? It might be an XY problem.

dkarlovi · 2017-03-25T12:11:16Z

@baartosz BTW thank you very much for your contribution, it's a great effort on your part.

baartosz · 2017-03-25T12:34:49Z

@dkarlovi makes sense. At this point generated client is using psr7 compatible request and response, so client itself is easy to replace with whatever is decided.

Multipart stream from guzzle/psr7 is used here: https://github.com/swagger-api/swagger-codegen/pull/5190/files#diff-ae9e1fe068c165f6731482ae703b427dR132. Same case with GuzzleHttp\Psr7\Request and GuzzleHttp\Psr7\Response - it's just implementation of psr interface so I don't see any problem using it still even if httplug will be removed.

Thanks for feedback.

wing328 · 2017-03-28T08:50:44Z

modules/swagger-codegen/src/main/resources/php/Configuration.mustache

-     * @var string
-     */
-    protected $curlConnectTimeout = 0;
+    protected $host = '{{basePath}}';


Please use unescape basePath instead: {{{basePath}}}

wing328 · 2017-03-28T08:53:38Z

modules/swagger-codegen/src/main/resources/php/Configuration.mustache

@@ -123,51 +109,6 @@ class Configuration
    protected $tempFolderPath;

    /**
-     * Indicates if SSL verification should be enabled or disabled.


I notice that the SSL, proxy and timeout methods have been removed. Can you share more with me how a developer will set SSL, proxy, timeout in the new client?

@wing328 idea is to remove the HTTP client implementation completely and inject a PSR-7 capable client such as Guzzle.

This means setting up these kinds of configuration would be outside of scope of generated code, it would assume the injected client is capable of being configured properly in its own right. This code would just use it as-is.

@dkarlovi thanks for explaining more. To ease the migration, I think we will need to provide auto-generated documentation on how to set these values using Guzzle (the default PSR-7 http client)

@wing328 Need some guidance here. I am not sure about priorities - is it more important to have working client out of the box, or having more flexible one?
Should we add client or httplug+client dependencies by default?
Is enough to provide generated apis classes calling send($request) on some ApiClientInterface, and expect users to choose client library and select/create adapter?
Or maybe leave no-client version as default and add --library=httplug?

Here's a suggested flow which makes sense to me:

generate no implementation dependency by default, this will generate a "clean" version, add additional info / sample code on how to inject own HTTP client

generate an additional class if --library=httplug is provided, this version is basically 1) with an additional item in composer.json and a HTTP client pre-injected for you (so, you can still inject another client yourself if you don't want to use the additional class, it just doesn't install a depenency you might not want by default)

We might make 2) the default to keep BC though.

@wing328 sounds good to me, documentation should note how to create a "bare" version, for example --library=psr7.

Settled then. I'll update this PR soon with default version (with guzzle as its easier to use than httplug) and add --library=psr7 later as separate feature.

@baartosz Guzzle is a better choice as you don't need an abstraction since PSR7 is your preferred abstraction, no need for an abstraction over an abstraction.

@baartosz want to test&adopt your httplug client + guzzle. Good time to make it a try? would I wait a little for your " I'll update this PR soon with default version.."

thx a lot for your work

@richhl guzzle6-only version is almost ready, it will be better to wait for it instead of reviewing and re-reviewing later.

baartosz · 2017-03-30T07:58:00Z

@wing328

Currently readme.md says that client works with php 5.4 which is not maintained for 1,5 years
http://php.net/supported-versions.php

Should default version work with
guzzle 5 which is not psr7-compatible to still have it working with php>=5.4?
or use guzzle 6 psr compatible but requiring php>=5.5 and drop 5.4 support?

arnested · 2017-03-30T08:05:40Z

I would say go for as new a version as needed. For this new way of doing it I don't see a need to forcefully stay behind on old PHP versions.

wing328 · 2017-03-30T09:04:45Z

Agreed with @arnested. @baartosz please feel free to drop 5.4 support.

If someone needs the PHP API client to work with PHP 5.4, they can always use Swagger Codegen v2.2.2 to generate the client.

richhl · 2017-03-30T14:24:23Z

modules/swagger-codegen/src/main/resources/php/Configuration.mustache

@@ -123,51 +109,6 @@ class Configuration
    protected $tempFolderPath;

    /**
-     * Indicates if SSL verification should be enabled or disabled.


any scheduled date? thx a lot

baartosz · 2017-03-30T17:17:44Z

Updated, rebased, tested with php 7 and 5.5, ready to review.
Httplug removed, guzzle6 added, required php increased to 5.5.

Unfortunately python test suite is breaking build for some reason.

[INFO] Python Swagger Petstore Client .................... FAILURE [20.635s]

wing328 · 2017-03-30T17:32:35Z

@baartosz don't worry about the Python test failure. I've fixed it in the master. (I'll run some tests later and let you know if I've any feedback)

richhl · 2017-03-31T08:47:44Z

do you want us to test @baartosz ? any guidance to test it? Thx in advance.

…

___________________________ Richard C. Hidalgo Lorite Director de Sistemas ISI Argonauta, S.L. C/Alcalá 196, apto. 13, 28028 Madrid Telf: +34 691 502 235 http://www.argonauta.org 2017-03-30 19:17 GMT+02:00 baartosz <notifications@github.com>:

Updated, rebased, tested with php 7 and 5.5, ready to review. Httplug removed, guzzle6 added, required php increased to 5.5. Unfortunately python test suite is breaking build for some reason. [INFO] Python Swagger Petstore Client .................... FAILURE [20.635s] — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5190 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBlDpFdWB23-vgCYInJym82cp51YyzUks5rq-O8gaJpZM4MorY-> .

wing328 · 2017-03-31T08:51:27Z

@richhl here are the steps to test it

git checkout -b baartosz-php_httplug2 2.3.0
git pull https://github.com/baartosz/swagger-codegen.git php_httplug2
mvn clean package

Then you can use the JAR to generate PHP API client by providing a spec.

wing328 · 2017-04-01T08:36:35Z

The Travis CI tests passed: https://travis-ci.org/swagger-api/swagger-codegen/builds/217127129

wing328 · 2017-04-01T09:54:04Z

I used PHP CodeSniffer to inspect the code and found some enhancements (code format) we can make to the PHP API client (this PR and master). I'll create a separate task for tracking.

…guration

…ialization in some tests

baartosz · 2017-04-03T07:15:05Z

just a rebase to solve conflicts

wing328 · 2017-04-03T07:48:29Z

@baartosz PR merged into 2.3.0. Thanks for the huge enhancement!

I'm sure the community appreciates your effort.

dkarlovi · 2017-04-03T07:59:12Z

@baartosz awesome work!

baartosz · 2017-04-03T08:12:43Z

@dkarlovi @wing328 that was quick. Did you even review the code? ;)

Thanks for your time and input!

richhl · 2017-04-03T08:15:06Z

don't worry @baartosz! hundred of eyes and hands are ready now to test and contribute changes if any.

we expect to test this week and next. will get in touch. thx 4 your work

wing328 · 2017-04-03T08:23:22Z

@baartosz I did review the code and play around with it over the weekend.

Like any other generators, I would expect it to improve over time thanks to the vibrant community.

wing328 · 2017-05-02T11:17:54Z

FYI. Updated README to include @baartosz as the template owner of PHP Guzzle

Robert430404 · 2017-05-18T19:07:19Z

Hello,

Not sure if this is the right place, but I'm wanting to use the template specified in this PR and I can't find any documentation on how I go about using it. If you guys could point me in the right direction, I would really appreciate it!

Thanks in advance,
Robert

wing328 · 2017-05-19T07:14:20Z

@Robert430404 Please open a new issue with the question instead.

wing328 added the Client: PHP label Mar 25, 2017

wing328 added this to the v2.2.3 milestone Mar 25, 2017

wing328 added the Enhancement: Feature label Mar 25, 2017

wing328 mentioned this pull request Mar 25, 2017

[PHP] update HTTP library to use Guzzle #1482

Closed

wing328 reviewed Mar 28, 2017

View reviewed changes

richhl reviewed Mar 30, 2017
View reviewed changes

baartosz changed the title ~~Php client using httplug & psr7 instead of curl~~ Php client using guzzle6 & psr7 instead of curl Mar 31, 2017

wing328 mentioned this pull request Apr 1, 2017

[PHP] Better code format #5277

Open

1 task

Bartosz Bialek added 4 commits April 3, 2017 08:08

reimplemented basic requests with httpplug

6e396f1

added returning headers

5691bcd

added query params support

af05bed

removed constant reference to model class

2147b5e

Bartosz Bialek added 10 commits April 3, 2017 08:12

made all methods static in ObjectSerializer

cea2693

updated test.php, replaced autoload.php with composer's autoloader

4355ec9

updated api doc template

aef17cf

removed classes used for testing; regenerated Fake_classname_tags123Api

da56526

replaced httplug with guzzle6

f8fe1d9

updated required php version to 5.5

124d2f4

clean up

c1f5748

readded missing userAgent feature; removed default headers from Confi…

6ce64f9

…guration

updated test.php

0a83557

downgraded phpunit back to 4.8 to work with php5.5; fixed client init…

54b2ee7

…ialization in some tests

wing328 merged commit 2315ef2 into swagger-api:2.3.0 Apr 3, 2017

baartosz mentioned this pull request Apr 3, 2017

[PHP] Add option to generate client without Guzzle #5305

Open

baartosz mentioned this pull request Apr 3, 2017

[PHP] Add support for async requests #5306

Closed

wing328 changed the title ~~Php client using guzzle6 & psr7 instead of curl~~ [PHP] using guzzle6 & psr7 instead of curl Apr 4, 2017

wing328 changed the title ~~[PHP] using guzzle6 & psr7 instead of curl~~ [PHP] API client using guzzle6 & psr7 instead of curl Apr 4, 2017

dkarlovi mentioned this pull request May 2, 2017

[PHP] add Accept encoding using CURLOPT_ENCODING #5525

Merged

3 tasks

Robert430404 mentioned this pull request May 19, 2017

[PHP] Cant Use Guzzle PHP Generation #5671

Closed

baartosz mentioned this pull request May 24, 2017

[php] 2.3 - fix form params json encoding #5701

Merged

3 tasks

wing328 modified the milestones: v2.3.0, v2.2.3 Jun 5, 2017

wing328 added the Breaking change (without fallback) label Dec 19, 2017

[PHP] API client using guzzle6 & psr7 instead of curl #5190

[PHP] API client using guzzle6 & psr7 instead of curl #5190

Conversation

baartosz commented Mar 24, 2017 • edited

PR checklist

Description of the PR

wing328 commented Mar 25, 2017

wing328 commented Mar 25, 2017

dkarlovi commented Mar 25, 2017

dkarlovi commented Mar 25, 2017

baartosz commented Mar 25, 2017

dkarlovi commented Mar 25, 2017

dkarlovi commented Mar 25, 2017

baartosz commented Mar 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baartosz Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baartosz Mar 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baartosz Mar 30, 2017 • edited

Choose a reason for hiding this comment

baartosz commented Mar 30, 2017 • edited

arnested commented Mar 30, 2017

wing328 commented Mar 30, 2017

richhl Mar 30, 2017 via email

Choose a reason for hiding this comment

baartosz commented Mar 30, 2017

wing328 commented Mar 30, 2017 • edited

richhl commented Mar 31, 2017 via email

wing328 commented Mar 31, 2017

wing328 commented Apr 1, 2017

wing328 commented Apr 1, 2017

baartosz commented Apr 3, 2017

wing328 commented Apr 3, 2017 • edited

dkarlovi commented Apr 3, 2017

baartosz commented Apr 3, 2017

richhl commented Apr 3, 2017

wing328 commented Apr 3, 2017

wing328 commented May 2, 2017

Robert430404 commented May 18, 2017

wing328 commented May 19, 2017

baartosz commented Mar 24, 2017 •

edited

baartosz commented Mar 25, 2017 •

edited

baartosz Mar 29, 2017 •

edited

baartosz Mar 29, 2017 •

edited

baartosz Mar 30, 2017 •

edited

baartosz commented Mar 30, 2017 •

edited

wing328 commented Mar 30, 2017 •

edited

wing328 commented Apr 3, 2017 •

edited