Contrib.social

Provide an interface for Drupal HTTP ClientFactory service

Open on Drupal.org →
Created on 23 July 2024, about 2 months ago
Updated 6 August 2024, about 1 month ago

Problem/Motivation

Sometimes we need a custom HTTP client for some custom services, usually for testing purposes. For example, to provide mocked responses for outgoing HTTP requests in functional tests.

For those cases, Drupal provides http_client_factory service, which can provide http_client with custom options.

But in tests, we usually need to provide a custom HTTP Client, not the original Guzzle class. And we can't do dependency injection here and replace the default class Drupal\Core\Http\ClientFactory with a custom one because it doesn't implement any interface.

Steps to reproduce

1. Create a service that calls some external HTTP API, using an HTTP client from the factory, with custom options.

2. Try to create a test module for functional tests, which overrides the client factory and provides a custom HTTP Client with mocked responses.

You will get an error like this: 

TypeError: Argument #1 ($httpClientFactory) must be of type Drupal\Core\Http\ClientFactory, Drupal\my_module_test\TestClientFactory given.

Proposed resolution

The optimal way to resolve this issue is to make Drupal\Core\Http\ClientFactoryInterface and use it as the param type in the service constructors, instead of the Drupal Core class.

With this, we can easily create a custom factory, that implements this interface, and replace the core factory with a custom one.

Also, there is a workaround possible like this:

  /**
   * MyModule constructor.
   *
   * @param \Drupal\Core\Http\ClientFactory|\Drupal\my_module_test\TestClientFactory $httpClientFactory
   *   The HTTP client factory service.
   */
  public function __construct(
    protected \Drupal\Core\Http\ClientFactory|\Drupal\my_module_test\TestClientFactory $httpClientFactory,
  ) {
  }

But this looks pretty ugly because we should not mention test assets in the main code.

So, with the suggested change we can type just an interface like this:

  /**
   * MyModule constructor.
   *
   * @param \Drupal\Core\Http\ClientFactoryInterface $httpClientFactory
   *   The HTTP client factory service.
   */
  public function __construct(
    protected \Drupal\Core\Http\ClientFactoryInterface $httpClientFactory,
  ) {
  }

What do you think about this idea?

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

✨ Feature request
Status

Needs work

Version

11.0 🔥

Component
Base  →

Last updated less than a minute ago

Created by

🇦🇲Armenia Murz Yerevan, Armenia

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

Merge Requests

Comments & Activities

  • Issue created by @Murz
  • Merge request !8883Issue #3463251: Provide an interface for Drupal HTTP ClientFactory service → (Open) created by Murz
  • Pipeline finished with Failed
    about 2 months ago
    Total: 237s
    #231772
  • Status changed to Needs review about 2 months ago
  • Comment about 2 months ago →
    🇦🇲Armenia Murz Yerevan, Armenia

    I made an MR https://git.drupalcode.org/project/drupal/-/merge_requests/8883 with implementing the Drupal\Core\Http\ClientFactoryInterface - please review.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 265s
    #231776
  • Comment about 2 months ago →
    🇳🇿New Zealand quietone New Zealand

    Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

    It is my understanding the the 'Needs Review Queue Initiative' tag is added after the issue has been reviewed by that initiative, so they can monitor the activity on reviewed issue. The definition → of this special tag supports my thinking. Am I wrong?

  • Status changed to Needs work about 2 months ago
  • Comment about 2 months ago →
    🇺🇸United States smustgrave 
            Ignored error pattern #^Call to deprecated method getConfig\(\) of                    
         class GuzzleHttp\\Client\:                                                            
         Client\:\:getConfig will be removed in guzzlehttp/guzzle\:8\.0\.$# in                 
         path                                                                                  
         /builds/issue/drupal-3463251/core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php  
         was not matched in reported errors.                                                   
  58     Call to deprecated method getConfig() of interface                                    
         GuzzleHttp\ClientInterface:                                                           
         ClientInterface::getConfig will be removed in guzzlehttp/guzzle:8.0.                  
         🪪  method.deprecated                                                                 
 ------ -----------------------------

    Appears to have phpstan issues

    @quietone that's a good question not sure we ever decided on that.

  • Status changed to Needs review about 2 months ago
  • Comment about 2 months ago →
    🇦🇲Armenia Murz Yerevan, Armenia

    > Appears to have phpstan issues

    Yes, but they are not related to the changes at all. And I have no idea about how to fix this error, only remove the test. Here is an issue about this: https://github.com/guzzle/guzzle/issues/3114

  • Status changed to Needs work about 1 month ago
  • Comment about 1 month ago →
    🇺🇸United States smustgrave

    Would first try rebasing as current MR is 67 commits behind.

    Then can see if it passes but can't mark it with failing checks.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024