[HttpClient] Configure `MockClient` if mock_response_factory has been set on a scoped client #52265

tarjei · 2023-10-24T10:44:50Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #52257
License	MIT

This makes it possible to use different mock response factories on different services and also to easily test services that uses http client endpoints.

It also generates an extra test service http_client.mock_client.$name that can be used to configure the MockHttpClient instance if you want to inject a different responsefactory.

Example usage:

#config/packages/test/http_client.yaml
framework:
  http_client:
    scoped_clients:
      apiServiceClient:
        base_uri: ...
        mock_response_factory: "acme_mock_factory"
     otherClient:
        base_uri: ...
        mock_response_factory: true
     notMockedClient:
        base_uri: ...

The first client will use the acme_mock_factory for mock_factory while otherClient will be a MockHttpClient but without a set responsefactory (it can be configured in the test).

tarjei · 2023-10-24T10:53:38Z

@nicolas-grekas I'll fix the last tests if you like the direction of the PR :)

nicolas-grekas · 2023-10-25T16:23:25Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+
+
+                $container->register('http_client.mock_client.' . $name, MockHttpClient::class)
+                    ->setDecoratedService($httpClientTransport, null, -10)  // lower priority than TraceableHttpClient (5)
+                    ->setArguments(
+                        $responseFactoryId === true ? [] : [new Reference($responseFactoryId)]);


this looks good to me but I don't get the true case, what is it for? we don't have it for the general mock factory

It is if I want to have a mock client here but I do not want to configure the responsefactory as a service but just do it in the test. I considered having a separate attribute for it, i.e. mock: true and then having the mock_response_factory be an optional attribute if needed.

Maybe being able to just say "mock this" and then add responses in the client is the main use case - i.e. that mock should be a separate parameter to make it more clear.

Got it. Let's do the same for the global factory?

Good idea. So add a mock: bool to both the global factory and the specific factory?

@nicolas-grekas : ping :) I just wanted to get a reaction to the last question before working on the PR. Also: Is it possible to get this into the 6.x series?

…d client. This makes it possible to use different mock response factories on different services and also to easily test services that uses http client endpoints. It also generates an extra test service http_client.mock_client.$name that can be used to configure the MockHttpClient instance if you want to inject a different responsefactory. We also change the framework.http_client.mock_client_factory attribute to allow for a boolean so you can just mock without injecting a responsefactory in the service setup.

Neirda24

Not core team so those remarks are my own. Only here to help ;)

Neirda24 · 2024-04-12T13:19:07Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+
+            if ($responseFactoryId = $scopeConfig['mock_response_factory'] ?? null) {
+                $container->register('http_client.mock_client.'.$name, MockHttpClient::class)
+                    ->setDecoratedService($httpClientTransport, null, -10)  // lower priority than TraceableHttpClient (5)


it is also decorated with priority 10 for the global mock_response_factory. Maybe worth having a lower priority here so it superseeds the global one ?

Neirda24 · 2024-04-12T13:20:22Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            if (null === $scope) {
                $baseUri = $scopeConfig['base_uri'];
                unset($scopeConfig['base_uri']);

                $container->register($name, ScopingHttpClient::class)
                    ->setFactory([ScopingHttpClient::class, 'forBaseUri'])
-                    ->setArguments([new Reference('http_client.transport'), $baseUri, $scopeConfig])
+                    ->setArguments([$httpClientTransport, $baseUri, $scopeConfig])


not sure this change is needed. Sure you save a few objects and memory but only during build time and this is like 3 objects.

Neirda24 · 2024-04-12T13:22:42Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -2521,7 +2530,7 @@ private function registerHttpClientConfiguration(array $config, ContainerBuilder
        if ($responseFactoryId = $config['mock_response_factory'] ?? null) {
            $container->register('http_client.mock_client', MockHttpClient::class)
                ->setDecoratedService('http_client.transport', null, -10)  // lower priority than TraceableHttpClient (5)
-                ->setArguments([new Reference($responseFactoryId)]);
+                ->setArguments(true === $responseFactoryId ? [] : [new Reference($responseFactoryId)]);


What is the goal here ? Tell me if I'm wrong but it looks like you are trying to set it to [] only if at least one dedicated mock was set. But then what if you have a global one and one specific for only one of the clients ?

Because that variable is set only during foreach.

"Because that variable " <-- do you mean $responseFactoryId which is set in the if ?

yup It is created and overidden in foreach at each iteration. So the only value it could have would be the last scoped client says its true

I'll look into this :)

.../Tests/DependencyInjection/Fixtures/php/http_client_scoped_mock_response_factory_service.php

Neirda24 · 2024-04-12T13:25:03Z

...FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/http_client_mock_response_factory.yml

@@ -6,4 +6,4 @@ framework:
        log: true
    http_client:
        default_options: ~
-        mock_response_factory: my_response_factory
+        mock_response_factory: true


I don't understand properly the true. Do you have a pending documentation PR that could help me understand ?

From a comment above:

It is if I want to have a mock client here but I do not want to configure the responsefactory as a service but just do it in the test. I considered having a separate attribute for it, i.e. mock: true and then having the mock_response_factory be an optional attribute if needed.

Maybe being able to just say "mock this" and then add responses in the client is the main use case - i.e. that mock should be a separate parameter to make it more clear.

Yes I think a dedicated parameter is better because I do not find intuitive to set this to true to delegate at runtime during test to be able to provide the factory in question. Maybe some other approach would be a better fit ? Don't have an idea at the moment though.

@Neirda24 maybe the problem is the parameter name in general?

If we renamed the parameter name to mock_responses then it could work the same way going forward. The problem then is backward compatibility which is harder to solve.

IMHO The right way would be be to change the parameter name, BUT due to backwards compability (as I understand Symfony's policy) this would require some extra gluecode to keep both options. One of the reasons I'm inclined to keep this current PRs solution is that it requires less code and tests than two options.

I'm more than willing to do a documentation PR for it though :)

Also: If you mock the default client then this choice becomes the default for scoped clients

tarjei · 2024-05-12T18:24:12Z

Ok, i've now updated this PR to separate out mock_client from mock_response_factory and also add tests and handling of overriding the options in scoped clients.

Example config:

    http_client:
        default_options: ~
        mock_client: true
        scoped_clients:
            notMocked:
                base_uri : https://symfony.com
                mock_client : false
            mocked:
                base_uri: https://symfony.com
            mocked_with_factory:
                base_uri: https://symfony.com
                mock_response_factory: 'my_response_factory'

Note: Today mock_response_factory is directly under the http_client key. Maybe it both should be moved under the default_options key to correspond better with the other options of the http client?

tarjei · 2024-05-14T11:16:52Z

@Neirda24 do the changes satisfy your concerns? :)

carsonbot added Status: Needs Review Bug Feature labels Oct 24, 2023

carsonbot added this to the 6.4 milestone Oct 24, 2023

tarjei force-pushed the 52257_http_client_mocks branch from 09ce17e to 5091cc6 Compare October 24, 2023 10:46

OskarStark added HttpClient and removed Bug labels Oct 25, 2023

carsonbot changed the title ~~Configure mockclient if mock_response_factory has been set on a scoped client~~ [HttpClient] Configure mockclient if mock_response_factory has been set on a scoped client Oct 25, 2023

symfony deleted a comment from carsonbot Oct 25, 2023

OskarStark changed the title ~~[HttpClient] Configure mockclient if mock_response_factory has been set on a scoped client~~ [HttpClient] Configure MockClient if mock_response_factory has been set on a scoped client Oct 25, 2023

nicolas-grekas reviewed Oct 25, 2023

View reviewed changes

nicolas-grekas modified the milestones: 6.4, 7.1 Oct 26, 2023

OskarStark changed the title ~~[HttpClient] Configure MockClient if mock_response_factory has been set on a scoped client~~ [HttpClient] Configure MockClient if mock_response_factory has been set on a scoped client Oct 30, 2023

tarjei force-pushed the 52257_http_client_mocks branch 2 times, most recently from a045ef5 to 182182c Compare November 6, 2023 08:32

tarjei requested a review from nicolas-grekas November 6, 2023 08:48

tarjei force-pushed the 52257_http_client_mocks branch from 182182c to 0e53491 Compare April 12, 2024 12:40

tarjei force-pushed the 52257_http_client_mocks branch from 0e53491 to 5fd1b32 Compare April 12, 2024 12:54

Neirda24 reviewed Apr 12, 2024

View reviewed changes

tarjei added 3 commits May 10, 2024 12:10

Improve testcoverage and add mock_client boolean parameter.

8b43395

Also: If you mock the default client then this choice becomes the default for scoped clients

Fix test

12fab44

Move mock_ options out of default_options and update tests accordingly

776e650

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

Jean85 mentioned this pull request Sep 2, 2024

[AssetMapper] Asset install fails due to mocked HTTP client #58149

Open

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

Uh oh!

[HttpClient] Configure MockClient if mock_response_factory has been set on a scoped client #52265

Are you sure you want to change the base?

[HttpClient] Configure MockClient if mock_response_factory has been set on a scoped client #52265

Uh oh!

Conversation

tarjei commented Oct 24, 2023 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarjei commented Oct 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neirda24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarjei commented May 12, 2024

Uh oh!

tarjei commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

[HttpClient] Configure `MockClient` if mock_response_factory has been set on a scoped client #52265

[HttpClient] Configure `MockClient` if mock_response_factory has been set on a scoped client #52265

tarjei commented Oct 24, 2023 •

edited by nicolas-grekas

Loading

tarjei commented May 14, 2024 •

edited

Loading