Add rule matchers for cacheable and must invalidate #354

ddeboer · 2017-02-07T21:52:35Z

This is a proposal for fixing #69. It’s a WIP but please have a look before I continue: do we agree on the direction taken here?

The idea is to:

move the logic for determining ‘cacheable’ and ‘must invalidate’ into rule matchers and then re-use those between the listeners and any other classes that need the logic
expose those rule matchers as services for customising (e.g.) cacheable status codes
get rid of the additional_cacheable_status per rule: do we really need that or can we suffice with one globally defined set of cacheable status codes?

ddeboer · 2017-02-07T22:15:12Z

tests/Functional/Fixtures/Controller/TagController.php

@@ -69,6 +69,6 @@ public function subrequestAction()

    public function twigAction()
    {
-        $this->render('::tag.html.twig');
+        return $this->render('::tag.html.twig');


Not sure how this worked before: a controller must return a response.

it probably means the test does not check the returned response as that will be a 500 status...

dbu · 2017-02-08T07:42:03Z

src/EventListener/TagListener.php

-        if ($response->isSuccessful()) {
-            $tags = $this->getAnnotationTags($request);
+        if (!$this->cacheableRule->matches($request, $response)
+            && !$this->mustInvalidateRule->matches($request, $response)


this would be an odd situation when neither matches. but i think there is not much else we can do.

dbu

i very much like this, thanks!

i think this is going in the right direction. i think instead of having additional cacheable states globally or specifically, we should document which services to replace when you want to change the rules. otherwise we add a million configuration options that make everything hard to understand, but real world will probably still need a custom matcher anyways.

dbu · 2017-02-08T07:43:41Z

src/EventListener/TagListener.php

                $this->symfonyResponseTagger->tagSymfonyResponse($response);
            }
-        } elseif (count($tags)) {
-            // For non-safe methods, invalidate the tags
+        } elseif (count($tags)


i would prfer to return early if !count($tags). with your early return if its neither cacheable nor mustinvalidate, this here would be a simple else i think.

This cannot be moved to earlier in the method because $this->symfonyResponseTagger->tagSymfonyResponse($response); still has to happen: e.g. for when tags are set from Twig. The testTwigExtension test fails if we add if (!count($tags)) { return; } before this.

sorry, you are right.

dbu · 2017-02-08T07:44:51Z

src/Http/ResponseMatcher/CacheableResponseMatcher.php

+        300, 301,
+        404, 405, 410, 414,
+        501,
+    ];


isn't this list defined in symfony itself somewhere? do we need to redo it?

Yes, in Response::isCacheable(). However, Symfony’s list is incomplete, and I wanted to stick to RFC 7234 for good default behaviour.

dbu · 2017-02-08T07:45:22Z

src/Http/ResponseMatcher/CacheableResponseMatcher.php

+
+    public function __construct(array $additionalStatusCodes = [])
+    {
+        $this->additionalStatusCodes = $additionalStatusCodes;


we could merge the additional codes with the default codes to make the check below simpler.

dbu · 2017-02-08T07:47:42Z

src/Http/ResponseMatcher/ResponseMatcher.php

+    private $expression;
+    private $expressionLanguage;
+
+    public function matchExpression($expression)


i would prefer to call this setMatchExpression to make it explicit this is setting up the matcher. (and same for status code below). or have a constructor so that we can require at least one of the two criteria to exist?

the phpdoc for $expression should explain what context the expression gets.

dbu · 2017-02-08T07:49:37Z

tests/Functional/Fixtures/Controller/TagController.php

@@ -69,6 +69,6 @@ public function subrequestAction()

    public function twigAction()
    {
-        $this->render('::tag.html.twig');
+        return $this->render('::tag.html.twig');


it probably means the test does not check the returned response as that will be a 500 status...

dbu · 2017-02-08T07:50:42Z

tests/Unit/EventListener/TagListenerTest.php

+
+        $this->mustInvalidateRule = new RuleMatcher(
+            new UnsafeRequestMatcher(),
+            new NonErrorResponseMatcher()


strictly speaking, a unit test should mock the rule matcher for the behaviour we expect and then have separate unit tests for each matcher...

ddeboer · 2017-02-08T08:11:52Z

i think instead of having additional cacheable states globally or specifically, we should document which services to replace when you want to change the rules.

That’s fine with me. What about ditching ResponseMatcher (that takes an expression or status code) altogether and only having NonErrorResponseMatcher (invalidates) and CacheableResponseMatcher (set caching headers/tags)? Would people still need the additional_cacheable_status and match_response (response expression)?

dbu · 2017-02-08T08:28:31Z

i think instead of having additional cacheable states globally or specifically, we should document which services to replace when you want to change the rules. That’s fine with me. What about ditching |ResponseMatcher| (that takes an expression or status code) and /only/ having |NonErrorResponseMatcher| (invalidates) and |CacheableResponseMatcher| (set caching headers/tags)? Would people still need the |additional_cacheable_status| and |match_response| (response expression)?

hm. okay maybe it would still make sense to have the status list and expression as a global option. one each for cacheable and one for must invalidate. i would for example want to always invalidate cache, even on a 500 response (because part of the operation might have succeeded and beeing too agressive with invalidation is less risky than not agressive enough). if there is a globa configuration for the status codes and expression, its easy to adjust and could cover some cases. i feel that there are use cases for this without needing a completely custom matcher. (i know, i just say the opposite of what i said half an hour ago...)

ddeboer · 2017-02-08T08:40:44Z

No problem. 😉

So, I’ll

add (global) bundle configuration params for the cacheable and must_invalidate rule matchers;
remove the (specific) rule response matching: we only keep the request part.

My assumption here is that the definition of which request/response combinations are cacheable or must cause invalidation should be done on the application-level, not within individual rules.

dbu · 2017-02-08T08:50:29Z

i agree. specific behaviour can still be done on individual controller actions.

ddeboer · 2017-02-08T17:18:15Z

src/DependencyInjection/FOSHttpCacheExtension.php

+        // Change CacheableResponseMatcher to ExpressionResponseMatcher
+        if ($config['response']['expression']) {
+            $definition->setClass(ExpressionResponseMatcher::class)
+                ->setArguments([$config['response']['expression']]);


This is a bit odd, but it works: when an expression is configured, switch to another RuleMatcher, which is dedicated to expressions.

ddeboer · 2017-02-08T17:25:52Z

Ready for final review.

Decided not do document how to do fully custom cacheable/must_invalidate rules (by overriding the services in a compiler pass) for now. Let’s find out if users really need that before we bloat the docs. 😉

dbu

i think this is indeed a very good cleanup and makes the code a lot clearer and better structured!

added some small comments. we can also do some of that later and merge this, or decide to not do all of that.

dbu · 2017-02-09T09:22:52Z

Resources/doc/reference/glossary.rst

-        200, 203, 300, 301, 302, 404, 410. This range of status codes can be
-        extended with :ref:`additional_cacheable_status` or overridden with
-        :ref:`match_response`.
+        200, 203, 300, 301, 302, 404, 410.


erm, this does not match the list in other places. afaik its the same list that includes 414 and 501, no?

dbu · 2017-02-09T09:25:04Z

src/DependencyInjection/FOSHttpCacheExtension.php

+    {
+        $container->setParameter(
+            $this->getAlias().'.cacheable.response.additional_status',
+            $config['response']['additional_status']


should we do that in the else if the expression is empty? or do we want this parameter to always be defined? or we could make it not a container parameter but set the argument, like we do for expression.

dbu · 2017-02-09T09:28:25Z

src/EventListener/TagListener.php

                $this->symfonyResponseTagger->tagSymfonyResponse($response);
            }
-        } elseif (count($tags)) {
-            // For non-safe methods, invalidate the tags
+        } elseif (count($tags)


sorry, you are right.

dbu · 2017-02-09T09:55:21Z

src/Http/RuleMatcher.php

-    {
+    public function __construct(
+        RequestMatcherInterface $requestMatcher = null,
+        ResponseMatcherInterface $responseMatcher = null


can you please update the class doc here to reflect this change?

dbu · 2017-02-09T09:57:46Z

src/Http/RuleMatcher.php

-     */
-    public function __construct(RequestMatcherInterface $requestMatcher, array $criteria)
-    {
+    public function __construct(


please add a method doc that explains that both matchers must match for the rule matcher to match.

and i wonder if RuleMatcher is still the right name for this. its actually a RequestResponseMatcher or a HttpMatcher. not happy with both those names, but well... and we do a BC break here anyways, might as well rename it if we find a better name.

dbu · 2017-02-09T09:58:36Z

CHANGELOG.md

+* **BC break:** The `match_response` and `additional_cacheable_status` 
+  configuration parameters were removed for individual match rules. 
+
+* Cacheable status codes are now configured globally (`cacheable.response`).


we should also mention that the signature of RuleMatcher changed (or that its replaced by RequestResponseMapper or whatever if we decide to rename it)

* Remove additional_cacheable and match_response. * Add global cacheable response config. * Add tests for reported bugs.

ddeboer · 2017-02-09T11:24:13Z

Thanks for the feedback! Updated.

dbu · 2017-02-10T14:45:15Z

great job, thanks a lot!

ddeboer added the wip/poc label Feb 7, 2017

ddeboer force-pushed the rule-matcher branch 3 times, most recently from 3f8e91a to 17846cb Compare February 7, 2017 22:14

ddeboer commented Feb 7, 2017

View reviewed changes

ddeboer force-pushed the rule-matcher branch 3 times, most recently from 32719c9 to 8dcbf46 Compare February 8, 2017 07:39

dbu reviewed Feb 8, 2017

View reviewed changes

ddeboer force-pushed the rule-matcher branch from 8dcbf46 to 7653cdd Compare February 8, 2017 08:05

ddeboer force-pushed the rule-matcher branch 4 times, most recently from 43e5cdf to 2462398 Compare February 8, 2017 12:16

ddeboer changed the title ~~[WIP] Add rule matchers for cacheable and must invalidate~~ Add rule matchers for cacheable and must invalidate Feb 8, 2017

ddeboer force-pushed the rule-matcher branch 4 times, most recently from 59c95ed to d97a502 Compare February 8, 2017 17:16

ddeboer commented Feb 8, 2017

View reviewed changes

ddeboer force-pushed the rule-matcher branch 2 times, most recently from 10adfaf to 7f22e3c Compare February 8, 2017 17:25

ddeboer added this to the 2.0 milestone Feb 8, 2017

ddeboer force-pushed the rule-matcher branch from 7f22e3c to afeb4f6 Compare February 8, 2017 17:26

dbu reviewed Feb 9, 2017

View reviewed changes

ddeboer force-pushed the rule-matcher branch 3 times, most recently from 020dac4 to 75da3ca Compare February 9, 2017 11:22

Add rule matchers for cacheable and must invalidate

2dc0a5b

* Remove additional_cacheable and match_response. * Add global cacheable response config. * Add tests for reported bugs.

ddeboer force-pushed the rule-matcher branch from 75da3ca to 2dc0a5b Compare February 9, 2017 11:23

dbu merged commit 2463553 into master Feb 10, 2017

dbu removed the wip/poc label Feb 10, 2017

dbu deleted the rule-matcher branch February 10, 2017 14:45

dbu mentioned this pull request Feb 10, 2017

clarify documentation of cacheable response status #355

Merged

This was referenced Feb 10, 2017

Taging via configuration doesn't work for BAN requests #241

Closed

[cache tagging] OPTIONS call invalidates tags #279

Closed

Cache tags not being added on redirect response. #286

Closed

dbu mentioned this pull request Jun 20, 2017

cache control listener should only set caching headers on cacheable responses #372

Closed

2 tasks

Add rule matchers for cacheable and must invalidate #354

Add rule matchers for cacheable and must invalidate #354

Uh oh!

Conversation

ddeboer commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu commented Feb 8, 2017 via email

Uh oh!

ddeboer commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu left a comment

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!

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 Feb 9, 2017

Uh oh!

dbu commented Feb 10, 2017

Uh oh!

Uh oh!

ddeboer commented Feb 7, 2017 •

edited

Loading

ddeboer Feb 8, 2017 •

edited

Loading

ddeboer commented Feb 8, 2017 •

edited

Loading

ddeboer commented Feb 8, 2017 •

edited

Loading

ddeboer commented Feb 8, 2017 •

edited

Loading