Contrib.social

Catch ClientException

Open on Drupal.org →
Created on 10 March 2024, 9 months ago
Updated 1 August 2024, 4 months ago

Problem/Motivation

I discovered, that on our site there are links where, "Last check" is always "Never". When I checked in details, I discovered, that all of them return exception with message: Could not resolve host. This exceptions should be catched and reported.

Steps to reproduce

  1. Add link for domain that doesn't exist.
  2. Check it's status.

Proposed resolution

The problem is with LinkCheckerService::check(), which catches only RequestException errors. But httpClient can throw ClientException as well. This should be extended like this:

function (RequestException|ClientException $e) use ($link) {
  $this->exceptionHandling($e, $link);
}

or use TransferException (both above extend it):

function (TransferException $e) use ($link) {
  $this->exceptionHandling($e, $link);
}

Also LinkCheckerService:exceptionHandling() should be modified:

protected function exceptionHandling(TransferException $e, LinkCheckerLinkInterface $link) {
  if ($e instanceof ConnectException) {
    $link->setStatusCode('404');
  }
  else {
    $link->setStatusCode('502');
  }
  ...

The only thing is, what status code should be assigned:

  • 404: because gives site can't be found (from user perspective). From user perspective it's error, so it should be something bigger than 399.
  • 0: because gives host can't be found at all, so no response is possible. But this is developer perspective. And e.g. filtering by errors, etc will be more complex, because to get all errors you have to use condition "where status > 399 or status = 0).

I decided for 404, but discussion is open here.

🐛 Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

🇵🇱Poland gugalamaciek

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @gugalamaciek
  • Comment 9 months ago →
    🇵🇱Poland gugalamaciek

    This patch catches TransferException and assigns 404 status code to errors "could not resolve host".

  • Comment 9 months ago →
    🇵🇱Poland gugalamaciek

    This patch catches TransferException and assigns 404 status code to errors "could not resolve host".

  • Comment 9 months ago →
    🇵🇱Poland gugalamaciek

    Ups... wrong patch. Don't use #2 and #3.
    This patch catches TransferException and assigns 404 status code to errors "could not resolve host".

  • Status changed to Needs review 9 months ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 9 months ago
    86 pass
  • Comment 9 months ago →
    🇵🇱Poland gugalamaciek
  • Comment 8 months ago →
    🇳🇴Norway eiriksm Norway

    Very good suggestion. Thanks! ♥️

    Regarding the 404 problem, I'm not sure... From a developer perspective it makes no sense, but I totally see the value for the non technical editor. Would love to hear some more perspective on it from other people.

    Ideally we should have a couple Tests for this. Are you able to write up something like that too?

  • Comment 8 months ago →
    🇩🇪Germany kle

    Even if I can live well with a 404 - the status is called "failed" and unfortunately this does not exist in the status codes.
    Wouldn't something like 999 be a good idea? Or the user can enter the desired code himself in a setting?

  • Comment 4 months ago →
    🇺🇸United States simplyshipley

    Patch #4 was failing to install with composer-patches so I applied the changes to the file and created this patch. I attempted to create an interdiff between the files, but interdiff created a blank file and gave this message "interdiff: Whitespace damage detected in input".

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024