Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpClient] logger integration #30537

Open
wants to merge 10 commits into
base: master
from

Conversation

@antonch1989
Copy link
Contributor

antonch1989 commented Mar 12, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #30502
License MIT
Doc PR

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 12, 2019

@nicolas-grekas nicolas-grekas referenced this pull request Mar 12, 2019

Open

[HttpClient] Next steps #30502

8 of 17 tasks complete
@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

thanks for starting this, let's do something useful with the logger now :)

Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClient.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClient.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/HttpClient.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/Tests/CurlHttpClientTest.php Outdated
@HeahDude
Copy link
Member

HeahDude left a comment

Thanks for this PR!

/**
* @param array $defaultOptions Default requests' options
* @param int $maxHostConnections The maximum number of connections to a single host
*
* @see HttpClientInterface::OPTIONS_DEFAULTS for available options
*/
public function __construct(array $defaultOptions = [], int $maxHostConnections = 6)
public function __construct(LoggerInterface $logger = null, array $defaultOptions = [], int $maxHostConnections = 6)

This comment has been minimized.

@HeahDude

HeahDude Mar 13, 2019

Member

This should be the last argument IMHO.

This comment has been minimized.

@linaori

linaori Mar 13, 2019

Contributor

That does make it harder to pass the default values properly (in case they change) on line 165: $this->multi->handle = (new self($this->logger))->multi->handle;

This comment has been minimized.

@HeahDude

HeahDude Mar 13, 2019

Member

True, what about something like $this->multi->handle = (clone $this)->multi->handle;, with any proper cloning handling if needed?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 13, 2019

Member

We would then have to empty the clone

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 13, 2019

Member

What about putting the argument 2nd?
Looks best ordering to me, from most often passed to most often left to defaults.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 13, 2019

Member

Alternatively: withLogger()

This comment has been minimized.

@HeahDude

HeahDude Mar 13, 2019

Member

Having it last in the constructor and/or using a mutator looks more legit, since from a strict object point of view, if we create it the most needed is to configure it. The logger is not a requirement for this client, this is obviously why we add it in this PR. I don't think we should break the previous implementation. Plus using a logger also surely means in most cases that DI is used and we don't care about having it third.

This comment has been minimized.

@dunglas

dunglas Mar 13, 2019

Member

Or it could implement Psr\Log\LoggerAwareInterface (adds a setLogger() method). This interface triggers the autoconfiguration in Symfony IIRC.

This comment has been minimized.

@fabpot

fabpot Mar 14, 2019

Member

I like @dunglas suggestion.

@joelwurtz

This comment has been minimized.

Copy link
Contributor

joelwurtz commented Mar 14, 2019

Not really a fan of having a logger inside the client implementation when it could use decoration, but really depends if you want to log what the implementation does.

If it's only logging request / response transmission it should use a decoration system (or at least a trait but not a fan of this either). Using decorations would allow:

  • other implementation to not care about it (less code = happier maintainers)
  • also this allow to log at a specific time if you have other decorators (you can log before and after something has change the request to debug what's changed between the original request and the one received by the client)
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 14, 2019

@joelwurtz same as #30539: logging in a cross-cutting concern, it's a bad fit for a middleware. I'd like implementation to log what they do internally also, e.g. dealing with redirections, or with server pushes for the CurlHttpClient. Exposing these steps for the purpose of logging means adding a lot of complexity, not worth it.

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

I'm fine with the adding as 2nd argument personally. We should now log some things :)

Show resolved Hide resolved src/Symfony/Component/HttpClient/NativeHttpClient.php Outdated

antonch1989 added some commits Mar 14, 2019

if (\extension_loaded('curl')) {
return new CurlHttpClient($defaultOptions, $maxHostConnections);
$logger->info('Curl extension is enabled. Creating client.', ['client' => CurlHttpClient::class]);

This comment has been minimized.

@stof

stof Mar 18, 2019

Member

this should be at the debug level IMO rather than at info, to be able to exclude it more easily (that's not a very useful log message most of the time)

@@ -31,6 +32,10 @@ final class HttpClient
*/
public static function create(array $defaultOptions = [], LoggerInterface $logger = null, int $maxHostConnections = 6): HttpClientInterface
{
if ($logger === null) {
$logger = new NullLogger();
}

This comment has been minimized.

@yceruto

yceruto Mar 18, 2019

Contributor
$this->logger = $logger ?? new NullLogger();

? Consequently with what you did for other clients.

This comment has been minimized.

@antonch1989

antonch1989 Mar 18, 2019

Author Contributor

logger in not needed as a class property there, it is only in the scope of create method

This comment has been minimized.

@smoench

smoench Mar 18, 2019

Contributor

Do we really need this? Both constructors of CurlHttpClient and NativeHttpClient are already handling this case.

This comment has been minimized.

@antonch1989

antonch1989 Mar 18, 2019

Author Contributor

yes we need, to test classes separately

antonch1989 added some commits Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.