Change user hash to use tags to drop ban capable proxy_client requirement

[andrerom]

As discussed on original Fastly PR, one blocker to be able to to do user hash feature on Fastly is current requirement of ProxyClient being BanCapable.

In order to solve that it can be changed to use tags instead which now works across all at least Varnish, Symfony and Fastly.

[dbu]

agreed. i guess that means having some config flag to switch to tagging, to not create a BC break.

i think once we have all this stuff in and released, it is about time to do a new major version to remove the BC things and cleanup...

[andrerom]

i guess that means having some config flag to switch to tagging, to not create a BC break.

most likely yes

i think once we have all this stuff in and released, it is about time to do a new major version to remove the BC things and cleanup...

indeed, a major version taking advantage of Symfony 4+ maybe even

[Toflar]

I would also like to rework the proxy implementation. I feel it's not the right way to have response tagging and purging configured separately because they have to match. While there might be special cases where one does not follow the official FOS docs and is happy they can configure it, 99% of all the users don't need that. And the other 1% can still implement their own proxy solution.
So I'm thinking about having something like public function tagResponse(Response $response) in the interface. Also, I know I've introduced the TagHeaderFormatter but in hindsight I think this is was just overkill. Overall the library is way too complex for what it does 😄

[andrerom]

@dbu Looking back at this, wasn't this fixed in FriendsOfSymfony/FOSHttpCacheBundle@2005da4?

Or is there something still open and relevant around this?

[andrerom]

Besides maybe config exception now being misleading:

throw new InvalidConfigurationException('To enable the user context logout handler, you need to configure a ban capable proxy_client.');

[dbu]

oh, yep seems we fixed it and forgot to close this one. but indeed that exception sounds wrong - do you want to do a pull request to improve the message and close this issue? (you are the author of it anyways ;-) )

[andrerom]

I can, any suggestion on what the error actually is now?

As in, I don't see any checks for TagCaple, so is it implicit by hard coded to varnish/symfony/noop or is there some other logic other places to take into account?

[dbu]

its about whether that logout handler should be enabled. it can be false, then we return immediately. if it is auto we set it to true if we have a custom client (that is a bit iffy, just hoping the custom handle is tag capable) or if we have symfony/varnish/noop, and otherwise (ngnix) set it to false. if the handler is forced to true in the config and we have nginx, we end at the exception state. i think its correct to change the exception text to say TagCapable instead of BanCapable.

[andrerom]

@Toflar Regarding your comment

So I'm thinking about having something like public function tagResponse(Response $response) in the interface. Also, I know I've introduced the TagHeaderFormatter but in hindsight I think this is was just overkill. Overall the library is way too complex for what it does

Would this be the direction you had in mind?

diff --git a/src/ProxyClient/Response/TagResponseCapable.php b/src/ProxyClient/Response/TagResponseCapable.php
new file mode 100644
index 0000000..5296cee
--- /dev/null
+++ b/src/ProxyClient/Response/TagResponseCapable.php
@@ -0,0 +1,33 @@
+<?php
+
+/*
+ * This file is part of the FOSHttpCache package.
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\HttpCache\ProxyClient\Response;
+
+use FOS\HttpCache\ProxyClient\ProxyClient;
+use Psr\Http\Message\ResponseInterface;
+
+/**
+ * An HTTP cache that supports formatting tag response.
+ */
+interface TagResponseCapable extends ProxyClient
+{
+
+    /**
+     * Tag a response using provided tags.
+     *
+     * @param ResponseInterface  $response Response we'd like to add tags to
+     * @param array              $tags     List of tags to add to response
+     * @param bool               $replace  Replace exisitng tags on response, or append to the list
+     *
+     * @return $this
+     */
+    public function tagResponse(ResponseInterface $response, array $tags, bool $replace = true);
+}
diff --git a/src/ResponseTagger.php b/src/ResponseTagger.php
index a091b58..cc033f2 100644
--- a/src/ResponseTagger.php
+++ b/src/ResponseTagger.php
@@ -12,6 +12,7 @@
 namespace FOS\HttpCache;
 
 use FOS\HttpCache\Exception\InvalidTagException;
+use FOS\HttpCache\ProxyClient\Response\TagResponseCapable;
 use FOS\HttpCache\TagHeaderFormatter\CommaSeparatedTagHeaderFormatter;
 use FOS\HttpCache\TagHeaderFormatter\TagHeaderFormatter;
 use Psr\Http\Message\ResponseInterface;
@@ -158,6 +159,14 @@ class ResponseTagger
             return $response;
         }
 
+        if ($this->proxy instanceof TagResponseCapable) {
+            $this->proxy ->tagResponse($response, $this->tags, $replace);
+            $this->clear();
+
+            return $response;
+        }
+
+        @trigger_error('In 3.0 it will be a requirement that proxy clients implements TagResponseCapable', E_USER_DEPRECATED)
         $tagsHeaderValue = $this->getTagsHeaderValue();
         $this->clear();
 
diff --git a/src/TagHeaderFormatter/CommaSeparatedTagHeaderFormatter.php b/src/TagHeaderFormatter/CommaSeparatedTagHeaderFormatter.php
index 625da9a..c302836 100644
--- a/src/TagHeaderFormatter/CommaSeparatedTagHeaderFormatter.php
+++ b/src/TagHeaderFormatter/CommaSeparatedTagHeaderFormatter.php
@@ -15,6 +15,7 @@ namespace FOS\HttpCache\TagHeaderFormatter;
  * Service for tag header formatting.
  *
  * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ * @deprecated As of 3.0 ProxyClients needs to implement TagResponseCapable instead.
  */
 class CommaSeparatedTagHeaderFormatter implements TagHeaderFormatter
 {
diff --git a/src/TagHeaderFormatter/MaxHeaderValueLengthFormatter.php b/src/TagHeaderFormatter/MaxHeaderValueLengthFormatter.php
index aab0eff..573054c 100644
--- a/src/TagHeaderFormatter/MaxHeaderValueLengthFormatter.php
+++ b/src/TagHeaderFormatter/MaxHeaderValueLengthFormatter.php
@@ -20,6 +20,7 @@ use FOS\HttpCache\Exception\InvalidTagException;
  * exceeds the configured limit.
  *
  * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ * @deprecated As of 3.0 ProxyClients needs to implement TagResponseCapable instead.
  */
 class MaxHeaderValueLengthFormatter implements TagHeaderFormatter
 {
diff --git a/src/TagHeaderFormatter/TagHeaderFormatter.php b/src/TagHeaderFormatter/TagHeaderFormatter.php
index 305b192..5d6c886 100644
--- a/src/TagHeaderFormatter/TagHeaderFormatter.php
+++ b/src/TagHeaderFormatter/TagHeaderFormatter.php
@@ -15,6 +15,7 @@ namespace FOS\HttpCache\TagHeaderFormatter;
  * The TagHeaderFormatter is used for cache tagging with HTTP headers.
  *
  * @author Yanick Witschi <yanick.witschi@terminal42.ch>
+ * @deprecated As of 3.0 ProxyClients needs to implement TagResponseCapable instead.
  */
 interface TagHeaderFormatter
 {

[Toflar]

Yes, pretty much exactly that 👍 I just think it makes no sense to abstract all the tagging logic because nobody will change the default behaviour in 99% of all use cases. So for 99% of all the users of this library, we're adding additional complexity for no real gain. Or I mean, if you read something in the docs along the lines of "We recommend using xkey when using Varnish", would you then decide to not use xkey and instead of using a whitespace or comma as a separator go for a | and implement the purging logic in Varnish yourself? I highly doubt this 😄 But if you really want to, you can still extend the Varnish proxy and override the tagResponse() method.

[dbu]

the reason to not use xkey with varnish would be when you do not have the power to have the xkey extension installed on varnish. but that indeed is rather an edge case and not the common thing. as invalidating the tags is a client responsibility anyways, it seems to make sense to also have the client tag the response. for version 2, i even think that tagResponse should become part of TagCapable itself. (note that we will want another interface in the bundle to support symfony response in addition to psr response)

that means that the proxy clients can't be final classes.