Symfony taggable cache #376

dbu · 2017-09-26T06:02:41Z

replace #373, fix #286
squashed all commits and did some tweaks to the documentation.

TODO

Once Symfony 3.4 is out: remove the @beta in composer.json

=> Bundle support: FriendsOfSymfony/FOSHttpCacheBundle#403

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

Are you sure I should not extract an extra library? Maybe we should wait for Javier's response there?

dbu · 2017-09-27T06:18:04Z

@Toflar okay, pushed my last changes. mainly i realized that the cache directory parameter only makes sense when not passing the cache and lock instance to the store, so i moved it into the options as well. i also refactored the OptionsResolver to use closures for the default cache and lock implementation to avoid instantiating them when not needed. this looks ready to me.

do you want to take the store and its test into its own repo and create a readme from the doc section on the store?

Toflar · 2017-09-27T07:49:00Z

Perfect, thank you! Yes, I'll take care extracting it into its own library and then I'll update my original PR against FOSHttpCache and see if I can build some kind of bridge. Would that be fine with you?

dbu · 2017-09-27T08:50:29Z

great! i did some tweaks in this branch. can you do the update from this branch? or i can just edit it myself, its mostly deleting the things that moved to your repo anyways :-)

i would keep the store with the tag handling support - its basically free with PSR-6. then there should be no need to bridge things, we can just tell in the doc to require your repo when you want cache tagging in symfony HttpCache, and everything should just work the same as it works now in this pull request (apart from adjusting the namespace of the store)

Toflar · 2017-09-27T09:00:09Z

Yeah I'll let you know when it's ready :)

Toflar · 2017-09-27T13:26:45Z

Here we go: https://github.com/Toflar/psr6-symfony-http-cache-store 😄

Registered over at packagist already too, so you could update the PR requiring dev-master (I guess I'll leave it like that until Symfony 3.4.0 stable is published, wdyt?).

dbu · 2017-09-27T15:56:30Z

thank you! i update the PR, but tests fail because the Psr6Store is final. this feels stupid, seems to be something with how we do the mocking of the store in the unit test.

lets gather some feedback on the store and then release it asap. to merge this, i'd want at least an 1.0.0-alpha1 - i think thats reasonable to do soon.

Toflar · 2017-09-27T15:56:31Z

doc/symfony-cache-configuration.rst

+    ) {
+        $store = new Psr6Store([
+            'cache_directory' => $kernel->getCacheDir(),
+            'cache_tags_header' => 'X-Cache-Tags',


You noticed. Well done!

Toflar · 2017-09-27T15:58:22Z

thank you! i update the PR, but tests fail because the Psr6Store is final.

Yeah I think it should not be extended but I can remove it. I just don't want to provide BC for child classes...

dbu · 2017-09-28T06:35:01Z

having it final seems correct to me. but how to test? i wonder if it could make sense to define an interface for this that extends the symfony StoreInterface with the prune and invalidateTags methods. that would define clearly what is added, and we could mock the interface...

Toflar · 2017-09-28T07:40:20Z

Makes sense to me, I also mocked the Lock which is final too and I could mock it thanks to the LockInterface so to me that seems like good practice. Can you work with Toflar/psr6-symfony-http-cache-store@ebf22b7?

dbu · 2017-09-28T08:05:53Z

that looks great! i added some comments about wording and naming, but the structure is exactly what i had in mind.

dbu

Great. I added symfony/lock and /cache to the dependencies for now - the @dev is not inherited from bundles, so we can't install this with default stability.

that means we will have to wait until symfony 3.4 is released, then release your library and then i can update here and we can merge and release...

dbu · 2017-09-27T15:53:04Z

composer.json

@@ -36,7 +36,8 @@
        "php-http/mock-client": "^0.3.2",
        "symfony/process": "^2.3||^3.0",
        "symfony/http-kernel": "^2.3||^3.0",
-        "phpunit/phpunit": "^5.7 || ^6.0"
+        "phpunit/phpunit": "^5.7 || ^6.0",
+        "toflar/psr6-symfony-http-cache-store": "dev-master"


we should create an alpha release before merging this

dbu · 2017-09-28T18:04:01Z

composer.json

+        "phpunit/phpunit": "^5.7 || ^6.0",
+        "toflar/psr6-symfony-http-cache-store": "dev-master",
+        "symfony/lock": "^3.4@dev",
+        "symfony/cache": "^3.4@dev"


we need to adjust this once the psr6-symfony-http-cache-store and symfony 3.4 are released.

Toflar · 2017-09-28T18:06:43Z

CHANGELOG.md

+* [Testing] Upgraded phpunit to 5.7 / 6. If you use anything from the
+  `FOS\HttpCache\Test` namespace you need to update your project to use
+  PHPUnit 6 (or 5.7, if you are using PHP 5.6).
+* [Symfony HttpCache] Added a `TaggableStore` for tag based invalidation with 


This needs to be updated.

i deleted the wrong half of this :-( thanks for spotting, fixed

Toflar · 2017-09-28T18:07:21Z

doc/symfony-cache-configuration.rst

+
+To support :doc:`cache tags <response-tagging>`, register the
+``PurgeTagsListener``. The purge listener needs the special
+``Toflar\Psr6HttpCacheStore\Psr6Store`` cache store with tag support.


tagging support

Toflar · 2017-09-28T18:07:52Z

doc/symfony-cache-configuration.rst

+    information on the store.
+
+    To install the store, run
+    ``composer require toflar/psr6-symfony-http-cache-store dev-master``.


I'd remove dev-master here. Once we release it it's obsolete and composer will choose the right version anyway :)

indeed. i thought we might merge this before the store is tagged, but as we have to wait for stable symfony 3.4 anyways we can wait.

andrerom · 2017-10-31T20:30:04Z

src/ProxyClient/Symfony.php

+        $resolver->setDefault('purge_tags_header', PurgeTagsListener::DEFAULT_PURGE_TAGS_HEADER);
+        $resolver->setAllowedTypes('purge_tags_header', 'string');
+        $resolver->setDefault('purge_tags_header_length', 7500);
+        $resolver->setAllowedTypes('purge_tags_header_length', 'int');


we should perhaps align config naming with Varnish one or the other way?

Also code wise easier to read if setDefaults() is used imo: https://github.com/FriendsOfSymfony/FOSHttpCache/pull/332/files#diff-006816ea733cc0564d6a5b345ed47929R160

agreed with setDefaults. for the naming i agree as long as its really the same semantics. we can't change existing names easily, because backwards compatibility is important.

andrerom · 2017-10-31T20:33:03Z

with 3.4 now in beta2 I guess this can be updated to @beta flag and moved forward to merge.

Is there plans for functional tests here?

dbu · 2017-11-03T09:49:15Z

i want to wait with merging until 3.4 is tagged as stable release.

functional tests would indeed be good. i don't have the time to build them - if toflar finds the time to do them would be awesome!

dbu

i rebased on master, hopefully the build is now green.

@Toflar would you have time to address the unifcation of varnish vs symfony proxy client method to invalidate tags avoiding too long http headers?

dbu · 2017-12-04T15:29:13Z

src/ProxyClient/HttpProxyClient.php

+     *
+     * @return array
+     */
+    protected function splitLongHeaderValue($value, $length = 7500, $delimiter = ',')


finally looking over this again and i notice that this method is quite different from what we do to solve the same problem in Varnish::invalidateTags. can we instead put the logic from Varnish::invalidateTags into a HttpProxyClient::generateTagInvalidationChunks and use that for both?

this method is inefficient because the caller first implodes all tags, then splits them again and builds chunks from it

dbu · 2017-12-04T15:32:20Z

hm, the toflar store requires symfony kernel 3.0 or newer. is it time to drop symfony 2 support here? or would it be easy to allow symfony 2 in the store?

Toflar · 2017-12-04T15:55:34Z

@Toflar would you have time to address the unifcation of varnish vs symfony proxy client method to invalidate tags avoiding too long http headers?

I don't think they can be unified. The Varnish proxy requires a very special regex-style invalidation header which makes absolutely no sense for the SF proxy.

hm, the toflar store requires symfony kernel 3.0 or newer. is it time to drop symfony 2 support here? or would it be easy to allow symfony 2 in the store?

It depends on the Lock component, so SF 3.4+ only.

Toflar · 2017-12-04T16:01:04Z

Not sure but we wanted to add it as a suggest, didn't we? I mean, if you don't use the SF HttpCache but Varnish instead, why would you need to install it?

andrerom · 2017-12-04T19:24:29Z

It depends on the Lock component, so SF 3.4+ only.

but technically lock can be installed independently right? same as with cache afaik

andrerom · 2017-12-04T19:25:46Z

@Toflar would you have time to address the unifcation of varnish vs symfony proxy client method to invalidate tags avoiding too long http headers?

I don't think they can be unified. The Varnish proxy requires a very special regex-style invalidation header which makes absolutely no sense for the SF proxy.

@dbu Maybe we can try to do that as followup once/if xkey gets in?

Toflar · 2017-12-04T20:18:34Z

but technically lock can be installed independently right? same as with cache afaik

the question was about the store and this is based on top of both of these components so they are a requirement 😊 but the whole store lib is optional for foscache 😊

dbu · 2017-12-05T07:57:38Z

composer.json

        "symfony/process": "^2.3 || ^3.0 || ^4.0",
        "symfony/http-kernel": "^2.3 || ^3.0 || ^4.0",
-        "phpunit/phpunit": "^5.7 || ^6.0"
+        "symfony/lock": "^3.4 || ^4.0",
+        "symfony/cache": "^3.4 || ^4.0",


@Toflar do we need the lock and cache as direct dependency? guess not, we only had that for stability weakening? the store brings them and its the store that has the dependency, not directly us?

They're not needed. They're dev requirements anyway and I guess it's a leftover from the times where the store implementation was part of this PR and not a separate lib yet :)

Was also probably needed during dev/beta of lock :)

dbu · 2017-12-05T16:58:29Z

@Toflar i refactored the tag invalidation header splitting. can you check if you have any feedback? i think i am now happy with this.

Toflar · 2017-12-05T17:19:33Z

Nice work, I've added a comment about the delimiter but otherwise: 👍

Toflar · 2017-12-06T08:02:43Z

src/ProxyClient/HttpProxyClient.php

    {
-        if (mb_strlen($value) <= $length) {
-            return [$value];
+        if (mb_strlen(implode(',', $escapedTags)) < $this->options['header_length']) {


Either I'm blind or my comment is gone? Hope I do not repeat myself here now but just in case: I mentioned that , is hardcoded now but I might want to use foobar as a delimiter which later affects the length of the header so it should be the second argument of this method :)

you commented on the commit, instead of in the pull request ;-)

thanks for the input, i guess i should indeed pass in the delimiter, it costs us nothing and is future proof.

both xkey and Surrogate-Key (Fastly) works with space separated tags afaik, so a bit outside of this but might be worth to either abstract as stated in this thread or even just align.

dbu · 2017-12-13T16:17:37Z

@andrerom i added whitespace to the things to replace in tags. do you want to refactor this PR to use spaces instead of comma? i don't have time to do it and want to merge it this week...

andrerom · 2017-12-13T23:15:24Z

I'm unsure if I have time for my own PR's, so would not bet on that, we are doing two releases in parallel for end of year, one is major.

dbu · 2017-12-14T07:53:23Z

I'm unsure if I have time for my own PR's, so would not bet on that, we are doing two releases in parallel for end of year, one is major.

sounds like a busy week indeed. i think its not that important that we are consistent with the separator between different caches, as each cache has its own client anyways.

i will revert the whitespace escaping for now. thanks to the failing tests i realized that this would be a potential BC break if you don't wipe the cache after deploying this version - invalidations would then use the new escaping and not hit old tags.

dbu · 2017-12-14T13:03:46Z

@Toflar want to do a last review of this before i merge?

Toflar

It looks all just fine to me, will try to integrate it as soon as possible so in case I'd stumble over anything we may still release a quick hotfix but I don't expect this to happen :)

dbu · 2017-12-14T13:49:42Z

yay, done! thanks a ton!

i will only tag the library next week, hopefully getting the other big contribution with varnish xkey support in. if you spot something until monday, we can just get it into the head. otherwise we can still to a .1 patch release with a fix. its a new feature, so no drama if that happens.

pbowyer · 2017-12-14T13:52:15Z

Thanks everyone!

dbu force-pushed the symfony-taggable-cache branch 3 times, most recently from 238a548 to 2ae81c0 Compare September 27, 2017 06:16

dbu force-pushed the symfony-taggable-cache branch from df19852 to 0290e22 Compare September 27, 2017 15:52

Toflar reviewed Sep 27, 2017

View reviewed changes

dbu force-pushed the symfony-taggable-cache branch from 0290e22 to 5eadc48 Compare September 28, 2017 06:51

dbu force-pushed the symfony-taggable-cache branch 2 times, most recently from 34c2763 to 07eedae Compare September 28, 2017 18:03

dbu commented Sep 28, 2017

View reviewed changes

dbu mentioned this pull request Sep 28, 2017

Symfony taggable cache #373

Closed

Toflar reviewed Sep 28, 2017

View reviewed changes

dbu force-pushed the symfony-taggable-cache branch from 07eedae to 9664698 Compare September 28, 2017 18:11

dbu mentioned this pull request Sep 28, 2017

Symfony HTTP Cache / Tag Invalidation #286

Closed

dbu mentioned this pull request Oct 9, 2017

Fix: UserContextListener set http method wrong #378

Merged

andrerom reviewed Oct 31, 2017

View reviewed changes

dbu force-pushed the symfony-taggable-cache branch 2 times, most recently from b564705 to cd0a2c1 Compare November 6, 2017 07:55

dbu force-pushed the symfony-taggable-cache branch 2 times, most recently from 5bd72d6 to 9fc1319 Compare December 4, 2017 15:23

dbu changed the title ~~WIP Symfony taggable cache~~ Symfony taggable cache Dec 4, 2017

dbu commented Dec 4, 2017

View reviewed changes

dbu commented Dec 5, 2017

View reviewed changes

dbu force-pushed the symfony-taggable-cache branch from 1ff4612 to 4d1627b Compare December 5, 2017 16:57

Toflar reviewed Dec 6, 2017

View reviewed changes

dbu added this to the 2.1.0 milestone Dec 13, 2017

dbu force-pushed the symfony-taggable-cache branch 2 times, most recently from ef5e897 to 1705566 Compare December 13, 2017 16:15

dbu force-pushed the symfony-taggable-cache branch from 3fe903b to 427cb38 Compare December 14, 2017 08:04

Toflar approved these changes Dec 14, 2017

View reviewed changes

TaggableStore and cache tag handling for Symfony HttpCache

b1ce46c

dbu force-pushed the symfony-taggable-cache branch from 4e602a4 to b1ce46c Compare December 14, 2017 13:39

dbu merged commit fb6128b into master Dec 14, 2017

dbu deleted the symfony-taggable-cache branch December 14, 2017 13:48

Symfony taggable cache #376

Symfony taggable cache #376

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!