Skip to content

Upgrade php-http integration #257

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

Merged
merged 6 commits into from
Jan 4, 2016
Merged

Upgrade php-http integration #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
465c35c
Upgrade php-http integration
dbu Dec 4, 2015
3113dc3
Move mock client to php-http/mock-client
ddeboer Dec 29, 2015
156058d
Improve wording in docs
ddeboer Dec 29, 2015
a3559b1
Drop PHP 5.4 support
ddeboer Dec 31, 2015
342e4e5
Add Puli dependency for php-http/discovery
ddeboer Dec 31, 2015
2459eb1
Set correct minimum versions for --prefer-lowest
ddeboer Dec 31, 2015
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ vendor/
composer.lock
phpunit.xml
doc/_build/
puli.json
.puli/
5 changes: 1 addition & 4 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
language: php

php:
- 5.4
- 5.5
- 5.6
- hhvm
Expand All @@ -12,7 +11,7 @@ env:

matrix:
include:
- php: 5.4
- php: 5.5
env: SYMFONY_VERSION=2.3.* VARNISH_VERSION=3.0 COMPOSER_FLAGS="--prefer-lowest"

branches:
Expand All @@ -22,8 +21,6 @@ branches:
- '/^\d+\.\d+$/'

install:
- 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
- composer update $COMPOSER_FLAGS --prefer-source --no-interaction

before_script:
Expand Down
18 changes: 10 additions & 8 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,22 @@
"homepage": "https://github.com/friendsofsymfony/FOSHttpCache/contributors"
}
],
"minimum-stability": "dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed because of puli dependency? could it be avoided by using ^1.0.0@beta or similar?

Copy link

Choose a reason for hiding this comment

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

It doesn't matter, it is not applied in packages where this is installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but while rather remote because of the "prefer-stable" flag, it looked like it might cause other package versions to be pulled in on travis tests on this repo as opposed to what will be pulled in on project installs

Copy link

Choose a reason for hiding this comment

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

as opposed to what will be pulled in on project installs

For the time being php http packages are mostly unstable as well, so you are right, without relying on development versions, not the same packages will be installed. Wonder how we could improve that until stable versions are out.

Copy link
Member

Choose a reason for hiding this comment

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

This minimum-stability is necessary because some of the php-http packages themselves require unstable versions of other php-http packages. If your minimum-stability is stable (the default) Composer won’t find them.

When stable versions are available, they will be installed instead: see "prefer-stable": true one line below.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the time being php http packages are mostly unstable as well,...

This minimum-stability is necessary because some of the php-http packages themselves require unstable versions of other php-http packages...

Any chance those will be stable in the near future (by the time 2.0 of FOSHttpCache is stable)?
Otherwise might have to document which packages are unstable in order for people to specify every single one in project install to avoid having to set minimum-stability to dev.

Copy link

Choose a reason for hiding this comment

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

Yes, that's the plan (since we are the author of php-http packages as well 😄 )

Copy link
Member

Choose a reason for hiding this comment

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

@andrerom Definitely. FOSHttpCache 2.0 will not ship before we can remove the minimum-stability flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

"prefer-stable": true,
"require": {
"php": ">=5.4.8",
"php": "^5.5.9||^7.0.0",
"symfony/event-dispatcher": "^2.3||^3.0",
"symfony/options-resolver": "^2.3||^3.0",
"psr/http-message-implementation": "~1.0",
"php-http/adapter-implementation": "^0.1.0",
"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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

"php-http/message": "^0.2.1",
"puli/cli": "^1.0.0-beta8"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. just move it from require to suggest and document it properly; and maybe add conditional checks around the Discovery calls in our code
  2. 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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

no questions asked 😄

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dbu dbu Jan 3, 2016 via email

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

php-http/discovery#39

},
"require-dev": {
"mockery/mockery": "~0.9.1",
"monolog/monolog": "~1.0",
"php-http/guzzle6-adapter": "^0.1.0",
"guzzlehttp/psr7": "^1.0",
"monolog/monolog": "^1.0",
"php-http/guzzle6-adapter": "^0.3.1",
"php-http/mock-client": "~0.1",
"symfony/process": "^2.3||^3.0",
"symfony/http-kernel": "^2.3||^3.0"
},
Expand Down
7 changes: 2 additions & 5 deletions doc/contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,9 @@ The library also includes functional tests against a Varnish and NGINX instance.
The functional test suite by default uses PHP’s built-in web server. If you have
PHP 5.4 or newer, simply run with the default configuration.

If you want to run the tests on PHP 5.3, you need to configure a web server
If you want to run the tests on HHVM_, you need to configure a web server
listening on localhost:8080 that points to the folder
``tests/Functional/Fixtures/web``.

If you want to run the tests on HHVM_, you need to configure a web server and
start a `HHVM FastCGI server`_.
``tests/Functional/Fixtures/web`` and start a `HHVM FastCGI server`_.

To run the functional tests:

Expand Down
18 changes: 8 additions & 10 deletions doc/installation.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,20 @@ Installation
------------

The FOSHttpCache library is available on Packagist_. You can install it using
Composer_:
Composer_.

.. code-block:: bash
The library relies on HTTPlug_ for sending invalidation requests over HTTP, so
you need to install an HTTPlug-compatible client or adapter first:

$ composer require friendsofsymfony/http-cache
.. code-block:: bash

Note that the library needs a ``psr/http-message-implementation`` and
``php-http/adapter-implementation``. If your project does not contain one,
composer will complain that it did not find ``psr/http-message-implementation``.
$ composer require php-http/guzzle6-adapter

When on PHP 5.5+, use the following line instead:
Then install the FOSHttpCache library itself:

.. code-block:: bash

$ composer require friendsofsymfony/http-cache guzzlehttp/psr7:^1.0 php-http/guzzle6-adapter:^0.1.0

On PHP 5.4, the ``php-http/guzzle5-adapter:^0.1.0`` works fine.
$ composer require friendsofsymfony/http-cache

.. note::

Expand Down Expand Up @@ -61,3 +58,4 @@ invalidation requests:
.. _Packagist: https://packagist.org/packages/friendsofsymfony/http-cache
.. _Composer: http://getcomposer.org
.. _Semantic Versioning: http://semver.org/
.. _HTTPlug: http://httplug.io
38 changes: 22 additions & 16 deletions doc/proxy-clients.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,21 @@ which caching solution you use.
Setup
-----

HTTP Adapter Installation
~~~~~~~~~~~~~~~~~~~~~~~~~
HTTP Client Installation
~~~~~~~~~~~~~~~~~~~~~~~~

Because the clients send invalidation requests over HTTP, an `HTTP adapter`_
must be installed. Which one you need depends on the HTTP client library that
you use in your project. For instance, if you use Guzzle 6 in your project,
install the appropriate adapter:
Because the clients send invalidation requests over HTTP, an `HTTPlug client`_
must be installed. Pick either a standalone client or an adapter to a client
library that is already included in your project. For instance, if you use
Guzzle 6 in your project, install the appropriate adapter:

.. code-block:: bash

$ composer require php-http/guzzle6-adapter

You also need a `PSR-7 message implementation`_. If you use Guzzle 6, Guzzle’s
implementation is already included. If you use another client, install one of
the implementations. Recommended:
implementation is already included. If you use another client, you need to
install one of the message implementations. Recommended:

.. code-block:: bash

Expand All @@ -40,13 +40,13 @@ Alternatively:

.. _HTTP adapter configuration:

HTTP Adapter Configuration
~~~~~~~~~~~~~~~~~~~~~~~~~~
HTTP Client Configuration
~~~~~~~~~~~~~~~~~~~~~~~~~

By default, the proxy client will find the adapter that you have installed
through Composer. But you can also pass the adapter explicitly. This is most
useful when you have created a HTTP client with custom options or middleware
(such as logging)::
By default, the proxy client will automatically locate an HTTP client that you
have installed through Composer. But you can also pass the adapter explicitly.
This is most useful when you have created a HTTP client with custom options or
middleware (such as logging)::

use GuzzleHttp\Client;

Expand All @@ -66,6 +66,13 @@ Then pass that adapter to the caching proxy client::
$proxyClient = new Varnish($servers, '/baseUrl', $adapter);
// Varnish as example, but also possible for NGINX and Symfony

HTTP Message Factory Configuration
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Similar to the HTTP client, the HTTP message factory is automatically located
by default. You can pass an explicit instance of the message factory if you
need to.

.. _varnish client:

Varnish Client
Expand Down Expand Up @@ -276,6 +283,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
.. _HTTP Adapter: http://php-http.readthedocs.org/en/latest/
.. _HTTPlug client: http://httplug.io/
.. _PSR-7 message implementation: https://packagist.org/providers/psr/http-message-implementation
8 changes: 4 additions & 4 deletions src/Exception/ProxyUnreachableException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

namespace FOS\HttpCache\Exception;

use Http\Adapter\Exception\HttpAdapterException;
use Http\Client\Exception\RequestException;

/**
* Thrown when a request to the reverse caching proxy fails to establish a
Expand All @@ -20,18 +20,18 @@
class ProxyUnreachableException extends \RuntimeException implements HttpCacheExceptionInterface
{
/**
* @param HttpAdapterException $adapterException
* @param RequestException $adapterException
*
* @return ProxyUnreachableException
*/
public static function proxyUnreachable(HttpAdapterException $adapterException)
public static function proxyUnreachable(RequestException $adapterException)
{
$message = sprintf(
'Request to caching proxy at %s failed with message "%s"',
$adapterException->getRequest()->getHeaderLine('Host'),
$adapterException->getMessage()
);

return new ProxyUnreachableException(
$message,
0,
Expand Down
Loading