Add abstracted tag handling #237

andrerom · 2015-08-09T23:47:15Z

Aims to allow support for tagging also on Symfony HTTP Cache and
potentially other caches by limiting the feature to focus on only
tags and not random headers.

Closes #234

explain changes in CHANGELOG.md
Update documentation

dbu · 2015-08-10T06:18:04Z

README.md

i suggest:

... and Symfony HttpCache.

dbu · 2015-08-10T06:31:58Z

@ddeboer can you please also give your input here? there are some architectural decisions to take, would appreciate your opinion.

andrerom · 2015-08-10T21:21:30Z

Did an attempt to move tag header name, value, and escaping down to cache invalidations. Looks a bit better imo.

dbu · 2015-08-15T15:35:42Z

README.md

i think we wrote HttpCache elsewhere (the php class is written in camelcase)

dbu · 2015-08-15T15:40:50Z

at this point, the only thing the TagHandler is doing is collecting the tags for tagging a response. that is really not much. i think we should either let TagHandler talk to the proxy client directly or get rid of the handler and move all to CacheInvalidator. i tend to keep TagHandler because its a whole topic of its own and not only about invalidation. I wonder if the TagInterface should be outside the Invalidation namespace, because it is not only about invalidating tags but also about tagging responses.

andrerom · 2015-08-17T12:50:34Z

I wonder if the TagInterface should be outside the Invalidation namespace, because it is not only about invalidating tags but also about tagging responses.

ok, so then I'll skip the header related parts added to CacheInvalidator as well right? (given we then instead inject the tag "something in lack of calling it invalidator anymore" instance)

dbu · 2015-08-17T13:43:08Z

sorry, still trying to get some input from @ddeboer what he thinks about this one. can we wait another day or two? if he does not respond by then we just move forward with what we think is cleanest.

andrerom · 2015-08-17T14:56:42Z

ok, no problem. Probably vacation :)

ddeboer · 2015-08-17T21:18:04Z

That's right, I'm on holidays. Back in civilisation with proper internet connection in a week. :)

dbu · 2015-08-18T06:47:34Z

@andrerom sorry to keep you waiting, but i would prefer to wait for the other david to be back before we take a decision on how to do this one. its a rather important architectural decision.

andrerom · 2015-08-18T14:22:41Z

No problem, opened a possible followup issue to discuss in the mean time (#241).

andrerom · 2015-08-31T22:24:25Z

@ddeboer any views on this topic?

ilukac · 2015-09-15T12:15:03Z

This looks promising. With this feature we could better optimise invalidation...

ddeboer · 2015-09-30T17:02:46Z

src/ProxyClient/AbstractProxyClient.php

Why not private?

But this, and maybe some other parts will be needed for HttpCache as well

Good point.

ddeboer · 2015-09-30T19:18:47Z

Very interesting! I agree with the general approach: we’ve been following Varnish terminology in differentiating purge, refresh and ban, but nothing is stopping is from introducing a separate tags type.

Did an attempt to move tag header name, value, and escaping down to cache invalidations. Looks a bit better imo.

@andrerom Can you elaborate a bit on why you did this? Especially since @dbu and I decided earlier to move tag-specific logic out of the CacheInvalidator and into the TagHandler.

andrerom · 2015-09-30T21:42:59Z

@andrerom Can you elaborate a bit on why you did this? Especially since @dbu and I decided earlier to move tag-specific logic out of the CacheInvalidator and into the TagHandler

I'll need to read up on the discussions above where @dbu and me hit some issues with first approach, but think it was among other things because some of the escaping was clearly varnish specific.

ddeboer · 2015-10-01T06:46:31Z

Right, in the discussion (on outdated diffs) I read:

i think we should move this out of here and the CacheInvalidator. if a header is needed, its up to the client to know that, its an implementation detail specific to the caching proxy.

@dbu Did you mean here to move the tag header logic into CacheInvalidator? If we’re saying it’s client-specific, it should go into the proxy client classes (Varnish/Nginx/Symfony/AbstractProxyClient). After all, we use a header for tagging in Varnish, but we won’t be using one in Symfony HttpCache (if I understand #234 correctly).

In the current solution, getTagsHeaderValue (e.g.) is duplicated in CacheInvalidator and TagHandler.

dbu · 2015-11-18T11:34:31Z

doc/response-tagging.rst

s/service/helper/ ?

andrerom · 2015-11-18T12:20:38Z

@dbu second pass

dbu · 2015-11-18T12:23:33Z

doc/response-tagging.rst

i think the first line can simply be removed, it does not say anything more than the second line.

dbu · 2015-11-18T12:26:37Z

cool! can you adress these last two details and squash commits? then we can merge this.

andrerom · 2015-11-18T12:34:49Z

done

Aims to allow support for tagging also on Symfomy HTTP Cache and potentially other caches by limiting the feature to focus on only tags and not any random header. Closes FriendsOfSymfony#234

andrerom · 2015-11-18T13:14:01Z

~~Still failing on doc, as I don't know anything about rst, how is it supposed to work?~~~

fixed in bc36103

dbu · 2015-11-18T14:34:59Z

doc/proxy-clients.rst

you have a - too much. the line should start with ..

Thanks, copy past fail, hopefully fixed in bc36103

Add abstracted tag handling

dbu · 2015-11-18T16:26:25Z

great job, thanks a lot for your patience!

andrerom · 2015-11-18T16:49:33Z

@dbu Likewise :)

ddeboer · 2015-11-18T19:34:34Z

Thanks @andrerom and @dbu! 👍

andrerom mentioned this pull request Aug 9, 2015

Support Tags with Symfony Http cache #234

Closed

dbu reviewed Aug 10, 2015
View reviewed changes

README.md Outdated

Copy link

Contributor

dbu Aug 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest:

... and Symfony HttpCache.

andrerom force-pushed the tags branch from 1d567d8 to 5783c99 Compare August 10, 2015 09:14

dbu reviewed Aug 15, 2015
View reviewed changes

dbu added this to the 2.0 milestone Aug 15, 2015

dbu mentioned this pull request Aug 15, 2015

Ban subscriber for symfony HttpCache #148

Closed

andrerom mentioned this pull request Aug 18, 2015

Generic graceful tag expiry (aka not using BAN) #241

Closed

dbu mentioned this pull request Aug 21, 2015

update to use 2.0 of FOSHttpCache FriendsOfSymfony/FOSHttpCacheBundle#235

Merged

2 tasks

andrerom mentioned this pull request Sep 30, 2015

Provide Symfony HttpCache as trait #233

Merged

2 tasks

ddeboer reviewed Sep 30, 2015
View reviewed changes

dbu reviewed Nov 18, 2015
View reviewed changes

doc/response-tagging.rst Outdated

Copy link

Contributor

dbu Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/service/helper/ ?

andrerom force-pushed the tags branch from 4a83ade to 6aa656d Compare November 18, 2015 12:19

dbu reviewed Nov 18, 2015
View reviewed changes

doc/response-tagging.rst Outdated

Copy link

Contributor

dbu Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the first line can simply be removed, it does not say anything more than the second line.

andrerom force-pushed the tags branch from 6aa656d to 4c11919 Compare November 18, 2015 12:34

Add abstracted tag handling

e412091

Aims to allow support for tagging also on Symfomy HTTP Cache and potentially other caches by limiting the feature to focus on only tags and not any random header. Closes FriendsOfSymfony#234

andrerom force-pushed the tags branch from 4c11919 to 4ed2a70 Compare November 18, 2015 12:51

dbu reviewed Nov 18, 2015
View reviewed changes

Change doc for abstract tag changes

bc36103

andrerom force-pushed the tags branch from 4ed2a70 to bc36103 Compare November 18, 2015 16:14

dbu added a commit that referenced this pull request Nov 18, 2015

Merge pull request #237 from andrerom/tags

5a3d88a

Add abstracted tag handling

dbu merged commit 5a3d88a into FriendsOfSymfony:master Nov 18, 2015

dbu mentioned this pull request Nov 18, 2015

support tag invalidation with Symfony HttpCache #251

Closed

andrerom deleted the tags branch November 18, 2015 16:49

dbu mentioned this pull request Dec 10, 2015

Call to undefined method ...CacheManager::addSubscriber() FriendsOfSymfony/FOSHttpCacheBundle#260

Closed

Add abstracted tag handling #237

Add abstracted tag handling #237

Uh oh!

Conversation

andrerom commented Aug 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 10, 2015

Uh oh!

andrerom commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 15, 2015

Uh oh!

andrerom commented Aug 17, 2015

Uh oh!

dbu commented Aug 17, 2015 via email

Uh oh!

andrerom commented Aug 17, 2015

Uh oh!

ddeboer commented Aug 17, 2015

Uh oh!

dbu commented Aug 18, 2015

Uh oh!

andrerom commented Aug 18, 2015

Uh oh!

andrerom commented Aug 31, 2015

Uh oh!

ilukac commented Sep 15, 2015

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!

ddeboer commented Sep 30, 2015

Uh oh!

andrerom commented Sep 30, 2015

Uh oh!

ddeboer commented Oct 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrerom commented Nov 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 18, 2015

Uh oh!

andrerom commented Nov 18, 2015

Uh oh!

andrerom commented Nov 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Nov 18, 2015

Uh oh!

andrerom commented Nov 18, 2015

Uh oh!

ddeboer commented Nov 18, 2015

Uh oh!

Uh oh!