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
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:
- 3188920-make-guzzle-exception
changes, plain diff MR !168
Comments
Comment #3
xjmComment #4
xjmComment #5
xjmComment #6
xjmUpdated the @todo and added a CR: https://www.drupal.org/node/3188960
This is ready for review.
Comment #7
daffie commentedI 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
RequestExceptiontoTransferExceptionare 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.
Comment #8
alexpottI 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.
Comment #9
alexpottCommitted ee0a221 and pushed to 9.2.x. Thanks!
Comment #11
alexpottI created the follow-up - #3189301: Use \Psr\Http\Client\ClientExceptionInterface instead of \GuzzleHttp\Exception\TransferException
Comment #13
xjm