Support for easier registration of own cache purgers #1689

Toflar · 2018-02-08T13:35:08Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	TODO
Fixed tickets
License	MIT
Doc PR	TODO

This will make it pretty easy to implement and register custom cache purgers as follows:

http_cache:
    invalidation:
        enabled: true
        purger: my_superb_cache_purger
        tags_formatter: my_superb_cache_tags_formatter

This will then alias the api_platform.http_cache.purger to my_superb_cache_purger.

In addition to this, I've introduced a CacheTagsFormatterInterface that allows to delegate the formatting of the $iris to the purger and a Cache-Tags-Debug header that contains them unformatted in case kernel.debug is set to true.

dunglas

Great! Definitively a good improvement.

dunglas · 2018-02-08T14:01:34Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

@@ -498,6 +498,12 @@ private function registerHttpCache(ContainerBuilder $container, array $config, X

        $loader->load('http_cache_tags.xml');

+        // TODO: Decide on how you would like to configure whether to use Varnish or Symfony (or any other?)
+        if (0 == count($config['http_cache']['invalidation']['varnish_urls'])) {


We may allow to use both (to avoid such ifs). If a varnish_urls is defined, it's definitely to use Varnish.

So how would your preferred config look like? If varnish_urls then Varnish, else Symfony? What if we want to integrate another one?

dunglas · 2018-02-08T14:02:38Z

src/HttpCache/EventListener/AddTagsListener.php

+        // Encode tags for greater compatiblity with different proxies
+        // Some do not allow special characters like / or @ in cache tags and
+        // also it allows to use a , in a tag, if you wish to do so.
+        $resources = array_map(function($resource) {


We were using md5 initially, we switched back to plain text because it is very hard to debug when reading/grepping logs. I would prefer to encode special char at the cache adapter layer.

I can do that but to remain BC I need to introduce another interface for the Purgers that implement both PurgerInterface and...hmmm... CacheTagsFormatterInterface?

dunglas · 2018-02-08T14:04:24Z

src/HttpCache/StoreAwareKernelInterface.php

@@ -0,0 +1,36 @@
+<?php


Why do we need this interface and the related trait? Can't we just use inject the PSR-6 adapter in the purger?

No that's not possible. HttpCache works outside the application so you cannot access it from within the container.

We could implement the same as Varnish does. Have the purger execute a PURGE request and have a kernel.request listener that purges the data. But that's way too complicated I find.

dunglas · 2018-02-08T14:14:04Z

src/HttpCache/SymfonyPurger.php

+
+namespace ApiPlatform\Core\HttpCache;
+
+use Toflar\Psr6HttpCacheStore\Psr6StoreInterface;


Wouldn't it be possible to move this new implementation directly in Symfony?

No, I tried and it was denied. See symfony/symfony#20061 for the whole discussion.

Ok... I'm not confident with adding (soft) dependencies to non-popular yet libraries (like Psr6HttpCacheStore). We already had problems with bridges to 3rd party libraries not very well maintained (FOS things...).

My proposal is:

Tweak the config in this PR to allow to easily register external Purgers from the config (using a service name for instance).

Move the SymfonyPurger class and the trait in an external bundle (or directly in Psr6HttpCacheStore) for now

WDYT?

Yeah I understand. In that case, it's my library and it consists of one class so I can basically guarantee it's maintained because I can't think of a lot of stuff that would need to be maintained there 😄 But anyway, I just feel it can never get popular if we don't give it the chance. I mean, I'm totally okay with putting the APIP integration into it's own bundle if you don't want it in core but then it will be just another one floating around in my personal namespace instead of APIP so chances are, people won't even notice there's such a thing.

It's mostly a docs issue to me. I'm totally ok to add a new doc entry explaining how to install your lib (that will be required to install manually anyway) to get cache invalidation in pure PHP.

The Kernel should be in whatever namespace.

If the purger is implementing our PurgerInterface yeah it can be in a Bridge/ApiPlatform thingy

Okay, no problem. There's one thing that needs to be clarified first though: the encoding of the tags. I see two options here:

I add a new interface every purger can implement that allows to ask the purger to encode the tags whereas fallback would be the current, comma-separated list. Pros: Very flexible, Cons: more complicated.

We use md5 or sha1 by default and add a Cache-Tags-Debug header which shows the raw tags if kernel.debug is enabled. I cannot think of any cache implementation that cannot handle hashes as keys. Pros: Simple, Cons: Less flexible.

WDYT?

I'd go for option 1

Okay, would you still add the additional debug header? I think it might be very useful and it doesn't harm because it's hidden in production anyway.

Sure I think it's a good idea as well!

Toflar · 2018-02-12T16:38:41Z

Changed title, description, implementation, rebased on master branch. Ready for review round 2 😄

dunglas · 2018-02-12T17:52:16Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

@@ -498,6 +498,14 @@ private function registerHttpCache(ContainerBuilder $container, array $config, X

        $loader->load('http_cache_tags.xml');

+        // Load purger
+        $purger = $config['http_cache']['invalidation']['purger'];
+        $container->setAlias('api_platform.http_cache.purger', 'api_platform.http_cache.purger.'.$purger);


Can you use "api_platform.http_cache.purger.$purger" to trigger https://blog.blackfire.io/php-7-performance-improvements-encapsed-strings-optimization.html

Oh nice one! I've read a lot about opcode optim but must have missed that one! Done in f4aec2e.

dunglas · 2018-02-12T17:55:03Z

src/HttpCache/EventListener/AddTagsListener.php


-    public function __construct(IriConverterInterface $iriConverter)
+    public function __construct(IriConverterInterface $iriConverter, PurgerInterface $purger, bool $debug = false)


Can't we just typehint CacheTagsFormattingPurgerInterface directly?

No because the current VarnishPurger does not implement that and I don't know what other implementations have been made out there. Also it's completely optional to adjust the formatting of the tags :)

I don't get it. The dependency wasn't existent before. CacheTagsFormatterInterface $formatter = null will do the trick. Btw we try to avoid setter injection as much as possible.

dunglas · 2018-02-12T17:55:52Z

src/HttpCache/CacheTagsFormattingPurgerInterface.php

+ *
+ * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ */
+interface CacheTagsFormattingPurgerInterface extends PurgerInterface


WDYT about not extending PurgerInterface and renaming it CacheTagsFormatterInterface? It will allow to extract this logic in a separated class if necessary.

Yeah, thought about that too but then again, where would you put that? It should not reside in the HttpCache namespace etc. so I thought I'm going to create a very specific interface for that purpose only.

Why not in HttpCache? This namespace looks good to me.

dunglas · 2018-02-12T17:58:14Z

src/HttpCache/EventListener/AddTagsListener.php


-    public function __construct(IriConverterInterface $iriConverter)
+    public function __construct(IriConverterInterface $iriConverter, PurgerInterface $purger, bool $debug = false)


Can you make the purger (formatter) optional? It's useless when using the builtin one (it eases to use classes outside of Symfony / API Platform full stack). + it's a BC break

dunglas · 2018-02-12T17:59:17Z

src/HttpCache/EventListener/AddTagsListener.php

@@ -66,6 +73,16 @@ public function onKernelResponse(FilterResponseEvent $event)
            return;
        }

-        $event->getResponse()->headers->set('Cache-Tags', implode(',', $resources));
+        if ($this->debug) {


I'm not sure it is worth it. The Formatter can do it by itself by using RequestStack (and it's useless for the default purger).

By the way, even in your purger, I suggest to emit non-encoded headers, and to do the encoding/decoding at the cache store level.

It's just an easy debugging help without any negative side effects.
It's the Symfony Cache that does not allow certain characters, that comment should probably go there :-) I still think it's fine here. Sometimes you don't have control over the cache internals and then you're lost without that. I will make both optional using setter injection though.

Sorry to insist but please drop it, it brings nothing for the default setup (cache tags are already in clear text) and may cause maintenance issues (I don't like injecting the debug parameter in this listener). It can easily be added in an external class/bundle.

Toflar · 2018-02-13T10:43:41Z

PR is ready for another review then :)

meyerbaptiste · 2018-03-06T13:43:34Z

src/HttpCache/EventListener/AddTagsListener.php


-    public function __construct(IriConverterInterface $iriConverter)
+    public function __construct(IriConverterInterface $iriConverter, CacheTagsFormattingInterface $purger = null, bool $debug = false)


$purger as variable name does not seem appropriate here.

True, fixed in a1b08de

meyerbaptiste · 2018-03-06T13:59:53Z

src/HttpCache/CacheTagsFormattingInterface.php

+ *
+ * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ */
+interface CacheTagsFormattingInterface


CacheTaggingFormatterInterface?

I don't think that's correct. The implementation of that interface "formats the cache tags". So "CacheTagsFormatting" is correct. "CacheTaggingFormatter" sounds really weird to me.

"formats the cache tags"

So... What do you think of CacheTagsFormatterInterface? 😄

Changed 😄

meyerbaptiste · 2018-03-06T14:20:25Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -207,6 +207,7 @@ public function getConfigTreeBuilder()
                            ->info('Enable the tags-based cache invalidation system.')
                            ->canBeEnabled()
                            ->children()
+                                ->scalarNode('purger')->defaultValue('varnish')->cannotBeEmpty()->info('The name of the purger to use.')->end()


IMO, it should be directly the name of the service instead of a tricky concatenation (i.e api_platform.http_cache.purger.$purger). Or did I miss something specific?
So, create an implementation of your new interface for Varnish whose the service name will be the default value here.

WDYT?

Addressed in 2e5bbbf.

Toflar · 2018-03-09T09:04:52Z

Rebased onto master.

dunglas · 2018-03-09T16:31:50Z

src/HttpCache/EventListener/AddTagsListener.php

+
+        // BC
+        if (null === $this->tagsFormatter) {
+            @trigger_error('Passing no implementation of the CacheTagsFormatterInterface is deprecated since version 2.3 and will be removed in 3.0.', E_USER_DEPRECATED);


Please don't trigger a deprecation notice here. It makes it header to use for no benefit.

dunglas · 2018-03-09T16:32:10Z

src/HttpCache/EventListener/AddTagsListener.php

    {
        $this->iriConverter = $iriConverter;
+        $this->tagsFormatter = $tagsFormatter;


$this->tagsFormatter = $tagsFormatter ?? new CsvFormatter();

Or even better:

$this->tagsFormatter = $tagsFormatter ?? function (array $iris): string { return implode(',', $iris); };

dunglas · 2018-03-09T16:33:56Z

src/HttpCache/CsvFormatter.php

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


I'm think it is not worth it to add a new class for an implode (see my other suggestion).

dunglas · 2018-03-09T16:35:35Z

src/HttpCache/CacheTagsFormatterInterface.php

+ *
+ * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ */
+interface CacheTagsFormatterInterface


What about just using callable. For this use case it's enough IMO, no need to introduce an interface.

Callables are not a good choice at all here. They are very hard to add. Right now you can simply configure a different service by using the tags_formatter config setting and you're done.

Formatting tags is a very specific edge case to me, ok to allow to do it, but I'm not confortable to add so many things just for this. A callable looks perfect for this use case.

I've always said it's something that's specific to the purger because the purger knows how to format the tags so it can actually purge them. It never makes sense to arbitrarily combine purgers and tag formatters. That's why I had interface CacheTagsFormattingPurgerInterface extends PurgerInterface and passed the purger to the AddTagsListener. That way, any purger can format if it needs but it can also just leave this interface alone and go with the default implementation which is just a CSV list. This is still the version that makes the most sense from a software architectural POV to me because responsibilities are put where they belong.

dunglas · 2018-03-09T16:39:30Z

src/HttpCache/EventListener/AddTagsListener.php

@@ -66,6 +73,16 @@ public function onKernelResponse(FilterResponseEvent $event)
            return;
        }

-        $event->getResponse()->headers->set('Cache-Tags', implode(',', $resources));
+        if ($this->debug) {


Sorry to insist but please drop it, it brings nothing for the default setup (cache tags are already in clear text) and may cause maintenance issues (I don't like injecting the debug parameter in this listener). It can easily be added in an external class/bundle.

dunglas

Please see the comments

Toflar · 2018-03-09T16:41:59Z

I've put the formatting into it's own class because of the discussion in Slack. I don't really feel like changing it yet again.

Toflar · 2018-03-09T16:45:44Z

Debugging is gone.

…ers and improved debug output

… injection instead of setter injection

dunglas · 2018-03-09T16:52:41Z

I can finish this if you want @Toflar

Toflar · 2018-03-09T16:53:28Z

No, it will just have to wait for after my holidays. And callables are the wrong choice to me 😄

Simperfit · 2018-04-06T13:56:14Z

src/HttpCache/EventListener/AddTagsListener.php

+
+        // BC
+        if (null === $this->tagsFormatter) {
+            @trigger_error('Passing no implementation of the CacheTagsFormatterInterface is deprecated since version 2.3 and will be removed in 3.0.', E_USER_DEPRECATED);


Passing an object that does not implement CacheTagsFormatterInterface is deprecated since version 2.3 and will be removed in 3.0. WDYT ?

Simperfit

Thanks for doing this!

Could you please update with master ;).

Toflar · 2018-04-07T11:49:28Z

I would like to rework it completely as outlined in #1776.

Toflar · 2018-04-07T11:49:38Z

But haven't gotten any feedback there yet.

dunglas · 2018-04-23T12:58:19Z

@Toflar ok for me!

soyuka · 2019-02-15T14:26:25Z

Should I do something with this @Toflar ?

Toflar · 2019-02-15T14:30:34Z

I think that can be closed. I've worked a lot on the FOSCache lib and bundle lately and I found that it would be better to tie tagging and purging to the same service so it would require a complete overhaul anyway.

soyuka · 2019-02-15T15:50:31Z

Thanks for the quick response @Toflar !

dunglas requested changes Feb 8, 2018

View reviewed changes

dunglas mentioned this pull request Feb 8, 2018

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

Closed

Toflar force-pushed the symfony-cache-invalidation branch from 80d3ac0 to 8158d69 Compare February 12, 2018 16:05

Toflar changed the title ~~Cache invalidation support with PHP only~~ Support for easier registration of own cache purgers Feb 12, 2018

Toflar force-pushed the symfony-cache-invalidation branch from 8158d69 to 149b857 Compare February 12, 2018 16:38

dunglas requested changes Feb 12, 2018

View reviewed changes

Toflar force-pushed the symfony-cache-invalidation branch 2 times, most recently from f2fdacb to 24b7f18 Compare February 13, 2018 09:47

meyerbaptiste requested changes Mar 6, 2018

View reviewed changes

Toflar force-pushed the symfony-cache-invalidation branch from 2e5bbbf to 0b80aa7 Compare March 9, 2018 09:04

Toflar force-pushed the symfony-cache-invalidation branch from 02b5c40 to ebf4369 Compare March 9, 2018 13:43

dunglas requested changes Mar 9, 2018

View reviewed changes

dunglas approved these changes Mar 9, 2018

View reviewed changes

dunglas requested changes Mar 9, 2018

View reviewed changes

Toflar added 11 commits March 9, 2018 17:47

Draft implementation for cache invalidation support with PHP only

a349c2e

Allow to easily register custom cache purgers with own header formatt…

ba9b2ea

…ers and improved debug output

Trigger opcode string optim

fcc9917

Use setter injection for the AddTagsListener

b2e435a

Extracted CacheTagsFormattingInterface and used optional construction…

fc13aae

… injection instead of setter injection

Use a more appropriate variable name

85a6e3a

Use a service name for the purger

7e59977

Renamed interface

d63bbf2

Allow to configure the tags formatter

259c53e

Moved csv formatting logic into its own class and implemented BC

7ca5f06

Removed debugging support

31fea1a

Toflar force-pushed the symfony-cache-invalidation branch from 6f046ed to 31fea1a Compare March 9, 2018 16:48

dunglas mentioned this pull request Mar 14, 2018

[RFC] Use xkey to purge cache #1776

Closed

Simperfit reviewed Apr 6, 2018

View reviewed changes

Simperfit added the [WIP] Work In Progress label Apr 6, 2018

dunglas mentioned this pull request Apr 23, 2018

Varnish required? #1881

Closed

Toflar closed this Feb 15, 2019

Toflar deleted the symfony-cache-invalidation branch February 15, 2019 15:51


		namespace ApiPlatform\Core\HttpCache;

		use Toflar\Psr6HttpCacheStore\Psr6StoreInterface;


		public function __construct(IriConverterInterface $iriConverter)
		public function __construct(IriConverterInterface $iriConverter, PurgerInterface $purger, bool $debug = false)

Support for easier registration of own cache purgers #1689

Support for easier registration of own cache purgers #1689

Conversation

Toflar commented Feb 8, 2018 • edited Loading

dunglas 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

Toflar commented Feb 12, 2018

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

dunglas Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Feb 12, 2018 • edited Loading

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 Feb 13, 2018

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 Mar 9, 2018

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

dunglas Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

Toflar commented Mar 9, 2018

Toflar commented Mar 9, 2018

dunglas commented Mar 9, 2018

Toflar commented Mar 9, 2018

Choose a reason for hiding this comment

Simperfit left a comment • edited Loading

Choose a reason for hiding this comment

Toflar commented Apr 7, 2018

Toflar commented Apr 7, 2018

dunglas commented Apr 23, 2018

soyuka commented Feb 15, 2019

Toflar commented Feb 15, 2019

soyuka commented Feb 15, 2019

Toflar commented Feb 8, 2018 •

edited

Loading

dunglas Feb 12, 2018 •

edited

Loading

dunglas Feb 12, 2018 •

edited

Loading

dunglas Mar 9, 2018 •

edited

Loading

Simperfit left a comment •

edited

Loading