guzzlehttp/psr7 version incompatible about notifications-php-client HOT 12 OPEN

Further to this, I assume the obvious fixes are to either:

• Increase the version number of guzzlehttp/psr7 in this package
• Decrease the version of php-http/curl-client at the higher level composer.json - composer defaults this to ^2.2 when following your documentation, using "composer require php-http/curl-client php-http/message alphagov/notifications-php-client" via the cli

Can you please confirm what is the best solution?

Thanks again,
Dave

from notifications-php-client.

Hi @365Dave,

Thank you for raising this and giving some potential fixes (super helpful for someone who is not a PHP expert).

Just to let you know that I've been able to replicate this and get the following error:

Fatal error: Uncaught TypeError: Argument 1 passed to Http\Client\Curl\Client::__construct() must be an instance of Psr\Http\Message\ResponseFactoryInterface or null, instance of Http\Message\MessageFactory\GuzzleMessageFactory given, called in /Users/davidmcdonald/php-test/test-send.php on line 9 and defined in /Users/davidmcdonald/php-test/vendor/php-http/curl-client/src/Client.php:79
Stack trace:
#0 /Users/davidmcdonald/php-test/test-send.php(9): Http\Client\Curl\Client->__construct(Object(Http\Message\MessageFactory\GuzzleMessageFactory), Object(Http\Message\StreamFactory\GuzzleStreamFactory))
#1 {main}
  thrown in /Users/davidmcdonald/php-test/vendor/php-http/curl-client/src/Client.php on line 79

I don't think there is anything too intentional going on here, this is likely a bug/mistake.

Let me do some further investigation and come back to you on what fix may be most appropriate!

from notifications-php-client.

Just a quick one before I head off for the weekend. I've tried your second suggested fix and downgraded to "php-http/curl-client": "1.7.1".

However I get a different error message this time.

Fatal error: Uncaught Alphagov\Notifications\Exception\InvalidArgumentException: An instance of HttpClientInterface must be set under 'httpClient' in /Users/davidmcdonald/php-test/vendor/alphagov/notifications-php-client/src/Client.php:125
Stack trace:
#0 /Users/davidmcdonald/php-test/test-send.php(9): Alphagov\Notifications\Client->__construct(Array)
#1 {main}
  thrown in /Users/davidmcdonald/php-test/vendor/alphagov/notifications-php-client/src/Client.php on line 125

We will have to look into this a bit more on Monday!

from notifications-php-client.

Hi @idavidmcdonald,

Thank you for your extremely quick response - I really appreciate it.

With regard to downgrading the "php-http/curl-client", I tried this straight after posting my message and I also got the same result. Worth a try never the less!

I will await your further findings with this issue.

Thanks again,
Dave

from notifications-php-client.

Hi @idavidmcdonald

I am also attempting to upgrade and I am hitting the same issue. We were advised to upgrade by 5th July due to infrastructure changes at notify.

Could you advise whether it will be possible to fix this issue by then? Or alternatively confirm the highest working version that will satisfy the requirement to upgrade. I have copied the message we received from Notify.

We haven't noticed an increase in failures, so perhaps we don't need to do anything at this point?

Message from Notify:

On Tuesday 5 July, we’re moving all our API traffic to AWS Cloudfront.

We’ve already migrated 95% of our traffic, and have been working with services to find and fix any issues.

If you’ve noticed more errors than usual in the last 2 months, please update your API integration before 5 July.

If you manage multiple services, you should check and update all API integrations.

If you rely on an external provider for managing your service(s), you should ask them to check the error rates for you.

Thanks,
Pete

from notifications-php-client.

Hi both,

I'm afraid I don't have an answer yet (I'm hoping to get some more done on this today/tomorrow).

However one thing I will just raise in case it is helpful which might take the pressure off upgrading.

You only need to upgrade your version of our PHP API client if you are currently seeing errors when sending requests to our API.

This is because 95% of your requests are already going through our new infrastructure and you would be seeing a large number of errors already if there was a problem.

The advice to upgrade is there because we are aware that in versions < 5.0.1 of our Python client, there is an issue that it sends GET requests with a body (albeit an empty body) which Cloudfront rejects. We are not aware of this issue or any other issues with any of our other clients or other versions. But if teams are seeing errors then upgrading your client is one approach to try and see if this solves it, in case you've stumbled across a different problem.

You are welcome to upgrade still when we get this fixed, but this is in your own time and there is no deadline from us.

Sorry this wasn't clearer in our email!

Do let me know if that takes the pressure off you to upgrade before 5 July please? And we will continue looking at a solution for this.

from notifications-php-client.

Thanks for getting back to us on this matter @idavidmcdonald.

Personally I have changed my implementation to the latest Guzzle v6 variant. This works well and caused little problems to integrate into my code.

Dave

from notifications-php-client.

Hi there,

Thanks for your reply @idavidmcdonald.

We don't see any errors with our current implementation, we only get an error when we do the update in our Drupal website.

We are already using Guzzlev6 and followed the instructions as outlined here https://docs.notifications.service.gov.uk/php.html#set-up-the-client but the suggested code
$notifyClient = new \Alphagov\Notifications\Client([ 'apiKey' => '{your api key}', 'httpClient' => new \Http\Adapter\Guzzle6\Client ]);
errors. It complains about not getting an HTTPClientInterface in httpClient.

This code was working just fine with the version 1.6.2 of the package.

Can you advise please?

Thanks,
Carole.

from notifications-php-client.

Hi,

So we've done some more digging on this.

We accidentally removed support for all versions of the curl client in version 3.0.0 of this API client

aac0cb7

In version 3.0.0 of this API client, we brought in a requirement for HTTP clients to be PSR7 compliant. We forgot to test whether the curl client would continue to work with this change. It turns out that the curl client is not PSR7 compliant. No version of the curl client has ever been PSR7 compliant, nor will it be (they have moved straight to PSR17 compliance, skipping PSR7).

This means that if you want to upgrade to version 3 or higher of our client, it is not currently possible to use any version of the curl client.

We plan to update our documentation to make it correct on what we currently support

This would involve dropping the documentation for using the curl client with our PHP client.

We have checked and our implementation for Guzzle 6 (using the Guzzle 6 adaptor to make it PSR7 compliant) still works.

We have checked and Guzzle 7 also works so we will add this to our documentation.

We need to check our implementation for Guzzle 5 (using the Guzzle 5 adaptor to make it PSR7 complaint) still works and update the documentation if needed.

We plan to put in some better automated testing to ensure that any future changes won't break integrations with any HTTP clients we are supporting.

We may be able to add support for the curl client

We think the only way to do this would be to make changes to our API client that make it PSR17 compliant. Both the latest versions of the curl client and Guzzle 7 appear to be PSR17 compliant.

This would however mean dropping support for Guzzle 5 and Guzzle 6 as neither are PSR17 compliant.

We haven't yet started looking at this. From the crude metric of github stars, Guzzle is about 60 times more popular than the curl client so there is a question about whether we should prioritise it (especially since no one has complained about it being broken for the last 2 years). We would be keen to hear your thoughts on this!

Thanks for your patience,
David

from notifications-php-client.

@c-ancia

Hi, would you be able to provide some more details please so we can try and replicate this locally. For example, what version of php-http/guzzle6-adapter are you running?

Also could you confirm if you have this running locally without errors but then running in Drupal with errors?

You said 'we only get an error when we do the update' which I wasn't entirely clear on what you mean by that?

Any further details would be much appreciated.

Thanks,
David

from notifications-php-client.

Hi @idavidmcdonald ,

So we are currently using "alphagov/notifications-php-client": "1.6.2" and "php-http/guzzle6-adapter": "v1.1.1".
We have an implementation of the Notify API inside Drupal which allows us to send email to subscribers. We also have a drush command to test if the notifications are properly being sent to a specific email address. I'm running this from my local environment but it's still a Drupal project, we don't have an independent implementation of this code.

In our current implementation, we're already using everything suggested in the documentation, including
`$notifyClient = new \Alphagov\Notifications\Client([ 'apiKey' => '{your api key}', 'httpClient' => new \Http\Adapter\Guzzle6\Client ]);``

And using these specific versions ("alphagov/notifications-php-client": "1.6.2" and "php-http/guzzle6-adapter": "v1.1.1"), everything works fine.

It's only following the update of the package "alphagov/notifications-php-client" to version 4.0.1 that we started to get the following error: An instance of HttpClientInterface must be set under 'httpClient'. This is triggered by the piece of code I mentioned above. The version of the "php-http/guzzle6-adapter" package hasn't changed.

Here's a shortened version of our code

`
use Alphagov\Notifications\Client as AlphaGovClient;
use Http\Adapter\Guzzle6\Client as GuzzleClient;

class DrushCustomCommand extends DrushCommands {

public function sendAlert($email) {
$api = new AlphaGovClient([
'apiKey' => getapikey(),
'httpClient' => new GuzzleClient(),
]);
[... send alert]
}
}
`

What I meant by 'we only get an error when we do the update' is that the code was working fine before doing the update of the alphagov package, which makes me think there might be an issue with the newer version, since this was the only change that happened.

Does that make sense? And I hope it makes everything a bit clearer.

Many thanks!
Carole.

from notifications-php-client.

Hi @c-ancia, I was able to replicate your issue. The reason for this is because of the change introduced to notifications-php-client in version 3.0 described by @idavidmcdonald above.

If you also update your php-http/guzzle6-adapter package to the latest version (^2.0), this adds the required compatibility to the Guzzle client which will work with the latest version of the Notify client. No changes to your code should be required.

from notifications-php-client.