alphagov / notifications-php-client

PHP client for the GOV.UK Notify API
https://docs.notifications.service.gov.uk/php.html
MIT License
11 stars 13 forks source link

PSR7 compatibility #119

Closed danielcarney96 closed 1 year ago

danielcarney96 commented 1 year ago

What problem does the pull request solve?

This solves the PSR7 compliance errors by updating the version of guzzlehttp/psr7.

The guzzle6 adapter still caused errors even after updating the the latest version, so it was also necessary to switch this with guzzle7.

Checklist

samuelhwilliams commented 1 year ago

Hey 👋 Thanks for putting this up, and sorry for the slow response.

I think this would need to be a major version bump rather than a minor one, as users of the library would need to update the code that instantiates the client to use Guzzle7 rather than Guzzle6, right?

        'httpClient' => new \Http\Adapter\Guzzle7\Client
danielcarney96 commented 1 year ago

Good point @samuelhwilliams, have updated that. Also have changed the documentation guzzle header to v7 rather than v6 since v6 doesn't seem to work. v5 docs are just below that too, not sure if you want that removing or not.

samuelhwilliams commented 1 year ago

Do you mind sharing the errors that you were getting with the guzzle6 adapter?

Also, do you have thoughts on whether we should continue to support the guzzle5-adapter and the implications of removing it? I'm afraid I don't have a huge amount of PHP experience so will need to bring myself up to speed. Any info you can share to help that would be appreciated :)

danielcarney96 commented 1 year ago

guzzle6-adapter and guzzle5-adapter aren't PSR7 compliant but this library has a PSR7 requirement so they can't be installed. Since they fail to install on the latest PHP version, they shouldn't be supported on master.

Heres an example of an error when running composer require php-http/guzzle6-adapter alphagov/notifications-php-client on PHP 8.1

Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - alphagov/notifications-php-client dev-master requires guzzlehttp/psr7 ^2.4.3 -> satisfiable by guzzlehttp/psr7[2.4.3, 2.4.x-dev].
    - php-http/guzzle6-adapter[v2.0.0, ..., v2.0.1] require php ^7.1 -> your php version (8.1.10) does not satisfy that requirement.
    - Root composer.json requires alphagov/notifications-php-client dev-master -> satisfiable by alphagov/notifications-php-client[dev-master].
    - php-http/guzzle6-adapter[v2.0.2, ..., 2.x-dev] require guzzlehttp/guzzle ^6.0 -> satisfiable by guzzlehttp/guzzle[6.0.0, ..., 6.5.x-dev].
    - guzzlehttp/guzzle[6.5.8, ..., 6.5.x-dev] require guzzlehttp/psr7 ^1.9 -> satisfiable by guzzlehttp/psr7[1.9.0, 1.x-dev].
    - guzzlehttp/guzzle[6.4.0, ..., 6.5.7] require guzzlehttp/psr7 ^1.6.1 -> satisfiable by guzzlehttp/psr7[1.6.1, ..., 1.x-dev].
    - guzzlehttp/guzzle[6.2.3, ..., 6.3.3] require guzzlehttp/psr7 ^1.4 -> satisfiable by guzzlehttp/psr7[1.4.0, ..., 1.x-dev].
    - guzzlehttp/guzzle[6.2.1, ..., 6.2.2] require guzzlehttp/psr7 ^1.3.1 -> satisfiable by guzzlehttp/psr7[1.3.1, ..., 1.x-dev].
    - guzzlehttp/guzzle[6.0.2, ..., 6.2.0] require guzzlehttp/psr7 ~1.1 -> satisfiable by guzzlehttp/psr7[1.1.0, ..., 1.x-dev].
    - guzzlehttp/guzzle[6.0.0, ..., 6.0.1] require guzzlehttp/psr7 ^1.0.0 -> satisfiable by guzzlehttp/psr7[1.0.0, ..., 1.x-dev].
    - You can only install one version of a package, so only one of these can be installed: guzzlehttp/psr7[1.0.0, ..., 1.x-dev, 2.0.0-beta1, ..., 2.4.x-dev].
    - Root composer.json requires php-http/guzzle6-adapter ^2.0 -> satisfiable by php-http/guzzle6-adapter[v2.0.0, v2.0.1, v2.0.2, 2.x-dev].
samuelhwilliams commented 1 year ago

I've squashed your commits and put them up in https://github.com/alphagov/notifications-php-client/pull/121 for our CI to run against. Going to close this - but very grateful for your contribution and kicking off this discussion. Let's move chat there if needed :)

samuelhwilliams commented 1 year ago

edit: moving to open PR 🤦