-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Make sure that decorated service works with kernel.reset tag #43972
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
Conversation
910f544
to
fbdade2
Compare
Thanks for the PR. I did think about that in the last days, and I'm not sure we should do it. Decorators should be able to decorate the reset method as any other method. Forcing a direct call to the decorated service would be unexpected to me. The issue that lead to this proposal has been closed and doesn't require this change, isn't it? It means there is already a working solution. Unless we have a real world use case that would allow us to reason about this topic, I think we should not do this. |
I see your point, but that would mean that every decorated service should also implement ResetInterface. Let's pick an example of HttpClientInterface: based on your suggestion, then client's interface should extend ResetInterface, because otherwise some other client's implementation (which is used as a decorator) might miss to implement ResetInterface leading to not resetting underlying decorated services (hopefully you get the idea I'm trying to explain) |
@nicolas-grekas if the decorator needs to have its own resetting logic, the decorator itself should implement ResetInterface and so be tagged as |
That's correct (and that's also correct for any decorator, as @stof just wrote).
|
Agree with both of you, but would also like to somehow enforce the developer who is implementing a custom HttpClientInterface to dont forget to add ResetInterface, because otherwise we will loose resetting functionality if that custom implementation is used as a decorator. WDYT @nicolas-grekas @stof Meanwhile I see that quite some http clients are missing ResetInterface (exp.: RetryableHttpClient, CachingHttpClient and others), so probably will prepare a PR to add it |
These decorators don't need to implement reset interface, unless they're used as service decorators (they're not to my knownledge.) |
Allright, closing this PR and preparing another with implemented ResetInterface for http clients |
…ents (rmikalkenas) This PR was merged into the 5.4 branch. Discussion ---------- [HttpClient] Implement ResetInterface for all http clients | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | License | MIT Result of discussion at #43972 Targeting 5.4 branch as I think its kind of a new feature without BC break Commits ------- ab53683 [HttpClient] Implement ResetInterface for all http clients
Do not transfer
kernel.reset
tag to decorators and added some tests to make sure that ServicesResetter has a decorated service