Contrib.social

Allow logging other response types

Open on Drupal.org →
Created on 4 November 2024, 11 days ago

Problem/Motivation

When there is no response (e.g. in the case of a ConnectException), or a response content type other than HTML, JSON or XML, no log entry is recorded. I suspect that this means that even the example on the project homepage of recording a 500 error, and when the Accept header is set to text/plain, is no longer even possible to reproduce currently. (Which is why I'm reporting this as a bug.)

As I've looked through the relevant code in \Drupal\http_client_log\Logger\Logger::log(), I also see the catch blocks which look to me like they could be impossible to reach. Where within the try block there would those exceptions be thrown? Request exceptions would normally be thrown by \GuzzleHttp\Middleware::httpErrors() at the end of processing the middleware stack, after this module's logging middleware is invoked.

I can see that the current behaviour may be desirable on some sites, so I suggest making it configurable (even via \Drupal\Core\Site\Settings if using ordinary config would introduce unwanted complexity / circular dependencies?). So the current behaviour could be the default, but sites could opt into logging all types of response, including when there is no response to a request (e.g. in the case of a ConnectException).

Steps to reproduce

Each of these situations can reproduce the issue:
* Make a request through Guzzle to a URL that returns a 'text/plain' Content-Type header in its response, such as the example on this project's homepage.
* Disconnect from the internet and make a request through Guzzle; nothing is added to the log

Proposed resolution

1. Remove current exception handling in \Drupal\http_client_log\Logger\Logger::log()
2. Replace current hardcoded filter for the response content type with something more dynamic, ideally using Drupal config so that sites can choose which response types to log. Ensure a default is set (if using config, that probably includes adding an update hook to set the default value(s)).

Remaining tasks

Implement solution, test & review.

User interface changes

Potentially adding a new setting to the module's config form.

API changes

None (by default).

Data model changes

Potentially an additional property in the module's config.

🐛 Bug report
Status

Active

Version

1.1

Component

Code

Created by

🇬🇧United Kingdom james.williams

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @james.williams
  • Comment 11 days ago →
    🇬🇧United Kingdom james.williams

    P.S. As I'm a keen user of your module, and have given various contributions in the past already, I'd be happy to help co-maintain this module if you would like that?

  • Comment 10 days ago →
    🇨🇳China zterry95

    Hi James,
    Thanks for your contribution, you have already became the maintainer of http_client_logs.
    So I think you can directly fixed the problem:)

    Cheers!!

  • Comment 10 days ago →
    🇬🇧United Kingdom james.williams

    Oh, thank you! Do you have any particular plans or ways of working on this module that would be worth discussing so that we can align well?

  • Comment 9 days ago →
    🇨🇳China zterry95

    Currently I have no further plan on this module.
    This module initially design to log the external rest API request make by Drupal::httpClient() and till now it works well.

    Why it logs only HTML, JSON or XML?
    Because if there is no such limit, the request to external image or pdf may cause other problem.

    If you had time to complete the more exception handling in \Drupal\http_client_log\Logger\Logger::log(), please go ahead,that's will be great.

  • Merge request !8Issue #3485384: Allow logging other response types & without responses. → (Open) created by james.williams
  • Comment 3 days ago →
    🇬🇧United Kingdom james.williams

    Thanks very much! I've opened MR !8 to introduce configuration for allowing logging when there is no response (defaulting to not), and filtering by response content type (defaulting to XML, JSON and HTML as currently). No doubt this could do with some more testing and a second pair of eyes.

    By the way, would you be up for switching the default branch of the repo to 1.1.x instead of 1.0.x?

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024