-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[HttpClient] Make CachingHttpClient
compatible with RFC 9111
#59576
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
base: 7.4
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Can we bring this to CachingHttpClient (with a BC layer) instead of introducing a new class that is the good implementation of caching but with a name that is much less clear ? |
src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpClient/Tests/LegacyCachingHttpClientTest.php
Outdated
Show resolved
Hide resolved
62db76a
to
5b52dd2
Compare
UPDATE:
Otherwise, PR is ready on my side. Ready for another reviews. |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
b7a0bb4
to
12bf66f
Compare
e93f3b2
to
d0be468
Compare
CachingHttpClient
compatible with RFC 9111
b9df14d
to
0405dc5
Compare
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Show resolved
Hide resolved
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
7c66ffd
to
17d0fdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'd like a review from @nicolas-grekas as he is the one knowing symfony/http-client best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get into all details but still LGTM thanks!
throw new \LogicException(\sprintf('Using "%s" requires the HttpKernel component, try running "composer require symfony/http-kernel".', __CLASS__)); | ||
} | ||
if ($cache instanceof StoreInterface) { | ||
trigger_deprecation('symfony/http-client', '7.3', 'Passing a "%s" as constructor\'s 2nd argument of "%s" is deprecated, "%s" expected.', StoreInterface::class, __CLASS__, TagAwareCacheInterface::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trigger_deprecation('symfony/http-client', '7.3', 'Passing a "%s" as constructor\'s 2nd argument of "%s" is deprecated, "%s" expected.', StoreInterface::class, __CLASS__, TagAwareCacheInterface::class); | |
trigger_deprecation('symfony/http-client', '7.4', 'Passing a "%s" as constructor\'s 2nd argument of "%s" is deprecated, "%s" expected.', StoreInterface::class, __CLASS__, TagAwareCacheInterface::class); |
@@ -5,6 +5,7 @@ CHANGELOG | |||
--- | |||
|
|||
* Deprecate using amphp/http-client < 5 | |||
* Add RFC 9111–based caching support to `CachingHttpClient` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's mention the deprecation also
* Deprecate passing an instance of `StoreInterface` as `$cache` argument to `CachingHttpClient` constructor
return new AsyncResponse($this->client, $method, $url, $options); | ||
} | ||
|
||
$requestHash = self::hash($method.$fullUrl.json_encode(array_intersect_key($options['normalized_headers'], self::RESPONSE_INFLUENCING_HEADERS), \JSON_THROW_ON_ERROR)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serialize instead of json_encode to work with non-utf8 chars?
Can you please maybe expand in the PR description? Think about a blog post we could derive from it :) |
I'm currently on holiday. I'll get back to it next week. Including my thoughts on possible race conditions discussed earlier.
May you be more specific ? You want me to explain the code ? |
What we could put in the doc about it yes (thus not internal details of course) |
https://www.rfc-editor.org/rfc/rfc9111.html