Problem/Motivation

In #3104353: Upgrade to Guzzle 7 we discovered that, aside from #3187315: Remove mink-goutte-driver as a core dependency, the only change required to make Drupal 9 compatible with Guzzle 7 is a small change to what exception we catch when handling request errors.

Both Guzzle\Exception\RequestException and Guzzle\Exception\ConnectException exist in both Guzzle 6 and 7. However, in Guzzle 6, ConnectException extends RequestException, whereas in Guzzle 7 they are siblings that implement different interfaces.

Both exceptions extend TransferException and are the only two types of TransferException within Guzzle itself. Guzzle itself also never calls TransferException directly. Furthermore, every usage we have of RequestException is catching it to log it, re-throw it as something else, set an error condition, etc. and so all those places are basically already allowing both RequestException and ConnectException since it's a child.

Proposed resolution

In most cases, catch TransferException instead. This will usually give us the same behavior unless the code specifically relies on the APIs of RequestException. In the latter case, we just need to add code to handle RequestException and Connectexception separately.

Remaining tasks

  • MR incoming.
  • It will need a CR, but I think it's non-disruptive enough that it doesn't need a release note.

API changes

Slight, backwards-compatible changes to Guzzle exception handling.

Release notes snippet

TBD if needed

Issue fork drupal-3188920

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:

Comments

xjm created an issue. See original summary.

xjm’s picture

Status: Active » Needs review
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Assigned: xjm » Unassigned
Issue tags: -Needs change record

Updated the @todo and added a CR: https://www.drupal.org/node/3188960

This is ready for review.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

I think that the choosen solution from the IS is the right solution.
All the changes from the MR from #3104353: Upgrade to Guzzle 7 regarding the change from RequestException to TransferException are from that MR. In that MR is also the change to Guzzle 7.2.0 and the testbot returns green for the MR.
In the MR from this issue those same change are tested with Guzzle 6.5 and the testbot returns green.
The text in the CR looks good to me.
For me it is RTBC.

alexpott’s picture

Issue tags: +Needs followup

I think in D10 we need to change much of this to use \Psr\Http\Client\ClientExceptionInterface instead but we can't do that in D9 because of needing to be Guzzle 6 compatible. We should open a follow-up to replace TransferException with that.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ee0a221 and pushed to 9.2.x. Thanks!

  • alexpott committed ee0a221 on 9.2.x
    Issue #3188920 by xjm: Make Guzzle exception handling forward-compatible...
alexpott’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.