Problem/Motivation

https://github.com/guzzle/guzzle/releases/tag/8.0.0

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3612544

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

godotislate’s picture

godotislate
he/him
commented
Related issues: +#3605380: Resolve GuzzleHttp\ClientInterface::getConfig() deprecations

This might be blocked by #3605380: Resolve GuzzleHttp\ClientInterface::getConfig() deprecations.

Also php-http/guzzle7-adapter (https://github.com/php-http/guzzle7-adapter) does not have a Guzzle 8 compatible version. I have not looked into why that package is needed though.

longwave made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: Update to guzzle 8 » Update to Guzzle 8

php-http/guzzle7-adapter was added for the OpenTelemetry performance monitoring in #3352389: Add open-telemetry/sdk and open-telemetry/exporter-otlp as dev dependencies. There is a slim chance it is not needed any more?

longwave opened merge request !16404

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

getClient() remains on the concrete class, just not on the interface...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Parent issue: » #3449808: [12.x] [meta] Requirements for tagging 12.0.0-beta1
godotislate’s picture

godotislate
he/him
commented

MR looks good, one Q about the exception handling. It's not clear to me which Guzzle exception class in the hierarchy should be caught.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The tree is

  • ResponseException
    • BadResponseException (4xx or 5xx)
      • ClientException (4xx)
      • ServerException (5xx)
    • TooManyRedirectsException
    • ResponseTransferException
    • ResponseTimeoutException

It feels like we should only be catching 4xx/5xx here, so I went with that; I think the others are unlikely even in explicit tests of failure scenarios - only when something goes wrong with the test run itself - but happy to change this if I'm wrong!

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the explanation.

Thinking about this:

  • TooManyRedirectsException should never be thrown, since the the client configuration in the test has allow_redirects set to FALSE and Browserkit is supposed to handle the redirects instead of Guzzle. So we don't have to worry about it
  • ResponseTimeoutException probably is only a problem if there is a CI environment issue, in which case tests probably will fail anyway

Reading docs: https://github.com/guzzle/guzzle/blob/8.0/docs/contributing/exception-gu...

Use ResponseTransferException for transfer-level failures after response
headers were received and a response object exists. This includes response-aware
connection, network, protocol, content-decoding, partial-body, and response-body
transfer failures.

I think we can let this go uncaught.

lgtm

  • larowlan committed 6e1efec8 on main 
    task: #3612544 Update to Guzzle 8

By: catch
By: godotislate
By:...

larowlan closed merge request !16404

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to main - thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Hm I realised I left v7 available via composer.json but no idea if it will actually work due to the other changes in this MR, maybe we should only support V8 in D12?

catch’s picture

catch
he/him
Primary language English
commented

I think it would be fine to only support guzzle 8.

We might want to look into adding guzzle 8 support to Drupal 11 in case guzzle 7 support gets dropped given there's another 2.5 years to go before we drop Drupal 11 support?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Opened followups for both things as children of this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +12.0.0 release notes