Symfony taggable cache #373

Toflar · 2017-08-31T16:39:45Z

Proposal for #286.
I actually did most of the work already a few months ago when trying to bring it to Symfony core so the TaggableStore was 80% ready and covered by all kinds of unit tests already 😄
It has a lot of unit tests, especially for proper Vary handling which I implemented quite different from what the Symfony default Store does. I did not use it in a project yet, so I have no idea if everything works as expected, I'm just pretty sure because of the unit tests. So it would be great if someone that actually has a project where he/she could use cache invalidation with tags could test this integration and see if the docs are clear 😄

I'll add a few comments/annotations to my own code in a few minutes.

Toflar · 2017-08-31T16:40:03Z

composer.json

@@ -36,6 +36,8 @@
        "php-http/mock-client": "^0.3.2",
        "symfony/process": "^2.3||^3.0",
        "symfony/http-kernel": "^2.3||^3.0",
+        "symfony/lock": "^3.4@dev",


These are require-dev.

Toflar · 2017-08-31T16:40:50Z

doc/symfony-cache-configuration.rst

+        SurrogateInterface $surrogate = null,
+        array $options = []
+    ) {
+        $store = new TaggableStore($kernel->getCacheDir());


Not sure if that's the best way to document but basically you just have to provide TaggableStore instead of having Symfony calling createStore().

Toflar · 2017-08-31T16:41:13Z

src/ProxyClient/Symfony.php

@@ -52,10 +53,26 @@ public function refresh($url, array $headers = [])
    protected function configureOptions()
    {
        $resolver = parent::configureOptions();
-        $resolver->setDefaults([


I've unified here so it matches the PurgeListener options.

Toflar · 2017-08-31T16:43:22Z

src/ProxyClient/Symfony.php

+     */
+    public function invalidateTags(array $tags)
+    {
+        // TODO: how should we escape best here?


I would like to escape , and maybe also new line but we need to unescape again in the PurgeListener so I'm not sure if using the same method as Varnish does is a good idea. Especially because Varnish just escapes by replacing both with a _ which makes it impossible to unescape again. To be honest, I find tags containing a , strange anyway so I'd prohibit this in the ResponseTagger upfront so it cannot happen at all but I guess that would be a BC break. Wdyt?

as far as i remember the logic with escaping was that we don't care about unescaping. the worst case would be that foo,bar and foo_bar collide, which would mean we invalidate too many things. its an edge case and one we decided we can live with, as its semantically correct (invalidating more then necessary is ok, the opposite would be a problem)

this works as long as escaping is done both when setting the tags and when creating invalidation requests.

I see. So I'll just escape and don't care about unescaping, right? It's an edge case anyway, I really don't know why I would ever use a comma in a tag.
We could, however, also use the same header multiple times. There's nothing wrong in using

X-Cache-Tags: foobar X-Cache-Tags: other tag

But I don't know if http-plug supports this?

i'd replace all , with _ but then collapse the tags into one header with , separation. in the listener, you can then still split by ,.

if i remember correctly, the varnish client even splits this into multiple requests if the list of tags is getting too long, to avoid problems with too long headers.

Implemented in b452710. Sorry, I'm too used to phpunit, I never used mockery so it was way faster for me to use the "native" mock stuff for consecutive argument assertions :)

Ah, and I implemented it so it's a more general solution. I thought that might be needed by other proxies too in the future. I did not adjust Varnish though because there's some regex stuff going on there and I figured that's maybe too specific.

Toflar · 2017-08-31T16:43:39Z

src/SymfonyCache/PurgeListener.php

+            && $store instanceof TaggableStore
+        ) {
+            // TODO: need to unescape again here
+            $tags = explode(',', $request->headers->get($this->purgeTagsHeader));


See my comment about unescaping in Symfony.

Toflar · 2017-08-31T16:44:00Z

src/SymfonyCache/TaggableStore.php

+    /**
+     * @return TagAwareAdapter
+     */
+    public function getCache()


Basically this is only needed for unit tests but it doesn't harm either.

i would use reflection in the unit test rather than expose this. we don't want people to interact with the tag cache from outside.

I agree. Fixed in d56f6a4.

Toflar · 2017-08-31T16:45:43Z

src/SymfonyCache/TaggableStore.php

+ *
+ * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ */
+class TaggableStore implements StoreInterface


Do you like that name? I wanted it to remain simple. The one the Symfony core provides is called Store so I just simply went for TaggableStore. Note that there's quite some stuff going on here. Proof that it works is a pretty extensive unit test suite for that class 😄

jderusse · 2017-08-31T17:05:21Z

src/SymfonyCache/TaggableStore.php

+        $cacheKey = $this->getCacheKey($request);
+
+        if (isset($this->locks[$cacheKey])) {
+            return false;


why not trying to acquire this lock ?

Basically, I checked the implementation of Store (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/HttpCache/Store.php#L69) so that's why it's the way it is. How should that whole method look like in your opinion?

So if you lock then release. You'll never be able to lock again... Is it the expected behavior?

I guess not :) You mean either we drop the isset() there or if we keep it as is, we need to unset($this->locks[$cacheKey]); in unlock()? :)

Yep, either unset in the release method. Or try to acquire it here. As you want

Fixed and covered by unit test in 5f7a1ea.

jderusse · 2017-08-31T17:06:31Z

src/SymfonyCache/TaggableStore.php

+    {
+        /** @var LockInterface $lock */
+        foreach ($this->locks as $lock) {
+            $lock->release();


you should wrap in a try/catch block

True, will add!

Added in 2f4b534.

jderusse · 2017-08-31T17:11:28Z

src/SymfonyCache/TaggableStore.php

+
+        $this->purgeTagsHeader = $purgeTagsHeader;
+        $this->cache = $cache = new TagAwareAdapter(new FilesystemAdapter('fos-http-cache', 0, $cacheDir));
+        $this->lockFactory = new Factory(new FlockStore($cacheDir));


The issue with Flock, is that we need to create an empty file for each key, and don't delete theme (for race condition).

Given we will create a lot of keys here, it will consume a lot of inode. I suggest tu use semaphore here and fallback to flock if semaphore is not supported. See https://github.com/symfony/symfony/blob/3ff0ff02accafaeb6f35eca5323bcb6be0f3311e/src/Symfony/Component/Console/Command/LockableTrait.php#L46-L50

Yeah, that's a general problem that is not specific to this implementation (it's the same in the default Store of HttpCache). I'll add semaphore though, thanks for the hint!

Fixed in 486008e.

jderusse · 2017-08-31T17:12:45Z

src/SymfonyCache/TaggableStore.php

+    }
+
+    /**
+     * @return TagAwareAdapter


what's about TagAwareAdapterInterface

See my comment in __construct(). We use a hardcoded implementation because we don't want other adapters to be used :)

jderusse · 2017-08-31T17:14:22Z

src/SymfonyCache/TaggableStore.php

+        }
+
+        $this->purgeTagsHeader = $purgeTagsHeader;
+        $this->cache = $cache = new TagAwareAdapter(new FilesystemAdapter('fos-http-cache', 0, $cacheDir));


Adapter could be injected instead of built (if you inject it, you should inject the lock too, because the injected cache can be a shared cache like redis, and the Lock should be shared too)

Both are not injectable on purpose. Otherwise it would be a fully PSR-6 compatible Store which was rejected in Symfony core as no other adapter except for file makes sense and might be dangerous using because of concurrency. See the discussion in #286 and the referenced PR in Symfony.

You can TypeHint with TagAwareInterface ? What if someone want to use redis tombe share cache between multiple front?

I guess you did not really understand: The users must not use any other adapter like redis etc. It must be forbidden. That's why it's hardcoded to the adapters that make sense.

The point of Fabien is to not use PSR6 and simply use $cached = include(...). (His comment give 3 good reasons).
As soon as you include cache component and use PSR6, I don't understand why adapters MUST be the Filesystem?

Here's the comment: symfony/symfony#20061 (comment)

Quote:

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/...,

So why would you allow anything else than Filesystem?

Because here you use a cache adapter on top of filesystem and you must deal with the same issue.

As soon as you start using PSR6, you are dealing with the same interface (same output, same evaluate things...) The only difference between adapter are performance vs scalability

Hmm, I still don't get your point, sorry 😄 So trying to sum up: There was a pull request for Symfony core allowing to use any PSR-6 cache back end. It was rejected because it would allow to use Redis, Memcache etc. as cache back ends. Ergo: everything except for a filesystem cache does not make sense. However, for me this does not mean we cannot use the cache component. We just have to make sure nobody can use anything else than the Filesystem adapter.

jderusse · 2017-08-31T17:16:08Z

src/SymfonyCache/TaggableStore.php

+     *
+     * @return bool|string true if the lock is acquired, the path to the current lock otherwise
+     */
+    public function lock(Request $request)


seems to be not used

What do you mean? It's part of the StoreInterface.

Oops.. nevermind

dbu

great job! i commented on a couple of things, but this looks really good to me.

i guess the most difficult question right now is about allowing to inject the cache implementation or not. i can see scenarios where it would indeed be a security problem to use memcache or redis, but i think there are also valid scenarios where its just as save as the filesystem (at least with redis which does password protect the connection)

dbu · 2017-09-01T12:18:37Z

doc/symfony-cache-configuration.rst

+Symfony's `HttpCache` does not support cache invalidation by tags by default.
+However, this library ships with a `TaggableStore` that provides exactly that.
+If you want to use this, adjust your `AppCache` as follows::
+


can you add a comment that specifies the filename? i guess // app/AppCache.php?

There are 2-3 examples before that all talk about the AppCache, I think it's redundant information :)

ah ok, the section above does the same. i find it still useful, but am ok with consistency here for now. might go over that doc at some point again.

dbu · 2017-09-01T12:19:29Z

doc/symfony-cache-configuration.rst

+
+    use FOS\HttpCache\SymfonyCache\TaggableStore();
+
+    // ...


please have the class AppCache extends HttpCache line here

Same here, I guess the people know about app/AppCache.php but I added a hint in 0e3d337 anyway (for the first example to not mess up with all other examples).

dbu · 2017-09-01T12:19:58Z

doc/symfony-cache-configuration.rst

+        HttpKernelInterface $kernel,
+        StoreInterface $store,
+        SurrogateInterface $surrogate = null,
+        array $options = []


where do these parameters come from?

I took the very same example as already used in the Cache event listeners section.

if we would use the one from FrameworkBundle, we could just overwrite the createStore method. i guess the reason is that here we are in the library and not the symfony bundle, so we don't want any extra dependencies. so this makes sense (apart from the $store parameter)

dbu · 2017-09-01T12:20:49Z

doc/symfony-cache-configuration.rst

+     */
+    public function __construct(
+        HttpKernelInterface $kernel,
+        StoreInterface $store,


can we remove this parameter? its never used as you then create the store manually.

Yeah well, these parameters depend on whether you use the HttpCache of the FrameworkBundle or HttpKernel directly. Not sure what to do here best but I mean, it's kind of obvious what you have to do there, isn't it? 😄

$store makes sense for normal constructor but not for this class, because its unused as you define $store below in the constructor code.

dbu · 2017-09-01T12:23:52Z

src/ProxyClient/Symfony.php

+     */
+    public function invalidateTags(array $tags)
+    {
+        // TODO: how should we escape best here?


as far as i remember the logic with escaping was that we don't care about unescaping. the worst case would be that foo,bar and foo_bar collide, which would mean we invalidate too many things. its an edge case and one we decided we can live with, as its semantically correct (invalidating more then necessary is ok, the opposite would be a problem)

this works as long as escaping is done both when setting the tags and when creating invalidation requests.

dbu · 2017-09-01T12:25:44Z

src/SymfonyCache/PurgeListener.php

+        $store = $event->getKernel()->getStore();
+
+        if ($request->headers->has($this->purgeTagsHeader)
+            && $store instanceof TaggableStore


should we not fail if there is a request to purge tags but we don't have a store supporting purge?

Matter of preference. I thought about that too. Either fail or ignore silently. You prefer failing?

its a basic setup error, not a runtime error that could occasionally happen, so yes i would explode loudly.

Implemented with the new listener now.

dbu · 2017-09-01T12:33:56Z

src/SymfonyCache/PurgeListener.php

-        if ($event->getKernel()->getStore()->purge($request->getUri())) {
+        $store = $event->getKernel()->getStore();
+
+        if ($request->headers->has($this->purgeTagsHeader)


why do we do tag invalidation in the same listener as purging? i feel like it should be a separate listener. in varnish, its glued into the ban handling because its a ban operation. for httpcache it seems to make sense to consider it not a BAN because its not a blacklist but a direct action onto the cache store. but purge sounds wrong to me too, because the path of the tag invalidation request does not matter. could we use a separate HTTP method like PURGETAGS? (this is an open question, i am not convinced it would be the best. but the mixup looks odd to me here)

Well, I can just tell what my idea behind this was: PURGE is not an official HTTP method by specification. If I was to pick one that exists, I would've probably gone with DELETE. Anyway, the spec allows to invent custom ones so yeah, we could use PURGETAGS. But then again, to purge, means to clean and so I checked the docs of Varnish (which I personally consider to be somewhat a market leader here) and it says:

A purge is what happens when you pick out an object from the cache and discard it along with its variants. Usually a purge is invoked through HTTP with the method PURGE.

And "tagging" to me is an object from the cache so imho it's perfectly fine. Also having it in the same listener is simple, doesn't change any behavior at all if you don't send that header, keeps things in place. I really like it where it is to be honest.

my problem is that just depending on the tags header, this either uses the url to purge a specific page, or it uses the header to purge all tagged things, and ignores the url. so basically the two things have no code in common. it shows in the nested if/elseif/else. i would prefer a separate listener for that reason.

for the naming i agree that the term purge is correct. but as the semantics of the purge payload is completely different, i'd rather use a separate http method name.

Okay, it's now in it's own PurgeTagsListener and the docs are updated accordingly.

dbu · 2017-09-01T12:39:58Z

src/SymfonyCache/TaggableStore.php

+    /**
+     * @return TagAwareAdapter
+     */
+    public function getCache()


i would use reflection in the unit test rather than expose this. we don't want people to interact with the tag cache from outside.

dbu · 2017-09-01T12:41:41Z

src/SymfonyCache/TaggableStore.php

+        try {
+            $this->locks[$cacheKey]->release();
+            unset($this->locks[$cacheKey]);
+        } catch (LockReleasingException $e) {


should we still unset if we get an exception here?

dbu · 2017-09-01T12:44:33Z

src/SymfonyCache/TaggableStore.php

+    }
+
+    /**
+     * Cleanups storage.


i am a bit confused by the name of the method, and this non-telling phpdoc. but just noticed that its the same on the interface. lets do {@inheritDoc} and then add a line Release all locks.

Toflar · 2017-09-01T13:17:00Z

i guess the most difficult question right now is about allowing to inject the cache implementation or not. i can see scenarios where it would indeed be a security problem to use memcache or redis, but i think there are also valid scenarios where its just as save as the filesystem (at least with redis which does password protect the connection)

I agree. Which is also why I supported the original PR and actually did a PR to finish it. I think it's not the task of this implementation to dictate a developer what to do. But then again it's also pretty fine if it helps you avoiding stupid mistakes 😄
I mean, I can allow injection, it's no problem...would also make the unit tests simpler and I could make the class final 😄

Toflar · 2017-09-01T15:13:37Z

I guess all the comments are addressed.
I've implemented setter injection in 16e4f22 for both, the cache and the lock factory and added phpdoc to explain why these are setter injections and not regular constructor injections. Personally, I feel like this should protect developers from blindly injecting their own implementations but read the method description first. I think this is probably the best way to protect the devs, provide sane defaults but still allow @jderusse and everybody else to adjust what they need. What do you think?

Toflar · 2017-09-03T18:32:46Z

I noticed in Symfony 3.4, the filesystem type cache adapters received a prune() method (see symfony/symfony#23603). Unfortunately the TagAwareAdpater does not yet but it will soon 😄 (see symfony/symfony#24075).
So I figured I would try to find some wanna-be-intelligent way on how to keep our store cleaned up. I count the number of write() calls now and if it reaches a certain threshold, the store is pruned 😄 How do you like that?

Toflar · 2017-09-04T20:50:53Z

Unit tests failing are not related to the lib. Seems like repo.varnish-cache.org is no more is the problem :( @dbu could you fix that in master so I can rebase? :)

dbu · 2017-09-08T13:16:15Z

i will look at this on monday.

dbu

great work! i have a couple of comments on the documentation and some other small change requests. but overall, this looks good to me.

it adds quite some more to the library, i am thinking whether it would make sense to provide a separate repository for the symfony HttpCache integration instead, where we can properly model the dependencies.

dbu · 2017-09-11T15:33:44Z

doc/response-tagging.rst

-If you need a different behavior, you can provide your own implementation of
-the ``TagHeaderFormatter`` interface. But be aware that your
-:ref:`Varnish configuration <varnish_tagging>` has to match with the tag on the response.
+As explained already, if you need a different behavior, you can provide your own


lets not say that we repeat here - it feels unfriendly ;-)

dbu · 2017-09-11T15:34:50Z

doc/response-tagging.rst

-the ``TagHeaderFormatter`` interface. But be aware that your
-:ref:`Varnish configuration <varnish_tagging>` has to match with the tag on the response.
+As explained already, if you need a different behavior, you can provide your own
+implementation of the ``TagHeaderFormatter`` interface. But again, be aware that your


lets not add "again".

maybe "provide your own TagHeaderFormatter instance"? does not need to be a custom implementation of the interface, like in the example below. (i know, that is just reformatting, but while we touch it...)

dbu · 2017-09-11T15:36:22Z

doc/symfony-cache-configuration.rst

+
+    You need at least Symfony 3.4 to use this feature!
+
+


extra blank line, please remove

dbu · 2017-09-11T15:37:30Z

doc/symfony-cache-configuration.rst

+
+Purge tags (cache invalidation using tags)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+


please add a .. versionadded:: 2.1.0 that says that cache tagging for AppCache has been added in FOSHttpCache 2.1.0

dbu · 2017-09-11T15:38:38Z

doc/symfony-cache-configuration.rst

+
+.. warning::
+
+    You need at least Symfony 3.4 to use this feature!


i think this is not strictly correct. you need symfony/lock and symfony/cache at 3.4, but afaik you can use symfony 3.3 together with those components, or am i wrong?

we should tell the composer require line for those two components, because they are optional dependencies.

Right, done.

dbu · 2017-09-11T15:49:23Z

src/SymfonyCache/TaggableStore.php

+     *
+     * @param string $cacheDir        The cache directory
+     * @param int    $pruneThreshold  The number of write operations until orphan data gets pruned (default: 500)
+     * @param string $purgeTagsHeader The tags header name


lets use an options resolver for the prune threshold and the header

To be honest, I don't like the options resolver. It's way too verbose. Real arguments are self documenting and better to read.

i disagree. the problem is that now if i want to change the header, i MUST specify some threshold. and if we add more options, the problem increases. imho optionsresolver is a very clean way to specify what is allowed, and includes validation like the threshold must be > 0 or the header must be a string without whitespace.

dbu · 2017-09-11T15:50:44Z

src/SymfonyCache/TaggableStore.php

+     *
+     * @return TaggableStore
+     */
+    public function setCache(TagAwareAdapterInterface $cache)


lets move this into the options. with the current implementation, the file system adapter is created and then replaced, which is just odd. i agree that we put a warning there - we should mention the "very good reasons" however, so that people can make an informed decision ;-)

dbu · 2017-09-11T15:51:34Z

src/SymfonyCache/TaggableStore.php

+     * that the local adapters are used by default and you do not inject them in
+     * the constructor but via setter injection instead. This is to protect you
+     * as a developer! Only use this method if you're really sure your lock
+     * implementation meets the needs of Symfony's HttpCache.


and same with the lock factory. mention that lock factory and cache storage must have the same scope e.g. on multi-server installations.

dbu · 2017-09-11T15:53:00Z

src/SymfonyCache/TaggableStore.php

+    /**
+     * @param string $cacheDir
+     *
+     * @return \Symfony\Component\Lock\StoreInterface


please use use statements for phpdoc as well.

I didn't feel like creating an alias for this one time usage. StoreInterface is already taken.

i would actually alias this to LockStoreInterface then. the point is that the use statements better list what things this class interacts with.

dbu · 2017-09-11T15:56:03Z

src/SymfonyCache/TaggableStore.php

+     * This only happens during write operations so cache retrieval is not
+     * slowed down.
+     */
+    private function pruneExpiredEntries()


good idea! i think its ok to have this active by default. but it should be possible to disable this by setting the threshold to -1, in which case we would not even get the cache item with the counter.

we should think of some way to trigger cache pruning out of band, like in a symfony command or such. that would be the better implementation if people really care about performance.

I chose 0 instead of -1 but it's now possible :-)

dbu · 2017-09-11T15:58:40Z

and i will look at the build error. aparently varnish changed where they provide their downloads :-(

ddeboer · 2017-09-11T19:44:19Z

Yeah, their packages are now hosted at packagecloud.io.

Toflar

Addressed your comments. Generally I agree to most of it, I don't want to add the options resolver though, that only makes things more complicated to understand in my opinion.

it adds quite some more to the library, i am thinking whether it would make sense to provide a separate repository for the symfony HttpCache integration instead, where we can properly model the dependencies.

I think that belongs to this library and I'm happy to help maintaining it :-)

Toflar · 2017-09-12T08:48:49Z

doc/response-tagging.rst

-If you need a different behavior, you can provide your own implementation of
-the ``TagHeaderFormatter`` interface. But be aware that your
-:ref:`Varnish configuration <varnish_tagging>` has to match with the tag on the response.
+As explained already, if you need a different behavior, you can provide your own


Toflar · 2017-09-12T08:48:56Z

doc/response-tagging.rst

-the ``TagHeaderFormatter`` interface. But be aware that your
-:ref:`Varnish configuration <varnish_tagging>` has to match with the tag on the response.
+As explained already, if you need a different behavior, you can provide your own
+implementation of the ``TagHeaderFormatter`` interface. But again, be aware that your


Toflar · 2017-09-12T08:49:12Z

doc/symfony-cache-configuration.rst

+
+Purge tags (cache invalidation using tags)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+


Toflar · 2017-09-12T08:49:20Z

doc/symfony-cache-configuration.rst

+
+.. warning::
+
+    You need at least Symfony 3.4 to use this feature!


Right, done.

Toflar · 2017-09-12T08:49:29Z

doc/symfony-cache-configuration.rst

+
+    You need at least Symfony 3.4 to use this feature!
+
+


Toflar · 2017-09-12T08:50:50Z

doc/symfony-cache-configuration.rst

+
+* **purge_tags_header**: HTTP Header that contains the comma-separated tags to purge.
+
+  **default**: ``X-Cache-Tags``


I copied it from within the same doc here (chapter Refresh).

Toflar · 2017-09-12T09:00:51Z

doc/symfony-cache-configuration.rst

+    This means that after every `500` HTTP cache writes, your file system directory
+    will be cleaned up and thus kept in good shape.
+    You can configure the prune threshold by providing a different threshold as
+    second argument to the constructor of `TaggableStore`.


Toflar · 2017-09-12T09:03:17Z

src/SymfonyCache/TaggableStore.php

+     *
+     * @param string $cacheDir        The cache directory
+     * @param int    $pruneThreshold  The number of write operations until orphan data gets pruned (default: 500)
+     * @param string $purgeTagsHeader The tags header name


To be honest, I don't like the options resolver. It's way too verbose. Real arguments are self documenting and better to read.

Toflar · 2017-09-12T09:04:30Z

src/SymfonyCache/TaggableStore.php

+    /**
+     * @param string $cacheDir
+     *
+     * @return \Symfony\Component\Lock\StoreInterface


I didn't feel like creating an alias for this one time usage. StoreInterface is already taken.

Toflar · 2017-09-12T09:08:01Z

src/SymfonyCache/TaggableStore.php

+     * This only happens during write operations so cache retrieval is not
+     * slowed down.
+     */
+    private function pruneExpiredEntries()


I chose 0 instead of -1 but it's now possible :-)

dbu · 2017-09-13T06:14:54Z

src/SymfonyCache/TaggableStore.php

+        );
+    }
+
+    /**


can you add some text here? actually, i would like to change this method to getDefaultLockFactory and let it return a lock factory instead. with a phpdoc:

Build and return a default lock factory for when no explicit factory is specified. The default factory uses the best quality lock store that is available on this system.

dbu · 2017-09-13T06:15:41Z

src/SymfonyCache/TaggableStore.php

+     * Constructor.
+     *
+     * @param string $cacheDir        The cache directory
+     * @param int    $pruneThreshold  The number of write operations until orphan data gets pruned (default: 500)


the magic value 0 needs to be mentioned here

dbu · 2017-09-13T06:15:55Z

src/SymfonyCache/TaggableStore.php

+    private $locks = [];
+
+    /**
+     * Constructor.


please remove this line, it does not add any information ;-)

Toflar · 2017-09-13T09:46:35Z

@dbu addressed all your comments and switched to options resolver. Tests still failing due to wrong package source which I don't like to fix in that PR because it does not belong here :-)

dbu

Tests still failing due to wrong package source which I don't like to fix in that PR because it does not belong here :-)

i work on that in #375, agreed to not mix this into this PR.

i have some more comments on the documentation. don't want to make things complicated, but its tricky to make understandable documentation, i am afraid. i hope to figure out #375 and then you can rebase this on master and we should be ready.

can you please add an entry to CHANGELOG.md too?

dbu · 2017-09-14T06:17:54Z

doc/symfony-cache-configuration.rst

+
+.. versionadded:: 2.1
+
+    The `TaggableStore` has been added in version 2.1.


in rst, you need to use two backticks for code formatting. a single backtick does not work (it only works in markdown). can you check that for all of the doc you are adding please?

dbu · 2017-09-14T06:18:45Z

doc/symfony-cache-configuration.rst

+    to use this feature! Add the following lines to your `composer.json` and run
+    `composer update`::
+
+    "symfony/lock": "^3.4",


please indent this by another 4 spaces

dbu · 2017-09-14T06:20:10Z

doc/symfony-cache-configuration.rst

+  Note that there are very good reasons that the local adapters are used by
+  default. This is to protect you as a developer! Only override it if you're
+  really sure your lock implementation meets the needs of Symfony's HttpCache.
+  Type: Factory


i think we need a fully qualified name with namespace here

dbu · 2017-09-14T06:20:17Z

doc/symfony-cache-configuration.rst

+  Note that there are very good reasons that the local adapters are used by
+  default. This is to protect you as a developer! Only override it if you're
+  really sure your cache implementation meets the needs of Symfony's HttpCache.
+  Type: TagAwareAdapterInterface


lets use a FQN here

dbu · 2017-09-14T06:24:06Z

doc/symfony-cache-configuration.rst

+
+.. _taggablestore:
+
+The TaggableStore


i think this location makes the taggable store a sub section of user context, which is not correct. i would make it a title on the same level as those, using ~. and lets place it right after purge tags.

dbu · 2017-09-14T06:25:40Z

doc/symfony-cache-configuration.rst

+
+        parent::__construct($kernel, $store, $surrogate, $options);
+
+        $this->addSubscriber(new PurgeTagsListener());


i would put the code example with the purge tags section, and mention the options of the PurgeTagsListener there as well (purge http method and such)

but keep the information on options for the taggable store here.

dbu · 2017-09-14T06:28:23Z

src/SymfonyCache/TaggableStore.php

+    public function cleanup()
+    {
+        try {
+            /** @var LockInterface $lock */


the docblock @var LockInterface[] on the $locks property should be enough. at least for phpstorm it is...

dbu · 2017-09-14T06:40:16Z

quick question to coordinate the FOSHttpCacheBundle release: do we need additional configuration for the tag purge header in the symfony cache proxy configuration? if so, i will wait with tagging a new version of the bundle until we wrapped this up and i tagged a library release.

dbu · 2017-09-14T07:55:31Z

ok, fixed the varnish install in master. can you please rebase?

…leStore

Toflar · 2017-09-14T14:38:41Z

Rebased. Updated docs. Added Changelog entry.

quick question to coordinate the FOSHttpCacheBundle release: do we need additional configuration for the tag purge header in the symfony cache proxy configuration?

Yeah would be a great addition. But I don't feel like this is a must. The default works out of the box and if you want to adjust the header name (for whatever reason you would like to do that) then you have to submit a PR adding support for that in the bundle 😄

dbu · 2017-09-19T06:28:00Z

thanks a lot. this looks good now! i want to do some cleanups and will then merge this later this week and ideally tag a release.

Toflar · 2017-09-19T07:43:05Z

Cool! I think we're not in a hurry here as Symfony 3.4 is still 1.5 months away.
I really hope this alternative HttpCache Store implementation will help out many people in the Symfony community 👍

javiereguiluz · 2017-09-19T09:08:50Z

@Toflar for those of us who can't review this code or follow this long discussion, could you please summarize as concisely as possible the advantages of this feature over the existing Symfony Http Cache and also, if there's any, the drawbacks or shortcomings compared to Symfony? Thanks!

Toflar · 2017-09-19T09:24:48Z

Hey @javiereguiluz, sure I can give it a try :)

As concisely as possible: It supports tag based cache invalidation and auto-pruning of local cache entries and builds on top of the latest Symfony components that were not available by the time the Store shipped with Symfony was developed.

And here are some more details, if you have time:

First off: If you are not familiar with the FOS/HttpCache library, you can ignore everything in the PR except for the TaggableStore (and its tests of course).

The TaggableStore does not replace the existing Symfony HttpCache but is just a different implementation of the StoreInterface the HttpCache uses than the one shipped with Symfony.
Instead of re-implementing locking and caching, it uses the well tested Symfony Cache and Lock components, both with the local filesystem adapters by default.
Thanks to the TagAwareAdapter of the Cache component, it supports tag based cache invalidation.
Thanks to the PrunableInterface of the Cache component, added in Symfony 3.4, it supports auto-pruning of the expired entries on the filesystem trying to prevent flooding the filesystem.
For the sake of completeness: Thanks to the Cache and Lock components, it also allows to use a different cache or lock adapter than the local filesystem. However, as stated many times all over the different projects, developers should be careful about choosing the right adapter as some of them are definitely not well suited for the purpose of HttpCache and you're better of switching to a "real" reverse proxy as Varnish or LiteSpeed Cache. That's why local adapters are default and usually the best choice.

javiereguiluz · 2017-09-19T09:56:31Z

@Toflar fantastic explanation! Thanks!!

dbu · 2017-09-19T10:10:28Z

@javiereguiluz toflar tried a pull request to make the default store of symfony use the cache + lock components, but that got refused. for the tag capabilities, we need something and i like this solution a lot more as its built on existing and tested components.

but if you have any kind of feedback on it, i would be glad to hear it, now we can still easily change things.

javiereguiluz · 2017-09-19T10:13:48Z

@dbu I don't care about tags because my HTTP Cache is super basic ... but "auto-pruning of the expired entries" is a super-killer feature to me. Otherwise, I must clean up the http_cache/ dir from time to time because nothing is deleted and it grows to GB of data every day :( We've received in the Symfony repository countless reports about this behavior.

Toflar · 2017-09-19T11:13:58Z

Thinking about it, it would maybe make sense to have the TaggableStore in its own library as I feel @javiereguiluz would use it but does not need anything else the FOSHttpCache library provides. Am I right about that?
And yes, I still think it would make sense to replace the default Store in Symfony with it but to be honest I have invested many, many hours in the PR against Symfony, then in this PR and the whole concept as such so I'm kind of happy I'm close to finally using it in my projects. So another PR for Symfony is out of scope as of today but a separate library would maybe make sense and should not be too big of a deal.

Toflar · 2017-09-26T10:36:33Z

Ping @javiereguiluz: any comment on having a standalone store implementation?

javiereguiluz · 2017-09-26T10:43:04Z

I can't make technical comments because I haven't reviewed this in detail ... but what I need (and lots of others, according to the community feedback) is a Symfony HttpCache store which doesn't grow in size without limits and which doesn't store expired entries forever.

Ideally, your changes would be merged in Symfony itself so we can benefit from it without having to do or install anything. Thanks!

dbu · 2017-09-26T15:37:44Z

at this point, i think a separate repository would be really cool! then the store should just be called Toflar/PsrCacheStore imho. the fact that it supports tags is free from the fact that it uses PSR-6. its important for FOSHttpCache but the main feature of the store is using PSR-6 caches.

@javiereguiluz toflar first tried a PR against symfony core but that got refused by fabien with this comment. i can see the concern, but think its a bit over-cautious and seeing how TaggableStore has less code thanks to the lock and cache components i feel it would be progress. lets see if this will change sometimes.

@Toflar i did a bit of refactoring on the store. i will push as soon as i got all tests fixed. if you agree, i suggest you then copy them from my branch into a repo, along with the documentation on the store. your composer.json can even properly specify that it requires symfony/cache and symfony/lock. and then i can adjust the FOSHttpCache doc to tell people to install toflar/symfony-cache-store (or whatever you want to call it) and reference the correct namespace. the tag listener for HttpCache would stay in this repository i think.

would that make sense?

dbu · 2017-09-28T18:06:59Z

continued in #376.

Toflar commented Aug 31, 2017

View reviewed changes

Toflar mentioned this pull request Aug 31, 2017

Symfony HTTP Cache / Tag Invalidation #286

Closed

Toflar force-pushed the symfony-taggable-cache branch from de5267b to d7f239c Compare August 31, 2017 16:50

jderusse reviewed Aug 31, 2017

View reviewed changes

dbu reviewed Sep 1, 2017

View reviewed changes

Toflar force-pushed the symfony-taggable-cache branch from 69233c1 to be66e36 Compare September 3, 2017 18:30

Toflar mentioned this pull request Sep 4, 2017

clarify user context cache impact #360

Merged

dbu reviewed Sep 11, 2017

View reviewed changes

Toflar commented Sep 12, 2017

View reviewed changes

dbu reviewed Sep 13, 2017

View reviewed changes

Toflar force-pushed the symfony-taggable-cache branch from 35f4d93 to 454a6b1 Compare September 13, 2017 09:40

dbu reviewed Sep 14, 2017

View reviewed changes

Toflar added 6 commits September 14, 2017 16:16

Unified options resolver of Symfony to match the PurgeListener

0841af1

Implemented handling purge request if the store is instance of Taggab…

e557658

…leStore

Implemented a TaggableStore

c173b23

CS

c7b6129

Use the SemaphoreStore for locking if available

577357a

Fixed locking of same request

318b357

Toflar added 10 commits September 14, 2017 16:16

Implemented a proposal to keep the storage clean

e0bd723

CS

e91fb5e

Added some more tests

e0f7749

Docs

10ba80d

Put all the docs to separate section

e21f5f6

Implemented support to disable the pruning

b4d8b3d

CS

14680c7

Switched to options resolver

5091e56

Adjusted docs

dcb6316

Documented the options also in docs

56e2d74

Toflar force-pushed the symfony-taggable-cache branch from 7e6ceda to 56e2d74 Compare September 14, 2017 14:16

Toflar added 3 commits September 14, 2017 16:22

Double backticks

933c9ec

Improved docs

4769437

Added changelog entry

cd7bfe0

dbu mentioned this pull request Sep 26, 2017

Symfony taggable cache #376

Merged

1 task

dbu closed this Sep 28, 2017

Toflar deleted the symfony-taggable-cache branch September 28, 2017 18:08


		Purge tags (cache invalidation using tags)
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


		.. warning::

		You need at least Symfony 3.4 to use this feature!


		* purge_tags_header: HTTP Header that contains the comma-separated tags to purge.

		default: ``X-Cache-Tags``

+                      );
+                  }
+                  /**


		.. versionadded:: 2.1

		The `TaggableStore` has been added in version 2.1.


		parent::__construct($kernel, $store, $surrogate, $options);

		$this->addSubscriber(new PurgeTagsListener());

Symfony taggable cache #373

Symfony taggable cache #373

Conversation

Toflar commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Sep 1, 2017 • edited Loading

Toflar commented Sep 1, 2017

Toflar commented Sep 3, 2017

Toflar commented Sep 4, 2017

dbu commented Sep 8, 2017

dbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Sep 1, 2017 •

edited

Loading