Skip to content

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

Merged
merged 1 commit into from
May 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ Changelog
1.3.0
-----

* **2015-05-08** Configured/annotated cache tags on subrequests
(twig `render(controller())`) are no longer ignored. Additionally, it is now
possible to add tags from code before the response object has been created,
by using the TagHandler.
If you defined custom services for the `InvalidateTagCommand`, you should
now inject the TagHandler instead of the CacheManager.

**deprecated** `CacheManager::tagResponse` in favor of `TagHandler::addTags`
* **2015-05-08** Added configuration option for custom proxy client (#208)

1.2.0
Expand Down
2 changes: 2 additions & 0 deletions CacheManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ public function setGenerateUrlType($generateUrlType)
* response
*
* @return $this
*
* @deprecated Add tags with TagHandler::addTags and then use TagHandler::tagResponse
Copy link
Contributor Author

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.

*/
public function tagResponse(Response $response, array $tags, $replace = false)
{
Expand Down
44 changes: 37 additions & 7 deletions Command/InvalidateTagCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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();
}

/**
Expand Down Expand Up @@ -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;
}
}
1 change: 1 addition & 0 deletions DependencyInjection/FOSHttpCacheExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter($this->getAlias().'.compiler_pass.tag_annotations', $config['tags']['enabled']);
if ($config['tags']['enabled']) {
// true or auto
$container->setParameter($this->getAlias().'.tag_handler.header', $config['tags']['header']);
$loader->load('tag_listener.xml');
if (!empty($config['tags']['rules'])) {
$this->loadTagRules($container, $config['tags']['rules']);
Expand Down
33 changes: 15 additions & 18 deletions EventListener/TagSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -27,9 +28,9 @@
class TagSubscriber extends AbstractRuleSubscriber implements EventSubscriberInterface
{
/**
* @var CacheManager
* @var TagHandler
*/
private $cacheManager;
private $tagHandler;

/**
* @var ExpressionLanguage
Expand All @@ -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;
}

Expand All @@ -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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

}
}

Expand Down
35 changes: 35 additions & 0 deletions Handler/TagHandler.php
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;
}
}
2 changes: 1 addition & 1 deletion Resources/config/cache_manager.xml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
</service>

<service id="fos_http_cache.command.invalidate_tag" class="%fos_http_cache.command.invalidate_tag.class%">
<argument type="service" id="fos_http_cache.cache_manager" />
<argument type="service" id="fos_http_cache.handler.tag_handler" />
<tag name="console.command"/>
</service>

Expand Down
7 changes: 6 additions & 1 deletion Resources/config/tag_listener.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@
</parameters>

<services>
<service id="fos_http_cache.handler.tag_handler" class="FOS\HttpCacheBundle\Handler\TagHandler">
<argument type="service" id="fos_http_cache.cache_manager"/>
<argument>%fos_http_cache.tag_handler.header%</argument>
</service>

<service id="fos_http_cache.event_listener.tag"
class="%fos_http_cache.event_listener.tag.class%">
<argument type="service" id="fos_http_cache.cache_manager" />
<argument type="service" id="fos_http_cache.handler.tag_handler" />
<tag name="kernel.event_subscriber" />
</service>
</services>
Expand Down
4 changes: 2 additions & 2 deletions Resources/doc/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
# built documents.
#
# The short X.Y version.
version = '1.0.0'
version = '1.3.0'
# The full version, including alpha/beta/rc tags.
release = '1.0.0'
release = '1.3.0'

# The language for content autogenerated by Sphinx. Refer to documentation
# for a list of supported languages.
Expand Down
25 changes: 15 additions & 10 deletions Resources/doc/features/tagging.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,25 +35,30 @@ Setting and Invalidating Tags
You can tag responses in three ways: with the cache manager, configuration and
annotations.

Cache Manager
~~~~~~~~~~~~~
Tagging from code
~~~~~~~~~~~~~~~~~

Use ``tagResponse($response, $tags)`` to set tags on a response::
Inject the ``TagHandler`` (service ``fos_http_cache.handler.tag_handler``) and
use ``addTags($tags)`` to add tags that will be set on the response::

use Symfony\Component\HttpFoundation\Response;
use FOS\HttpCacheBundle\Handler\TagHandler;

class NewsController
{
/**
* @var TagHandler
*/
private $tagHandler;

public function articleAction($id)
{
$response = new Response('Some news article');
$this->cacheManager->tagResponse($response, array('news', 'news-' . $id));
$this->tagHandler->addTags(array('news', 'news-' . $id));

return $response;
// ...
}
}

Then call ``invalidateTags($tags)`` to invalidate the tags you set::
To invalidate tags, call ``TagHandler::invalidateTags($tags)``::

class NewsController
{
Expand All @@ -63,13 +68,13 @@ Then call ``invalidateTags($tags)`` to invalidate the tags you set::
{
// ...

$this->cacheManager->invalidateTags(array('news-' . $id))->flush();
$this->tagHandler->invalidateTags(array('news-' . $id));

// ...
}
}

See the :ref:`Cache Manager reference <cache_manager_tags>` for full details.
See the :ref:`Tag Handler reference <tag_handler_addtags>` for full details.

Configuration
~~~~~~~~~~~~~
Expand Down
1 change: 1 addition & 0 deletions Resources/doc/reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ annotations and public methods.
reference/configuration
reference/annotations
reference/cache-manager
reference/tag-handler
reference/glossary
23 changes: 5 additions & 18 deletions Resources/doc/reference/cache-manager.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,25 +82,12 @@ Refresh a Route::

.. _cache_manager_tags:

``tagResponse()``
-----------------
``tagResponse()``, ``invalidateTags()``
---------------------------------------

Use the Cache Manager to tag responses::

// $response is a \Symfony\Component\HttpFoundation\Response object
$cacheManager->tagResponse($response, array('some-tag', 'other-tag'));

The tags are appended to already existing tags, unless you set the ``$replace``
option to true::

$cacheManager->tagResponse($response, array('different'), true);

``invalidateTags()``
--------------------

Invalidate cache tags::

$cacheManager->invalidateTags(array('some-tag', 'other-tag'));
.. versionadded:: 1.3
Since version 1.3, use the :doc:`TagHandler <tag-handler>` instead of the
CacheManager for working with tags.

.. _flushing:

Expand Down
28 changes: 28 additions & 0 deletions Resources/doc/reference/tag-handler.rst
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'));
1 change: 1 addition & 0 deletions Resources/doc/spelling_word_list.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ symfony
github
subdomains
yaml
invalidator
Loading