Skip to content

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

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Conversation

ddeboer
Copy link
Member

@ddeboer ddeboer commented Feb 7, 2017

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:

  1. 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
  2. expose those rule matchers as services for customising (e.g.) cacheable status codes
  3. 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 ddeboer added the wip/poc label Feb 7, 2017
@ddeboer ddeboer force-pushed the rule-matcher branch 3 times, most recently from 3f8e91a to 17846cb Compare February 7, 2017 22:14
@@ -69,6 +69,6 @@ public function subrequestAction()

public function twigAction()
{
$this->render('::tag.html.twig');
return $this->render('::tag.html.twig');
Copy link
Member Author

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.

Copy link
Contributor

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

@ddeboer ddeboer force-pushed the rule-matcher branch 3 times, most recently from 32719c9 to 8dcbf46 Compare February 8, 2017 07:39
if ($response->isSuccessful()) {
$tags = $this->getAnnotationTags($request);
if (!$this->cacheableRule->matches($request, $response)
&& !$this->mustInvalidateRule->matches($request, $response)
Copy link
Contributor

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.

Copy link
Contributor

@dbu dbu left a 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)
Copy link
Contributor

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.

Copy link
Member Author

@ddeboer ddeboer Feb 8, 2017

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.

Copy link
Contributor

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,
];
Copy link
Contributor

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?

Copy link
Member Author

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

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

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

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()
Copy link
Contributor

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

@ddeboer
Copy link
Member Author

ddeboer commented Feb 8, 2017

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
Copy link
Contributor

dbu commented Feb 8, 2017 via email

@ddeboer
Copy link
Member Author

ddeboer commented Feb 8, 2017

No problem. 😉

So, I’ll

  1. add (global) bundle configuration params for the cacheable and must_invalidate rule matchers;
  2. 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
Copy link
Contributor

dbu commented Feb 8, 2017

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

@ddeboer ddeboer force-pushed the rule-matcher branch 4 times, most recently from 43e5cdf to 2462398 Compare February 8, 2017 12:16
@ddeboer 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 ddeboer force-pushed the rule-matcher branch 4 times, most recently from 59c95ed to d97a502 Compare February 8, 2017 17:16
// Change CacheableResponseMatcher to ExpressionResponseMatcher
if ($config['response']['expression']) {
$definition->setClass(ExpressionResponseMatcher::class)
->setArguments([$config['response']['expression']]);
Copy link
Member 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 odd, but it works: when an expression is configured, switch to another RuleMatcher, which is dedicated to expressions.

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

ddeboer commented Feb 8, 2017

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

@ddeboer ddeboer added this to the 2.0 milestone Feb 8, 2017
Copy link
Contributor

@dbu dbu left a 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.

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.
Copy link
Contributor

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']
Copy link
Contributor

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

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
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

*/
public function __construct(RequestMatcherInterface $requestMatcher, array $criteria)
{
public function __construct(
Copy link
Contributor

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`).
Copy link
Contributor

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)

@ddeboer ddeboer force-pushed the rule-matcher branch 3 times, most recently from 020dac4 to 75da3ca Compare February 9, 2017 11:22
* Remove additional_cacheable and match_response.
* Add global cacheable response config.
* Add tests for reported bugs.
@ddeboer
Copy link
Member Author

ddeboer commented Feb 9, 2017

Thanks for the feedback! Updated.

@dbu dbu merged commit 2463553 into master Feb 10, 2017
@dbu dbu removed the wip/poc label Feb 10, 2017
@dbu
Copy link
Contributor

dbu commented Feb 10, 2017

great job, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuleMatcher for TagSubscriber, InvalidationSubscriber
2 participants