Have a method to add tags to the TagSubscriber directly #182

dbu · 2014-12-13T16:21:48Z

This is useful to tag responsens when the response object does not yet exist or is not known.

This solves part of #181

@ddeboer do you want me to add a twig extension to tag responses from inside twig templates here? or should we provide that from SymfonyCmfHttpCacheBundle rather? the cmf bundle will extend it to handle doctrine objects in addition to just strings.

TODO

Refactor TagSubscriber to use TagHandler
Adjust documentation to talk about TagHandler
Squash commits

ddeboer · 2014-12-15T07:14:37Z

I think we should add the Twig function here. Do you want to do so here or in a separate PR?

dbu · 2014-12-17T07:14:51Z

i'd rather do a separate PR. not good to keep PRs open longer than needed...

ddeboer · 2014-12-17T07:28:50Z

Resources/doc/reference/cache-manager.rst

+.. tip::
+
+    If you do not yet have the response available, you can use the
+    ``TagListener::addTags($tags)`` method to have your tags added once the


TagSubscriber

stof · 2014-12-17T08:39:16Z

This makes the subscriber stateful for something tied to a given Request, which is a bad idea IMO. It means it is now impossible to handle several requests with your kernel without shutting it down between requests (to drop the container and build a new one).
And the HttpCache will not shutdown the kernel between the different requests it does when using ESI.

stof · 2014-12-17T08:40:34Z

EventListener/TagSubscriber.php

@@ -63,12 +86,16 @@ public function onKernelResponse(FilterResponseEvent $event)
    {
        $request = $event->getRequest();
        $response = $event->getResponse();
-
-        $tags = array();
+        if ($request->isMethodSafe()) {


why having a different logic for safe responses ?

because on unsafe methods, the tags will be invalidated rather than added to the response (unsafe responses are not cached)

when invalidating, the user should directly call the tag invalidation rather than add them here. but we could either accept them here anyways, or throw an error if there are any because its the wrong way to do things.

@ddeboer what do we want to do about this? allow to invalidate tags by adding them to the tagHandler in an unsafe request, or throw an exception in this case and tell that to invalidate the user should explicitly call invalidate?

I prefer the exception, for I suspect it gets confusing to users if we let addTags() do invalidation too. The distinction makes sense: you have only one response during the request/response cycle that you want to set exactly the right set of tags on. While invalidating, however, you can just call invalidateTags() with some tag(s), and later in the cycle call it again with some other tag(s).

ddeboer · 2014-12-17T11:41:43Z

@stof Good point. Do you have any suggestion as to how we could offer this functionality (tagging responses when they’re not yet available) without introducing a stateful service?

dbu · 2014-12-17T16:39:37Z

EventListener/TagSubscriber.php

-        $tags = array();
+        if ($request->isMethodSafe()) {
+            $tags = $this->tags;
+            $this->tags = array();


this was my attempt to avoid adding the tags again on the next request. but the else case should also empty them (or do an error, see above).

could we end up with a mixup on subrequests? at least with ESI, each request is sent before the next is done so this should be enough.

do subrequests inside symfony (twig calling a controller and those) trigger the response event? if they do, are the headers of the responses merged? if not, we already have a problem with the current master.

while i agree that becoming stateful is not elegant, i don't see a better option. would we need a TagStack or something where we push and pop when subrequests are triggered?

@stof do you have a better suggestion how to do this, or how to avoid statefullness alltogether?

dbu · 2014-12-27T14:54:16Z

so, what are we going to do? @stof sorry for prodding, but is resetting the tags good enough? or do you have a better idea what we could do?

stof · 2014-12-31T13:20:20Z

Resetting the tags is wrong, because the controller for the master request is likely to be called before the subrequest, and your logic would then add these headers on the response of the subrequest triggered in the template instead of adding it on the master response.

IMO, a solution could be to associate the tags to the current request when setting them (getting the current request from the RequestStack for Symfony 2.4+ and using a setter for the request on 2.3 thanks to the synchronized services to keep the support for the LTS release). Then, in the kernel.response even, you would get the tags associated to the Request available in the event (and then drop them from the storage because they won't be needed anymore)

ddeboer · 2015-01-18T13:57:16Z

All right, so let’s try to group tags by the response they should be applied to.

dbu · 2015-01-18T17:10:56Z

yeah, i wanted to look into that, but realized it would make a lot more sense to do that in a TagHandler than adding more non-event things into the Tag event listener class and started with FriendsOfSymfony/FOSHttpCache#162 as precondition.

ddeboer · 2015-02-13T13:50:12Z

I guess this can be finished now, since FriendsOfSymfony/FOSHttpCache#162 and FriendsOfSymfony/FOSHttpCache#171 have been merged.

dbu · 2015-02-20T15:39:37Z

Resources/doc/features/tagging.rst


-Use ``tagResponse($response, $tags)`` to set tags on a response::
+Inject the ``CacheManager`` (service ``fos_http_cache.cache_manager``) and


TODO: adjust the doc to the new architecture

Todo still open?

dbu · 2015-02-20T18:15:55Z

so i propose that we only tag the response to the master request as we otherwise lose the tags of subrequests. note that this will be a behaviour change for existing features: the tag annotation in the subrequest gets lost in the 1.2 bundle. i think the new behaviour is correct and consider the other thing a bug.

ddeboer · 2015-02-20T19:37:28Z

Resources/doc/features/tagging.rst

-Then call ``invalidateTags($tags)`` to invalidate the tags you set::
+Or inject the ``TagSubscriber`` (service ``fos_http_cache.event_listener.tag``)
+and call ``addTags($tags)`` to set tags that will be added to the response once
+it exists::


“Or if you have no access to a Response object inject…”

ddeboer · 2015-02-20T19:41:28Z

so i propose that we only tag the response to the master request

👍

dbu · 2015-03-13T11:15:35Z

EventListener/TagSubscriber.php

@@ -86,8 +113,13 @@ public function onKernelResponse(FilterResponseEvent $event)
        }

        if ($request->isMethodSafe()) {
-            // For safe requests (GET and HEAD), set cache tags on response
-            $this->cacheManager->tagResponse($response, $tags);
+            if ($event->isMasterRequest()) {


argh, this is only available in symfony 2.4+, but not 2.3. i will fix that line to work with 2.3

dbu · 2015-03-13T11:16:12Z

@stof do you agree that we tag only on response to master request, not response to subrequests (you talked about using the request stack and associate tags with requests). it seems to me to make no sense to ever tag a response to a subrequest, as the headers of those responses are simply discarded.

dbu · 2015-03-13T13:17:06Z

CacheManager.php

@@ -67,6 +67,8 @@ public function setGenerateUrlType($generateUrlType)
     *                           response
     *
     * @return $this
+     *
+     * @deprecated Add tags with TagHandler::addTags and then use TagHandler::tagResponse


i don't go to the TagHandler here. so people who mix up new and old things might get into troubles. but otherwise we complicate things quite a bit.

dbu · 2015-03-13T13:20:41Z

Command/InvalidateTagCommand.php

     */
-    public function __construct(CacheManager $cacheManager = null, $commandName = 'fos:httpcache:invalidate:tag')
+    public function __construct(TagHandler $tagHandler = null, $commandName = 'fos:httpcache:invalidate:tag')


if people defined their own service for the InvalidateTagCommand or extended the command, this is a BC break. do we care about it or can we assume this an edge case? if we care, what are alternatives that make sense?

we can keep the BC method of CacheManager of course, but people using the new TagHandler then will be just as confused.

The BC way would be to allow both CacheManager and TagHandler as constructor arguments, check the type and throw an exception if it is neither of both.

good idea, adding that in a BC way

dbu · 2015-03-13T13:27:06Z

EventListener/TagSubscriber.php

            // For non-safe methods, invalidate the tags
-            $this->cacheManager->invalidateTags($tags);
+            $this->tagHandler->invalidateTags($tags);


this is a bit of a weird interaction with the tag handler:

for tagging the response, the only way to go is to do $tagHandler->addTags($tags); $tagHandler->tagResponse($response);

for invalidating, the only way to go is to $tagHandler->invalidateTags($tags);

so tagging a response is only possible from the values in the handler, invalidating is only possible with outside tags.

also, as we have no way to reset the tags on tagResponse, we will keep the tags in the handler and if there is ever more than one response sent, tags will accumulate. should we add a reset method to the tagHandler and trigger that reset in tagResponse?

Yes, this is unfortunate. It’s due to the TagHandler having two (albeit closely related) responsibilities instead of one: both tagging safe responses and invalidating unsafe ones. I don’t see how we can clear this up without splitting the class into two, which may be overkill.

As for resetting: is our current TagHandler suitable for tagging multiple responses? Magically resetting between tag actions may also be confusing if a user wants to tag two responses identically.

i'd say lets wait for a concrete use case and for now keep it with this flaw.

dbu · 2015-03-13T13:29:22Z

@ddeboer had another go on this. i think things are shaping up, i added some comments on the code. glad for some feedback / opinion so that we can wrap this up.

dbu · 2015-04-14T18:36:46Z

@ddeboer ping

dbu · 2015-04-14T19:45:26Z

only open point i can see now is whether we want to have something more elegant than if ($this->getTagsHeaderValue()) {

when we clear that, i can squash the commits and we can merge.

dbu · 2015-05-08T08:29:19Z

composer.json

@@ -22,7 +22,7 @@
    ],
    "require": {
        "php": ">=5.3.3",
-        "friendsofsymfony/http-cache": "~1.2",
+        "friendsofsymfony/http-cache": "~1.3",


Needs FriendsOfSymfony/FOSHttpCache#190 so tag 1.3.2 and change this to ~1.3,>=1.3.2

dbu · 2015-05-08T09:29:56Z

depend on fos httpcache 1.3.2 or newer, squashed commits. if travis is happy, this is now ready to merge.

ddeboer · 2015-05-08T10:49:09Z

Small doc issue in the build.

… request does not exist yet

dbu · 2015-05-08T11:58:30Z

now its green.

Have a method to add tags to the TagSubscriber directly

ddeboer · 2015-05-09T06:40:39Z

Great, thanks!

lsmith77 added the in progress label Dec 13, 2014

ddeboer reviewed Dec 17, 2014
View reviewed changes

dbu mentioned this pull request Dec 17, 2014

integrate FOSHttpCache with the CMF symfony-cmf/symfony-cmf#211

Open

4 tasks

stof reviewed Dec 17, 2014
View reviewed changes

dbu reviewed Dec 17, 2014
View reviewed changes

dbu mentioned this pull request Dec 17, 2014

Object invalidation #184

Closed

dbu force-pushed the tag-response-subscriber branch from 313bf04 to 12e4467 Compare December 27, 2014 14:53

dbu mentioned this pull request Jan 5, 2015

[WIP/RFC] Refactored CacheInvalidator into descreet units FriendsOfSymfony/FOSHttpCache#153

Closed

dbu mentioned this pull request Jan 18, 2015

Separating the tag handler into its own class FriendsOfSymfony/FOSHttpCache#162

Merged

3 tasks

dbu force-pushed the tag-response-subscriber branch from 12e4467 to 41d4c34 Compare February 20, 2015 15:38

dbu reviewed Feb 20, 2015
View reviewed changes

ddeboer reviewed Feb 20, 2015
View reviewed changes

dbu reviewed Mar 13, 2015
View reviewed changes

dbu mentioned this pull request Mar 13, 2015

need a way to access the TagHandler to refactor CacheManager in a BC way FriendsOfSymfony/FOSHttpCache#178

Closed

dbu force-pushed the tag-response-subscriber branch from fb89e32 to 594625f Compare March 13, 2015 13:15

dbu reviewed Mar 13, 2015
View reviewed changes

dbu force-pushed the tag-response-subscriber branch from 594625f to 7df5eb3 Compare March 13, 2015 13:23

dbu reviewed Mar 13, 2015
View reviewed changes

dbu mentioned this pull request Mar 19, 2015

Ban subscriber for symfony HttpCache FriendsOfSymfony/FOSHttpCache#148

Closed

dbu force-pushed the tag-response-subscriber branch from d3c3709 to 7eaffe9 Compare April 14, 2015 19:42

dbu reviewed May 8, 2015
View reviewed changes

dbu force-pushed the tag-response-subscriber branch from 7eaffe9 to 612bcc6 Compare May 8, 2015 09:29

dbu mentioned this pull request May 8, 2015

tagging response while building the content #181

Closed

2 tasks

dbu force-pushed the tag-response-subscriber branch from 612bcc6 to c72bb68 Compare May 8, 2015 09:40

dbu modified the milestone: 1.3 May 8, 2015

dbu force-pushed the tag-response-subscriber branch from c72bb68 to 6853411 Compare May 8, 2015 10:52

Have a method to add tags to the TagSubscriber directly, for when the…

712de49

… request does not exist yet

dbu force-pushed the tag-response-subscriber branch from 6853411 to 712de49 Compare May 8, 2015 11:21

ddeboer added a commit that referenced this pull request May 9, 2015

Merge pull request #182 from FriendsOfSymfony/tag-response-subscriber

a1e22ce

Have a method to add tags to the TagSubscriber directly

ddeboer merged commit a1e22ce into master May 9, 2015

lsmith77 removed the in progress label May 9, 2015

ddeboer deleted the tag-response-subscriber branch May 9, 2015 06:33


		Use ``tagResponse($response, $tags)`` to set tags on a response::
		Inject the ``CacheManager`` (service ``fos_http_cache.cache_manager``) and

Have a method to add tags to the TagSubscriber directly #182

Have a method to add tags to the TagSubscriber directly #182

Uh oh!

Conversation

dbu commented Dec 13, 2014

Uh oh!

ddeboer commented Dec 15, 2014

Uh oh!

dbu commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Dec 17, 2014

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 Dec 17, 2014

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!

dbu commented Dec 27, 2014

Uh oh!

stof commented Dec 31, 2014

Uh oh!

ddeboer commented Jan 18, 2015

Uh oh!

dbu commented Jan 18, 2015

Uh oh!

ddeboer commented Feb 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu Apr 14, 2015 via email

Choose a reason for hiding this comment

Uh oh!

dbu commented Feb 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Feb 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Mar 13, 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!

dbu Apr 14, 2015 via email

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!

dbu commented Mar 13, 2015

Uh oh!

dbu commented Apr 14, 2015

Uh oh!

dbu commented Apr 14, 2015

Uh oh!