Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpKernel] PSR-6 HttpCache store#42295
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 ourterms of service andprivacy 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
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 left some comments, great work, thank you!
| private $locks = []; | ||
| /** | ||
| * When creating a Psr6Store you can configure a number options. |
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.
| * When creating a Psr6Store you can configure a number options. | |
| * When creating a Psr6Store you can configure a numberofoptions. |
| /** | ||
| * When creating a Psr6Store you can configure a number options. | ||
| * See the README for a list of all available options and their description. |
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 would rather remove this sentence and add the options to the Symfony docs
| } | ||
| /** | ||
| * Invalidates all cache entries that match the request. |
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.
Sometimes you writeRequest, sometimesrequest, only one should be used
| /** | ||
| * Releases the lock for the given Request. | ||
| * | ||
| * @param Request $request A Request instance |
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.
Not sure we need this@param annotation 🤔
| $response->headers->set('X-Content-Digest', $contentDigest); | ||
| // Make sure the content-length header is present | ||
| if (!$response->headers->has('Transfer-Encoding')) { |
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.
What if it already has aContent-Length header? Do we always override it?
| * @param int $expiresAfter | ||
| * @param array $tags | ||
| */ | ||
| private function saveDeferred(CacheItemInterface $item, $data, $expiresAfter = null, $tags = []): bool |
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.
| privatefunction saveDeferred(CacheItemInterface$item,$data,$expiresAfter =null,$tags = []):bool | |
| privatefunction saveDeferred(CacheItemInterface$item,$data,int$expiresAfter =null,array$tags = []):bool |
and remove the doc block?
| use Symfony\Component\Lock\LockInterface; | ||
| use Symfony\Component\OptionsResolver\Exception\MissingOptionsException; | ||
| class Psr6StoreTest extends TestCase |
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.
| class Psr6StoreTestextends TestCase | |
| finalclass Psr6StoreTestextends TestCase |
| $this->store->cleanup(); | ||
| } | ||
| public function testCustomCacheWithoutLockFactory(): void |
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.
Novoid return in test classes
| /** | ||
| * @param null $store | ||
| */ | ||
| private function getCache($store = null): TagAwareAdapterInterface |
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.
Can we be more precise about the type here?
| // This test will fail if an exception is thrown, otherwise we mark it | ||
| // as passed. |
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.
Can be on one line
I think it makes more sense to target |
| $this->locks[$cacheKey] = $this->lockFactory | ||
| ->createLock($cacheKey); | ||
| return $this->locks[$cacheKey]->acquire(); |
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.
if lock is not acquired, should you unsetthis->locks[$cacheKey]?
one could retry to lock the resource several time until it works
Any news on this topic? Anything I can do to help? Thanks |
I'm very interested in this PR as well. I've thought a bit about the best path forward, but I'm not sure yet. |
To me, the mistake there was reusing the HttpCache store for the CachingHttpClient while it was designed for a reverse proxy cache, which has slightly different needs. |
I'm very aware of the issue@stof and this is part of my thoughts; I think I would like to start a new component that would include both types of proxies. |
As per@nicolas-grekas' request, here's a PR to merge myalternative PSR-6 HttpCache store into Symfony itself.
This is a simple copy & paste with a few namespace and test adjustments to get the discussion started for now.
Here are some additional thoughts:
Issues with the current
StoreInterface/HttpCacheDoes not support auto pruning which means it grows forever. This is has become an even bigger issue since the introduction of the
CachingHttpClientwhich also leverages theHttpCache. So you also grow endlessly big cache directories for your external HTTP requests with Symfony's HttpClient.Does not support cache tagging which is an extremely useful feature for reverse proxies.
Does some custom lock handling instead of relying on Symfony's Lock component.
Does not support caching
BinaryFileResponses.Issues with this PR, things to be clarified
It's not a 100% the same as my current implementation. Specifically, the current
StoreInterfacelacks the methods for the following three capabilities:clear()to be able to flush a cache completely.invalidateTags()to be able to invalidate certain cache items of a given set of cache tags only.prune()to be able to prune expired cache items.If you look at the things the
Psr6Storehas to do, it kind of becomes evident, thatHttpCachecurrently delegates too many responsibilities to theStoreInterface.Varyhandling and cache key generation e.g. should be the same for all stores.The current
CachingHttpClientrelies onHttpCacheas well and does not support greedy caching which is something that should be supported too, imho.I think, we should rather replace the current
HttpCachewith an alternative and deprecate the current one together with the currentStoreInterface. Imho that alternative could rely on the Symfony Cache abstraction directly. However, we should see if we can add the missing three capabilities to the Cache contracts as they are vital for a reverse proxy cache. We already haveTagAwareCacheInterfaceforinvalidateTags()but we're lackingprune()andclear()which would both make sense imho.List of things to do: