-
Notifications
You must be signed in to change notification settings - Fork 84
Have a method to add tags to the TagSubscriber directly #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,18 +11,25 @@ | |
|
||
namespace FOS\HttpCacheBundle\Command; | ||
|
||
use FOS\HttpCacheBundle\CacheManager; | ||
use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand; | ||
use Symfony\Component\Console\Input\InputInterface; | ||
use Symfony\Component\Console\Output\OutputInterface; | ||
use Symfony\Component\Console\Input\InputArgument; | ||
use FOS\HttpCacheBundle\CacheManager; | ||
use FOS\HttpCache\Handler\TagHandler; | ||
|
||
/** | ||
* A command to trigger cache invalidation by tag from the command line. | ||
* | ||
* @author David Buchmann <[email protected]> | ||
*/ | ||
class InvalidateTagCommand extends BaseInvalidateCommand | ||
class InvalidateTagCommand extends ContainerAwareCommand | ||
{ | ||
/** | ||
* @var TagHandler | ||
*/ | ||
private $tagHandler; | ||
|
||
/** | ||
* @var string | ||
*/ | ||
|
@@ -32,13 +39,24 @@ class InvalidateTagCommand extends BaseInvalidateCommand | |
* If no cache manager is specified explicitly, fos_http_cache.cache_manager | ||
* is automatically loaded. | ||
* | ||
* @param CacheManager|null $cacheManager The cache manager to talk to. | ||
* @param string $commandName Name of this command, in case you want to reuse it. | ||
* Passing CacheManager as argument is deprecated and will be restricted to TagHandler in 2.0. | ||
* | ||
* @param TagHandler|CacheManager|null $tagHandler The tag handler to talk to. | ||
* @param string $commandName Name of this command, in case you want to reuse it. | ||
*/ | ||
public function __construct(CacheManager $cacheManager = null, $commandName = 'fos:httpcache:invalidate:tag') | ||
public function __construct($tagHandler = null, $commandName = 'fos:httpcache:invalidate:tag') | ||
{ | ||
if (!($tagHandler instanceof TagHandler || $tagHandler instanceof CacheManager || null === $tagHandler)) { | ||
throw new \InvalidArgumentException( | ||
sprintf( | ||
'Expected instance of TagHandler, CacheManager or null, but got %s', | ||
get_class($tagHandler) | ||
) | ||
); | ||
} | ||
$this->commandName = $commandName; | ||
parent::__construct($cacheManager); | ||
$this->tagHandler = $tagHandler; | ||
parent::__construct(); | ||
} | ||
|
||
/** | ||
|
@@ -72,6 +90,18 @@ protected function execute(InputInterface $input, OutputInterface $output) | |
{ | ||
$tags = $input->getArgument('tags'); | ||
|
||
$this->getCacheManager()->invalidateTags($tags); | ||
$this->getTagManager()->invalidateTags($tags); | ||
} | ||
|
||
/** | ||
* @return TagHandler|CacheManager | ||
*/ | ||
protected function getTagManager() | ||
{ | ||
if (!$this->tagHandler) { | ||
$this->tagHandler = $this->getContainer()->get('fos_http_cache.handler.tag_handler'); | ||
} | ||
|
||
return $this->tagHandler; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,12 @@ | |
|
||
namespace FOS\HttpCacheBundle\EventListener; | ||
|
||
use FOS\HttpCacheBundle\CacheManager; | ||
use FOS\HttpCacheBundle\Handler\TagHandler; | ||
use FOS\HttpCacheBundle\Configuration\Tag; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpKernel\Event\FilterResponseEvent; | ||
use Symfony\Component\HttpKernel\HttpKernelInterface; | ||
use Symfony\Component\HttpKernel\KernelEvents; | ||
use Symfony\Component\ExpressionLanguage\ExpressionLanguage; | ||
|
||
|
@@ -27,9 +28,9 @@ | |
class TagSubscriber extends AbstractRuleSubscriber implements EventSubscriberInterface | ||
{ | ||
/** | ||
* @var CacheManager | ||
* @var TagHandler | ||
*/ | ||
private $cacheManager; | ||
private $tagHandler; | ||
|
||
/** | ||
* @var ExpressionLanguage | ||
|
@@ -39,14 +40,14 @@ class TagSubscriber extends AbstractRuleSubscriber implements EventSubscriberInt | |
/** | ||
* Constructor | ||
* | ||
* @param CacheManager $cacheManager | ||
* @param TagHandler $tagHandler | ||
* @param ExpressionLanguage|null $expressionLanguage | ||
*/ | ||
public function __construct( | ||
CacheManager $cacheManager, | ||
TagHandler $tagHandler, | ||
ExpressionLanguage $expressionLanguage = null | ||
) { | ||
$this->cacheManager = $cacheManager; | ||
$this->tagHandler = $tagHandler; | ||
$this->expressionLanguage = $expressionLanguage; | ||
} | ||
|
||
|
@@ -65,32 +66,28 @@ public function onKernelResponse(FilterResponseEvent $event) | |
$response = $event->getResponse(); | ||
|
||
$tags = array(); | ||
|
||
// Only set cache tags or invalidate them if response is successful | ||
if ($response->isSuccessful()) { | ||
$tags = $this->getAnnotationTags($request); | ||
} | ||
|
||
$configuredTags = $this->matchRule($request, $response); | ||
if ($configuredTags) { | ||
foreach ($configuredTags['tags'] as $tag) { | ||
$tags[] = $tag; | ||
} | ||
$tags = array_merge($tags, $configuredTags['tags']); | ||
foreach ($configuredTags['expressions'] as $expression) { | ||
$tags[] = $this->evaluateTag($expression, $request); | ||
} | ||
} | ||
|
||
if (!count($tags)) { | ||
return; | ||
} | ||
|
||
if ($request->isMethodSafe()) { | ||
// For safe requests (GET and HEAD), set cache tags on response | ||
$this->cacheManager->tagResponse($response, $tags); | ||
} else { | ||
$this->tagHandler->addTags($tags); | ||
if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) { | ||
// For safe requests (GET and HEAD), set cache tags on response | ||
$this->tagHandler->tagResponse($response); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i ended up doing this logic. otherwise tags from subrequests get lost. is this sane? for ESI requests, everything is a separate master request and its own cache lifecycle, so having different tags makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure about ESI requests being master requests? I thought they were sub requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pretty much so, yes: https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L492 - this is also the code path reached when esi is done (its only considered a sub request inside the HttpCache but then passed as a new master request to HttpKernel - which seems to me the expected and correct behaviour) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stof does this make sense to you or am i missing something? it seems to me we only want to tag on the response to master request. the current implementation introduces an inconsistency in that tags from configuration / annotation are preserved from subrequest controllers, while cache control of subrequests is lost. but i think that is ok, as such information set on a response object is lost in twig subrequests in general. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ESI requests are indeed master requests (they are subrequests inside Varnish/HttpCache, but as far as AppKernel is concerned, it is an incoming request) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and when using ESI, each master request has its own cache, so you will want to tag the ESI requests on themselves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks @stof for confirming. my other question was whether 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. |
||
} elseif (count($tags)) { | ||
// For non-safe methods, invalidate the tags | ||
$this->cacheManager->invalidateTags($tags); | ||
$this->tagHandler->invalidateTags($tags); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit of a weird interaction with the tag handler:
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is unfortunate. It’s due to the As for resetting: is our current There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd say lets wait for a concrete use case and for now keep it with this flaw. |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
<?php | ||
|
||
namespace FOS\HttpCacheBundle\Handler; | ||
|
||
use FOS\HttpCache\Handler\TagHandler as BaseTagHandler; | ||
use Symfony\Component\HttpFoundation\Response; | ||
|
||
class TagHandler extends BaseTagHandler | ||
{ | ||
/** | ||
* Tag response with the previously added tags. | ||
* | ||
* @param Response $response | ||
* @param bool $replace Whether to replace the current tags on the | ||
* response. If false, parses the header to merge | ||
* tags. | ||
* | ||
* @return $this | ||
*/ | ||
public function tagResponse(Response $response, $replace = false) | ||
{ | ||
if (!$replace && $response->headers->has($this->getTagsHeaderName())) { | ||
$header = $response->headers->get($this->getTagsHeaderName()); | ||
if ('' !== $header) { | ||
$this->addTags(explode(',', $response->headers->get($this->getTagsHeaderName()))); | ||
} | ||
} | ||
|
||
if ($this->hasTags()) { | ||
$response->headers->set($this->getTagsHeaderName(), $this->getTagsHeaderValue()); | ||
} | ||
|
||
return $this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
The Tag Handler | ||
=============== | ||
|
||
Service to work with :doc:`Cache Tagging <../features/tagging>`. You can add tags to the | ||
handler and generate invalidation requests that are queued in the invalidator. | ||
|
||
A response listener checks the ``TagHandler`` to detect tags that need to be | ||
set on a response. As with other invalidation operations, invalidation requests | ||
are flushed to the caching proxy :ref:`after the response has been sent <flushing>`. | ||
|
||
.. _tag_handler_addtags: | ||
|
||
``addTags()`` | ||
------------- | ||
|
||
Add tags to be sent with the response:: | ||
|
||
$tagHandler->addTags(array('some-tag', 'other-tag')); | ||
|
||
This method can be called regardless of whether the response object already | ||
exists or not. | ||
|
||
``invalidateTags()`` | ||
-------------------- | ||
|
||
Invalidate cache tags:: | ||
|
||
$tagHandler->invalidateTags(array('some-tag', 'other-tag')); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ symfony | |
github | ||
subdomains | ||
yaml | ||
invalidator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.