Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

rmikalkenas
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Do not transfer kernel.reset tag to decorators and added some tests to make sure that ServicesResetter has a decorated service

@rmikalkenas rmikalkenas force-pushed the decorated-service-reset branch from 910f544 to fbdade2 Compare November 9, 2021 13:06
@nicolas-grekas
Copy link
Member

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.

@rmikalkenas
Copy link
Contributor Author

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)

@stof
Copy link
Member

stof commented Nov 9, 2021

@nicolas-grekas if the decorator needs to have its own resetting logic, the decorator itself should implement ResetInterface and so be tagged as kernel.reset

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 9, 2021

client's interface should extend ResetInterface

That's correct (and that's also correct for any decorator, as @stof just wrote).

TraceableHttpClient already does so. Is this missing somewhere else where service decoration is used? A quick look makes me think we're OK.

@rmikalkenas
Copy link
Contributor Author

rmikalkenas commented Nov 9, 2021

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

@nicolas-grekas
Copy link
Member

These decorators don't need to implement reset interface, unless they're used as service decorators (they're not to my knownledge.)
To me there is nothing to do, unless provided otherwise.

@rmikalkenas
Copy link
Contributor Author

These decorators don't need to implement reset interface, unless they're used as service decorators (they're not to my knownledge.) To me there is nothing to do, unless provided otherwise.

Allright, closing this PR and preparing another with implemented ResetInterface for http clients

@rmikalkenas rmikalkenas closed this Nov 9, 2021
@nicolas-grekas
Copy link
Member

Thanks. I submitted #43983 and #43981. They should help on the topic also.

fabpot added a commit that referenced this pull request Nov 10, 2021
…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
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.

4 participants