-
Notifications
You must be signed in to change notification settings - Fork 84
Fix #105 #119
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
Fix #105 #119
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,6 @@ | |
use FOS\HttpCacheBundle\Configuration\Tag; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use Symfony\Component\HttpKernel\Event\FilterResponseEvent; | ||
use Symfony\Component\HttpKernel\KernelEvents; | ||
use Symfony\Component\ExpressionLanguage\ExpressionLanguage; | ||
|
@@ -78,10 +77,7 @@ public function onKernelResponse(FilterResponseEvent $event) | |
$tags[] = $tag; | ||
} | ||
foreach ($configuredTags['expressions'] as $expression) { | ||
$tags[] = $this->expressionLanguage->evaluate($expression, array( | ||
'request' => $request, | ||
'response' => $response, | ||
)); | ||
$tags[] = $this->evaluateTag($expression, $request); | ||
} | ||
} | ||
|
||
|
@@ -98,6 +94,16 @@ public function onKernelResponse(FilterResponseEvent $event) | |
} | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public static function getSubscribedEvents() | ||
{ | ||
return array( | ||
KernelEvents::RESPONSE => 'onKernelResponse' | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just moved this to have public methods above private ones. |
||
|
||
/** | ||
* Get the tags from the annotations on the controller that was used in the | ||
* request. | ||
|
@@ -118,9 +124,9 @@ private function getAnnotationTags(Request $request) | |
$tags = array(); | ||
foreach ($tagConfigurations as $tagConfiguration) { | ||
if (null !== $tagConfiguration->getExpression()) { | ||
$tags[] = $this->expressionLanguage->evaluate( | ||
$tags[] = $this->evaluateTag( | ||
$tagConfiguration->getExpression(), | ||
$request->attributes->all() | ||
$request | ||
); | ||
} else { | ||
$tags = array_merge($tags, $tagConfiguration->getTags()); | ||
|
@@ -131,12 +137,19 @@ private function getAnnotationTags(Request $request) | |
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* Evaluate a tag that contains expressions | ||
* | ||
* @param string $expression | ||
* @param Request $request | ||
* | ||
* @return string Evaluaated tag | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
*/ | ||
public static function getSubscribedEvents() | ||
private function evaluateTag($expression, Request $request) | ||
{ | ||
return array( | ||
KernelEvents::RESPONSE => 'onKernelResponse' | ||
return $this->expressionLanguage->evaluate( | ||
$expression, | ||
$request->attributes->all() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this does not have the Request itself anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it also did not have the request previously, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. depends. On line 81 it was passed. On line 122 it was not (and this call looks invalid by passing a Request in an argument expecting an array) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stof What do you mean exactly? https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/master/EventListener/TagSubscriber.php#L80 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ddeboer the comment to which I replied was refering to the previous code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ddeboer well, SensioFRameworkExtraBundle is inconsistent. For @dbu the behavior for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if we just add the request for advanced users, mention it in the doc but do not have any examples using the request, to keep them simple? the strategy to overwrite a possible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbu Ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see #144 |
||
); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,25 @@ | ||
test_list: | ||
pattern: /test/list | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::listAction } | ||
pattern: /test/list | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::listAction } | ||
methods: [GET] | ||
|
||
test_error: | ||
pattern: /test/error | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::errorAction } | ||
pattern: /test/error | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::errorAction } | ||
|
||
test_one: | ||
pattern: /test/{id} | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::itemAction } | ||
pattern: /test/{id} | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::itemAction } | ||
methods: [GET,POST] | ||
|
||
test_cached: | ||
pattern: /cached | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::contentAction } | ||
pattern: /cached/{id} | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::contentAction } | ||
methods: [GET,PUT] | ||
|
||
test_noncached: | ||
pattern: /noncached | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::contentAction } | ||
defaults: { _controller: FOS\HttpCacheBundle\Tests\Functional\Fixtures\Controller\TestController::contentAction } | ||
|
||
test_logout: | ||
pattern: /secured_area/logout |
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.
Unused.