[WIP][HttpKernel] PSR6 Store for HttpCache #20061

jakzal · 2016-09-27T08:38:38Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	not yet

This is a WIP PR to get early feedback. I'd love to get this in for 3.2.

linaori · 2016-09-27T10:06:04Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    {
+        if (!$response->headers->has('X-Content-Digest')) {
+            $contentDigest = $this->generateContentDigest($response);
+            $item = $this->cacheBackend->getItem($contentDigest);


I would make this a guard clause instead. Please note that it should be @return string|null

Actually, I should create a CacheItem instead. No need to query the backend as we're going to overwrite the value anyway (if it's there).

linaori · 2016-09-27T10:06:24Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+use Symfony\Component\HttpFoundation\Response;
+
+class Psr6Store implements StoreInterface
+{


Any reason to not make it final?

Nope. Good idea.

ro0NL · 2016-09-27T10:15:00Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    /**
+     * @var CacheItemPoolInterface
+     */
+    private $cacheBackend;


What about $cachePool or even just $pool. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/DoctrineProvider.php#L24

For being not in the Cache namespace, i'd favor $cachePool.

Changed for consistency 👍

linaori · 2016-09-27T10:21:37Z

@jakzal this ticket is probably explaining the causation? #4871

jakzal · 2016-09-27T10:24:48Z

@iltar could be fixed by this PR indeed (expiration will be easier with backends like redis), but I was more thinking of making it more "small-website-prod-ready" (see #19705).

linaori · 2016-09-27T10:27:18Z

@jakzal yeah saw that issue as well when looking for the one mentioned. While this is not the best to use for production servers, it's a nice way to get started if you don't (yet) have a proper caching layer. I think this is a nice feature to have.

nicolas-grekas

Looks like a good start :) Would it make sense to make an AbstractStorage to share a few things with other storages implems?

nicolas-grekas · 2016-09-28T13:29:06Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+final class Psr6Store implements StoreInterface
+{
+    /**
+     * @var CacheItemPoolInterface


not needed, already hinted in the constructor

nicolas-grekas · 2016-09-28T13:29:29Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    private $cachePool;
+
+    /**
+     * List of locks acquired by the current process.


not sure this adds value

nicolas-grekas · 2016-09-28T13:29:39Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    /**
+     * List of locks acquired by the current process.
+     *
+     * @var array


nicolas-grekas · 2016-09-28T13:29:52Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    private $locks = array();
+
+    /**
+     * @param CacheItemPoolInterface $cachePool


already in the constructor

nicolas-grekas · 2016-09-28T13:30:44Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    }
+
+    /**
+     * Locates a cached Response for the Request provided.


{@inheritdoc}

nicolas-grekas · 2016-09-28T13:31:06Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    }
+
+    /**
+     * Writes a cache entry to the store for the given Request and Response.


{@inheritdoc}

nicolas-grekas · 2016-09-28T13:32:07Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+        $headers = $response->headers->all();
+        unset($headers['age']);
+
+        $this->save($key, serialize(array(array($request->headers->all(), $headers))));


serializing is already done by the cache pool

nicolas-grekas · 2016-09-28T13:32:32Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+    }
+
+    /**
+     * Invalidates all cache entries that match the request.


{@inheritdoc}

(same below :) )

nicolas-grekas · 2016-09-28T13:36:04Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+            return true;
+        }
+
+        return $this->cachePool->hasItem($this->getLockKey($request));


use $lockKey

nicolas-grekas · 2016-09-28T13:39:35Z

src/Symfony/Component/HttpKernel/HttpCache/Psr6Store.php

+     *
+     * @return CacheItem
+     */
+    private function createCacheItem($key, $value, $isHit = false)


This makes the implementation work only with Symfony's Cache components: cache items aren't interoperable.
You must get the item from the pool first (and maybe keep this as an optimization when Symfony Cache is used, another idea being to keep track of fetched items in this storage object to use them here)

This is kind of feedback i wanted to get (ignore comments etc, i left them there for my reference until I'm done and replace them with inheritdoc) ;)

You must get the item from the pool first

I did it first, perhaps I tried to optimise too early and replaced a call to pool with this. I'll revert back. Once it's all working we can run tests and see if it needs improving.

Having the optimization is OK if you keep the non-optimized version for other pools

jakzal · 2016-09-28T14:49:36Z

Would it make sense to make an AbstractStorage to share a few things with other storages implems?

I thought about it, but the current implementation is very coupled to the filesystem. I'm currently mirroring the behaviour, but the current implementation is optimised for the filesystem. I expect the psr6 implementation diverge eventually to be optimised for the cache pool.

I'll try this approach nevertheless, to see which one makes more sense.

fabpot · 2016-09-28T15:00:57Z

Just for the record, I'm very skeptical about this one. We explicitly tied the HTTP cache to the filesystem.

jakzal · 2016-09-28T15:20:45Z

@fabpot no worries. If it's rejected here, I'll create a separate library (so work won't be wasted).

Toflar · 2017-03-30T16:54:33Z

Now that there's a Lock component, do things get easier? @jakzal do you plan to continue the work on this?

Toflar · 2017-04-07T21:53:23Z

FYI: I added a PR to the fork of @jakzal to push this further: jakzal#2
I was not sure whether I should just open a new PR because I actually like working together on something so I figured I'd just PR against the PR 😄

Toflar · 2017-04-24T14:57:01Z

@jakzal any feedback here? Even if it's just a "sorry, no time, please open a new PR"? 😄

fabpot · 2017-07-06T09:03:54Z

I'm closing this one as I think this is a bad idea for several reasons:

Using a filesystem allows for opcache to make the cache very effective;
The cache contains some PHP (when using ESI for instance) and storing PHP in anything else than a filesystem would mean eval()ing strings coming from Redis/Memcache/...,
HttpCache is triggered very early and does not have access to the container or anything else really. And it should stay that way to be efficient.

So, for these reasons, it won't be in Symfony core.

Toflar · 2017-12-01T11:25:57Z

Leaving it here for people looking for a solution without it being in Symfony core: https://packagist.org/packages/toflar/psr6-symfony-http-cache-store

dunglas · 2018-02-08T14:52:21Z

Relying on PSR-6 has a nice side-effect: the ability to use cache tags (see api-platform/core#1689). Is supporting cache tags something we may want in core?

carsonbot added Status: Needs Review HttpKernel Feature labels Sep 27, 2016

linaori reviewed Sep 27, 2016

View reviewed changes

ro0NL reviewed Sep 27, 2016

View reviewed changes

jakzal force-pushed the http-kernel/psr6-http-cache-store branch from d0ee060 to 1ed4431 Compare September 27, 2016 10:21

jakzal force-pushed the http-kernel/psr6-http-cache-store branch from 1ed4431 to 8aac80d Compare September 27, 2016 10:26

jakzal force-pushed the http-kernel/psr6-http-cache-store branch 2 times, most recently from 7e548bb to 479b3f2 Compare September 27, 2016 13:04

[HttpKernel] Start an implementation of the PSR6 store for HttpCache

8a4c89b

jakzal force-pushed the http-kernel/psr6-http-cache-store branch from 479b3f2 to 8a4c89b Compare September 27, 2016 14:21

nicolas-grekas reviewed Sep 28, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

Toflar mentioned this pull request Mar 30, 2017

Symfony HTTP Cache / Tag Invalidation FriendsOfSymfony/FOSHttpCache#286

Closed

fabpot closed this Jul 6, 2017

Toflar mentioned this pull request Sep 1, 2017

Symfony taggable cache FriendsOfSymfony/FOSHttpCache#373

Closed

jakzal deleted the http-kernel/psr6-http-cache-store branch September 26, 2017 10:42

Toflar mentioned this pull request Feb 8, 2018

Support for easier registration of own cache purgers api-platform/core#1689

Closed

PabloKowalczyk mentioned this pull request Mar 25, 2021

Command for clearing old/stale http_cache (Symfony Reverse Proxy) entries #40548

Closed

alexander-schranz mentioned this pull request Dec 16, 2022

[HttpKernel] Cache component based HttpCache Store #48304

Closed

Uh oh!

[WIP][HttpKernel] PSR6 Store for HttpCache #20061

[WIP][HttpKernel] PSR6 Store for HttpCache #20061

Uh oh!

Conversation

jakzal commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linaori commented Sep 27, 2016

Uh oh!

jakzal commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori commented Sep 27, 2016

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakzal commented Sep 28, 2016

Uh oh!

fabpot commented Sep 28, 2016

Uh oh!

jakzal commented Sep 28, 2016

Uh oh!

Toflar commented Mar 30, 2017

Uh oh!

Toflar commented Apr 7, 2017

Uh oh!

Toflar commented Apr 24, 2017

Uh oh!

fabpot commented Jul 6, 2017

Uh oh!

Toflar commented Dec 1, 2017

Uh oh!

dunglas commented Feb 8, 2018

Uh oh!

Uh oh!

jakzal commented Sep 27, 2016 •

edited

Loading

nicolas-grekas Sep 28, 2016 •

edited

Loading