-
Notifications
You must be signed in to change notification settings - Fork 83
Add rule matchers for cacheable and must invalidate #354
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
Conversation
3f8e91a
to
17846cb
Compare
@@ -69,6 +69,6 @@ public function subrequestAction() | |||
|
|||
public function twigAction() | |||
{ | |||
$this->render('::tag.html.twig'); | |||
return $this->render('::tag.html.twig'); |
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.
Not sure how this worked before: a controller must return a response.
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.
it probably means the test does not check the returned response as that will be a 500 status...
32719c9
to
8dcbf46
Compare
if ($response->isSuccessful()) { | ||
$tags = $this->getAnnotationTags($request); | ||
if (!$this->cacheableRule->matches($request, $response) | ||
&& !$this->mustInvalidateRule->matches($request, $response) |
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.
this would be an odd situation when neither matches. but i think there is not much else we can do.
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 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.
$this->symfonyResponseTagger->tagSymfonyResponse($response); | ||
} | ||
} elseif (count($tags)) { | ||
// For non-safe methods, invalidate the tags | ||
} elseif (count($tags) |
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 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.
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.
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.
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.
sorry, you are right.
300, 301, | ||
404, 405, 410, 414, | ||
501, | ||
]; |
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.
isn't this list defined in symfony itself somewhere? do we need to redo it?
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.
Yes, in Response::isCacheable()
. However, Symfony’s list is incomplete, and I wanted to stick to RFC 7234 for good default behaviour.
|
||
public function __construct(array $additionalStatusCodes = []) | ||
{ | ||
$this->additionalStatusCodes = $additionalStatusCodes; |
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.
we could merge the additional codes with the default codes to make the check below simpler.
private $expression; | ||
private $expressionLanguage; | ||
|
||
public function matchExpression($expression) |
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 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.
@@ -69,6 +69,6 @@ public function subrequestAction() | |||
|
|||
public function twigAction() | |||
{ | |||
$this->render('::tag.html.twig'); | |||
return $this->render('::tag.html.twig'); |
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.
it probably means the test does not check the returned response as that will be a 500 status...
|
||
$this->mustInvalidateRule = new RuleMatcher( | ||
new UnsafeRequestMatcher(), | ||
new NonErrorResponseMatcher() |
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.
strictly speaking, a unit test should mock the rule matcher for the behaviour we expect and then have separate unit tests for each matcher...
That’s fine with me. What about ditching |
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...)
|
No problem. 😉 So, I’ll
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. |
i agree. specific behaviour can still be done on individual controller actions. |
43e5cdf
to
2462398
Compare
59c95ed
to
d97a502
Compare
// Change CacheableResponseMatcher to ExpressionResponseMatcher | ||
if ($config['response']['expression']) { | ||
$definition->setClass(ExpressionResponseMatcher::class) | ||
->setArguments([$config['response']['expression']]); |
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.
This is a bit odd, but it works: when an expression is configured, switch to another RuleMatcher, which is dedicated to expressions.
10adfaf
to
7f22e3c
Compare
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. 😉 |
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 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.
Resources/doc/reference/glossary.rst
Outdated
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. |
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.
erm, this does not match the list in other places. afaik its the same list that includes 414 and 501, no?
{ | ||
$container->setParameter( | ||
$this->getAlias().'.cacheable.response.additional_status', | ||
$config['response']['additional_status'] |
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.
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.
$this->symfonyResponseTagger->tagSymfonyResponse($response); | ||
} | ||
} elseif (count($tags)) { | ||
// For non-safe methods, invalidate the tags | ||
} elseif (count($tags) |
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.
sorry, you are right.
{ | ||
public function __construct( | ||
RequestMatcherInterface $requestMatcher = null, | ||
ResponseMatcherInterface $responseMatcher = null |
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.
can you please update the class doc here to reflect this change?
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.
Done.
src/Http/RuleMatcher.php
Outdated
*/ | ||
public function __construct(RequestMatcherInterface $requestMatcher, array $criteria) | ||
{ | ||
public function __construct( |
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.
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.
CHANGELOG.md
Outdated
* **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`). |
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.
we should also mention that the signature of RuleMatcher changed (or that its replaced by RequestResponseMapper or whatever if we decide to rename it)
020dac4
to
75da3ca
Compare
* Remove additional_cacheable and match_response. * Add global cacheable response config. * Add tests for reported bugs.
Thanks for the feedback! Updated. |
great job, thanks a lot! |
Fix #69, #241, #279, #286.
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:
additional_cacheable_status
per rule: do we really need that or can we suffice with one globally defined set of cacheable status codes?