[FrameworkBundle] revert implementing LoggerAwareInterface in HTTP client decorators

[xabbuh]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	no
Deprecations?	no
Issues
License	MIT

[xabbuh]

Implementing the LoggerAwareInterface when the constructor already receives a logger doesn't look reasonable to me.

[xabbuh]

I also wonder if we shouldn't change the other decorators to let them optionally receive a logger in their constructor too instead of implementing the LoggerAwareInterface

[xabbuh]

doing so would also allow us to revert the composer.json changes

[xabbuh]

I am not convinced that a decorator should actually modify the decorated HTTP client this way. IMO setting the logger is the user's responsibility when constructing the client.

[nicolas-grekas]

Looking at your comments here, I wonder if we should keep #54668 or not?
Maybe @yann-eugone can tell use how he wires the client so that we understant why adding LoggerAwareInterface helps?
I'm not keen on the bump in composer.json, it'd prefer not doing it.

[yann-eugone]

Sure,

On that project, constructing an HttpClientInterface is a thing that is done at runtime, because it all depend on configuration (what service you are trying to call)

final class HttpClientFactory
{
    public function __construct(
        private readonly HttpClientInterface $http,
    ) {
    }

    public function create(Config $config, LoggerInterface $logger): HttpClientInterface
    {
        $client = match (...) {
            Config::A => $this->clientA(),
            Config::B => $this->clientB(),
        };
        if ($client instanceof LoggerAwareInterface) {
            $client->setLogger($logger);
        }

        return $client;
    }

    private function clientA(): HttpClientInterface
    {
        return $this->http->withOptions([
            ...
        ]);
    }

    private function clientB(): HttpClientInterface
    {
        return $this->http->withOptions([
            ...
        ]);
    }
}

I'm using Symfony\Contracts\HttpClient\HttpClientInterface::withOptions because it's way safer for unit testing to clone a client provided from a constructor than having to build from the ground.

As you can see, the $logger is provided in the factory method, because each feature has it's own logger instance, but they all share dependency on HttpClientFactory

#[WithMonologChannel('feature1')]
final class Feature1
{
    public function __construct(
        private readonly HttpClientFactory $httpClientFactory,
        private readonly LoggerInterface $logger,
    ) {
    }

    public function method(Config $config): void
    {
        $http = $this->httpClientFactory->create($config, $this->logger);
        ...
    }
}

#[WithMonologChannel('feature2')]
final class Feature2
{
    public function __construct(
        private readonly HttpClientFactory $httpClientFactory,
        private readonly LoggerInterface $logger,
    ) {
    }

    public function method(Config $config): void
    {
        $http = $this->httpClientFactory->create($config, $this->logger);
        ...
    }
}

I've only proposed adding LoggerAwareInterface because I saw that actual clients already implemented it, so I believed it was just about someone forgetting to add it everywhere.

[xabbuh]

What about supporting passing the logger to withOptions() instead? This could look something like this in your factory then:

private function clientA(LoggerInterface $logger): HttpClientInterface
{
    return $this->http->withOptions([
        'extra' => [
            'logger' => $logger,
        ],
        // ...
    ]);
}

[nicolas-grekas]

I wonder if we need this PR at all: to me, the actual http client that does log should already have the logger wired. Did you check this aspect?

[xabbuh]

to me, the actual http client that does log should already have the logger wired

I agree with this reasoning. That's why I suggest to revert implementing the LoggerAwareInterface in HTTP client decorators.

[yann-eugone]

I don't have any opinion on this honestly
But why removing setLogger on decorated clients, but not on actual ones ?

[xabbuh]

The actual clients do use the logger themselves. They do not use this property to configure another client. We can argue if using the interface was the right decision back when we introduced it, but that’s a different topic.

[nicolas-grekas]

🚀

We could even deprecate existing setLogger proxies.

[fabpot]

Thank you @xabbuh.

[xabbuh]

We could even deprecate existing setLogger proxies.

see #54806