Skip to content

[HttpClient] Add LoggerAwareInterface to ScopingHttpClient and TraceableHttpClient #35273

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

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

pierredup
Copy link
Contributor

@pierredup pierredup commented Jan 9, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

This allows changing the logger when using ScopingHttpClient (and TraceableHttpClient during dev)

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 9, 2020
@nicolas-grekas
Copy link
Member

But one should be able to configure the decorated client before decorating it. I think not having this method encourages better instantiation steps. I get this can be helpful, but maybe here we'd prefer favoring better design?

@pierredup
Copy link
Contributor Author

pierredup commented Jan 9, 2020

But one should be able to configure the decorated client before decorating it

This doesn't work when injecting a scoped client from the framework configuration, as you are then stuck with the default logger and can't change it at runtime (E.G injecting a ConsoleLogger when running in CLI)

@fabpot
Copy link
Member

fabpot commented Jan 10, 2020

Thank you @pierredup.

fabpot added a commit that referenced this pull request Jan 10, 2020
…ient and TraceableHttpClient (pierredup)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[HttpClient] Add LoggerAwareInterface to ScopingHttpClient and TraceableHttpClient

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | N/A

This allows changing the logger when using `ScopingHttpClient` (and `TraceableHttpClient` during dev)

Commits
-------

1137bdc Add LoggerAwareInterface to ScopingHttpClient and TraceableHttpClient
@fabpot fabpot merged commit 1137bdc into symfony:master Jan 10, 2020
@pierredup pierredup deleted the http-client-logger branch January 15, 2020 07:13
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
@vudaltsov
Copy link
Contributor

I think that if you need an HttpClient with a different logger in some place, you should then configure a separate HttpClient exactly for this use case. Services are lazy-loaded, so it does not matter how many clients you have anyway.

Mutating services at Runtime is a bad practice — it leads to unpredictable behavior in services that reuse them. It's really sad that LoggerAwareInterface allows and encourages that. The best practice is to inject services via constructor, so that they cannot be replaced later.

Also decorators should not imply that inner services have some other interfaces and downcast them to figure that out. What if -1 inner client is not LoggerAware and -2 inner client is? This decorator will then fail to replace the logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants