Upgrade php-http integration #257

dbu · 2015-12-04T13:29:19Z

trying to adjust to the current versions of php-http.

Tests fail because of problem with error response conversion
Some TODO in the code

dbu · 2015-12-04T13:31:49Z

src/ProxyClient/Http/HttpAdapterInterface.php

+ *
+ * @author David Buchmann <mail@davidbu.ch>
+ */
+interface HttpAdapterInterface


not sure if we should call this adapter, but turns out we do need some layer between the proxy client and Httplug, or we would need to duplicate logic or share it in a base class.

I agree, it should not be Http related at all. For example in FXMLRPC, http based client is just a transfer object. It could be SSH as well, but that's xmlrpc specific.

So this should be a generic interface with a naming which explains what it does (Invalidator?). The actual implementation can be Http dependent. Even if it does not make sense to invalidate without http, from an abstract point of view, it doesn't matter.

dbu · 2015-12-07T13:40:28Z

doc/proxy-clients.rst

@@ -276,6 +285,5 @@ Varnish client::
 Make sure to add any headers that you want to ban on to your
 :doc:`proxy configuration <proxy-configuration>`.

-.. _header: http://php.net/header


i did not see this being used anywhere in the doc.

dbu · 2015-12-07T13:55:20Z

@sagikazarmark when looking at the travis fails, i think we have a problem with the guzzle6 adapter. created php-http/guzzle6-adapter#13 to discuss that further

ddeboer · 2015-12-07T20:04:05Z

src/ProxyClient/Http/HttpAdapter.php

+        $exceptions = new ExceptionCollection();
+
+        foreach ($queue as $request) {
+            foreach ($this->multiplex($request) as $proxyRequest) {


I’m not sure ‘multiplex’ is the right terminology. Perhaps what you mean is inverse multiplexing? Unfortunately, that sounds a little convoluted, so perhaps just fanOut() or something would be better? Anyway, you’re the networking expert. 😉

i am fine with fanOut

dbu · 2015-12-22T12:23:02Z

the guzzle setup has been fixed: php-http/guzzle6-adapter#19

ddeboer · 2015-12-29T18:58:57Z

This is green for guzzle6-adapter now. Fixing guzzle5-adapter installation in php-http/guzzle5-adapter#9.

ddeboer · 2015-12-30T11:13:13Z

.travis.yml

-  - if [[ "$TRAVIS_PHP_VERSION" == "5.4" || "$TRAVIS_PHP_VERSION" == "hhvm" ]]; then composer remove "php-http/guzzle6-adapter" --dev --no-update; fi
-  - if [[ "$TRAVIS_PHP_VERSION" == "5.4" || "$TRAVIS_PHP_VERSION" == "hhvm" ]]; then composer require "php-http/guzzle5-adapter" --dev --no-update; fi
+  - if [[ "$TRAVIS_PHP_VERSION" == "5.4" ]]; then composer remove "php-http/guzzle6-adapter" --dev --no-update; fi
+  - if [[ "$TRAVIS_PHP_VERSION" == "5.4" ]]; then composer require "php-http/guzzle5-adapter" --dev --no-update; fi


Guzzle 6 now correctly runs on hhvm, yay!

ddeboer · 2015-12-30T11:14:34Z

Okay, everything is now green except PHP 5.4. This is caused by the current php-http/guzzle5-adapter not offering async. We could fix this, of course, but I’d actually opt for dropping 5.4 support: it’s been EOL for quite some time, and big ones like Symfony 3.0 don’t support it any longer either. If we drop 5.4, we don’t have to test against Guzzle 5 anymore.

@dbu Do you agree?

dbu · 2015-12-31T13:05:17Z

yup, sounds good. lets ditch 5.4 and go for 5.5. we are talking version 2.0 so lets not burden us with things that are already legacy.

convert to options array

Fix Symfony 2.3-incompatible calls

ddeboer · 2016-01-01T16:52:51Z

composer.json

+        "php-http/client-implementation": "^1.0.0",
+        "php-http/discovery": "^0.6.0",
+        "php-http/message": "^0.2.1",
+        "puli/cli": "^1.0.0-beta8"


Adding Puli as a dependency so php-http/discovery works out of the box.

I would rather add this as a development dependency and document that Puli is necessary. At least it seemed to me as a recommendation from Puli. Maybe @webmozart could help in in this question.

and puli is only required when not injecting a http client explicitly. maybe we should move discovery to optional. does discovery only work with puli but not require it? that would feel wrong to me.

@dbu Yes, Discovery should be optional with FOSHttpCache. We can make it so in two ways:

just move it from require to suggest and document it properly; and maybe add conditional checks around the Discovery calls in our code

remove all Discovery calls from our code and document that users should either pass an HTTP client explicitly or do the discovery in their own code.

My beef with 2 and @sagikazarmark’s idea to have users install Puli is that it becomes a lot more work for users to get started with FOSHttpCache.

For discovery’s Puli dependency, see php-http/discovery#36.

No, Puli is not optional. All the relevant Puli dependencies are required in the discover package. The cli is something that is up to the user: it can be installed as a phar globally in the prefix, or installed with composer in the application.

I guess there is nothing wrong in adding it as a dependency here. But I guess some time in the future having puli.phar in the users prefix will be as common as composer.

@dbu No problem. 😉

How will we then handle Discovery here?

either we require it, no questions asked

or we suggest it and add a note to our docs.

no questions asked 😄

Ah, sorry, missed the question, I thought it's still discovery-puli.

imho it would make more sense if php-http/discovery would have the cli as requirement than we have here. then discovery could be either suggestion or require, but only one thing not two.

php-http/discovery#39

ddeboer · 2016-01-01T16:55:13Z

We have a green Travis build. Scrutinizer still fails but I’d like to fix that on a separate PR. Squashed commits, so this is ready for merging.

andrerom · 2016-01-02T13:54:12Z

composer.json

    "require": {
-        "php": ">=5.4.8",
+        "php": ">=5.5.9",


nitpick, but could be ^5.5.9|^7.0 to be more explicit on future versions.

Good point, it seems to becoming the standard now.

dbu · 2016-01-02T16:39:18Z

composer.json

-        "php-http/discovery": "^0.1.1",
-        "php-http/message-decorator": "^0.1.0"
+        "php-http/client-implementation": "^1.0.0",
+        "php-http/discovery": "^0.6.0",


if the discovery is not usable without running a puli cli command or adding that command to the project's post-install commands in composer, does it make sense to require the discovery at all? or should we just document how to use discovery and puli for 0-config when wanted?

That's what I would like to achieve: php-http/discovery#33

Upgrade php-http integration

dbu · 2016-01-04T10:40:37Z

yay!

lets continue to tweak things, but i wanted to merge now as this PR was getting rather large

sagikazarmark · 2016-01-04T10:41:25Z

Discovery 0.6.2 tagged

ddeboer · 2016-01-04T11:41:13Z

👍

dbu mentioned this pull request Dec 4, 2015

Test against Symfony 3.0 FriendsOfSymfony/FOSHttpCacheBundle#255

Closed

dbu reviewed Dec 4, 2015
View reviewed changes

dbu force-pushed the adjust-php-http branch from 091f5e1 to 728dbad Compare December 7, 2015 13:39

dbu reviewed Dec 7, 2015
View reviewed changes

dbu force-pushed the adjust-php-http branch from 728dbad to ca404af Compare December 7, 2015 13:44

dbu mentioned this pull request Dec 7, 2015

how to set up guzzle php-http/guzzle6-adapter#13

Closed

dbu mentioned this pull request Dec 7, 2015

capability discovery php-http/httplug#88

Closed

ddeboer reviewed Dec 7, 2015
View reviewed changes

dbu mentioned this pull request Dec 22, 2015

Call to undefined method ...CacheManager::addSubscriber() FriendsOfSymfony/FOSHttpCacheBundle#260

Closed

ddeboer force-pushed the adjust-php-http branch from 561af35 to bc2d405 Compare December 29, 2015 18:27

ddeboer force-pushed the adjust-php-http branch from 049381b to b97f000 Compare December 29, 2015 19:00

ddeboer reviewed Dec 30, 2015
View reviewed changes

sagikazarmark mentioned this pull request Dec 31, 2015

Introducing HTTPlug thephpleague/oauth2-client#475

Closed

ddeboer force-pushed the adjust-php-http branch 3 times, most recently from e5244d8 to 1f1f01d Compare January 1, 2016 14:41

ddeboer force-pushed the adjust-php-http branch 9 times, most recently from 216eeb1 to 7aa8e47 Compare January 1, 2016 16:33

dbu and others added 3 commits January 1, 2016 17:45

Upgrade php-http integration

465c35c

convert to options array

Move mock client to php-http/mock-client

3113dc3

Improve wording in docs

156058d

ddeboer force-pushed the adjust-php-http branch from 7aa8e47 to 7486bfa Compare January 1, 2016 16:47

ddeboer changed the title ~~WIP: upgrade php-http integration~~ Upgrade php-http integration Jan 1, 2016

ddeboer added 2 commits January 1, 2016 17:49

Drop PHP 5.4 support

a3559b1

Fix Symfony 2.3-incompatible calls

Add Puli dependency for php-http/discovery

342e4e5

ddeboer force-pushed the adjust-php-http branch from 7486bfa to 03c331a Compare January 1, 2016 16:51

ddeboer reviewed Jan 1, 2016
View reviewed changes

andrerom reviewed Jan 2, 2016
View reviewed changes

Set correct minimum versions for --prefer-lowest

2459eb1

ddeboer force-pushed the adjust-php-http branch from 03c331a to 2459eb1 Compare January 2, 2016 14:17

dbu reviewed Jan 2, 2016
View reviewed changes

dbu added a commit that referenced this pull request Jan 4, 2016

Merge pull request #257 from FriendsOfSymfony/adjust-php-http

f976903

Upgrade php-http integration

dbu merged commit f976903 into master Jan 4, 2016

dbu deleted the adjust-php-http branch January 4, 2016 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade php-http integration #257

Upgrade php-http integration #257

dbu commented Dec 4, 2015

dbu Dec 4, 2015

sagikazarmark Dec 4, 2015

dbu Dec 7, 2015

ddeboer Jan 1, 2016

dbu commented Dec 7, 2015

ddeboer Dec 7, 2015

dbu Dec 8, 2015 via email

dbu commented Dec 22, 2015

ddeboer commented Dec 29, 2015

ddeboer Dec 30, 2015

ddeboer commented Dec 30, 2015

dbu commented Dec 31, 2015 via email

ddeboer Jan 1, 2016

sagikazarmark Jan 1, 2016

dbu Jan 2, 2016

ddeboer Jan 2, 2016

sagikazarmark Jan 2, 2016

ddeboer Jan 3, 2016

sagikazarmark Jan 3, 2016

sagikazarmark Jan 3, 2016

dbu Jan 3, 2016 via email

dbu Jan 4, 2016

ddeboer commented Jan 1, 2016

andrerom Jan 2, 2016

ddeboer Jan 2, 2016

dbu Jan 2, 2016

sagikazarmark Jan 2, 2016

dbu commented Jan 4, 2016

sagikazarmark commented Jan 4, 2016

ddeboer commented Jan 4, 2016

Upgrade php-http integration #257

Upgrade php-http integration #257

Conversation

dbu commented Dec 4, 2015

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

dbu commented Dec 7, 2015

Choose a reason for hiding this comment

dbu Dec 8, 2015 via email

Choose a reason for hiding this comment

dbu commented Dec 22, 2015

ddeboer commented Dec 29, 2015

Choose a reason for hiding this comment

ddeboer commented Dec 30, 2015

dbu commented Dec 31, 2015 via email

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

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

dbu Jan 3, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddeboer commented Jan 1, 2016

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

dbu commented Jan 4, 2016

sagikazarmark commented Jan 4, 2016

ddeboer commented Jan 4, 2016