[WIP] Add type on annotation and handle it #161
Conversation
|
We should keep purge as the default type so we don't break BC. |
| @@ -220,6 +226,26 @@ private function invalidateRoutes(array $routes, Request $request) | |||
| } | |||
| } | |||
|
|
|||
| if (null === $route->getParams()) { | |||
| $route = $this->router->getRouteCollection()->get($route->getName()); | |||
There was a problem hiding this comment.
Last time I checked, getRouteCollection() gets an uncached set of routes. Clearly, this is not desirable.
There was a problem hiding this comment.
ok, how can we check if there are params on route ?
There was a problem hiding this comment.
http://symfony.com/doc/current/components/routing/introduction.html#the-all-in-one-router there is an option to set cache on router.
There was a problem hiding this comment.
#line 209 in Symfony/src/Component/Routing/Router.php
public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH)
{
return $this->getGenerator()->generate($name, $parameters, $referenceType);
}#line 137 in Symfony/src/Component/Routing/Generator/UrlGenerator.php
// the Route has a cache of its own and is not recompiled as long as it does not get modified
$compiledRoute = $route->compile();It safe to use Router.php.
There was a problem hiding this comment.
your code snippets don't do anything to prove @ddeboer is wrong (and this is logical, because he is not)
| @@ -148,7 +148,7 @@ public static function getSubscribedEvents() | |||
| * | |||
| * @param Request $request | |||
| */ | |||
| private function handleInvalidation(Request $request, Response $response) | |||
| protected function handleInvalidation(Request $request, Response $response) | |||
There was a problem hiding this comment.
why convert all methods to protected? this will make it hard to work on the code and keep BC.
There was a problem hiding this comment.
It make easier to override method, especially for specific need in ban or purge.
|
why don't we add a new annotation invalidateRegex, just as we have different commands for the cli? that way things would be more straightforward. either way, to allow to specify a route name instead of a regex, we need to "compile" the route ourselves by using Route::getPath() and Route::getRequirements() to replace |
| protected function preFillParams($route) | ||
| { | ||
| $params = array(); | ||
| $route = $this->router->getRouteCollection()->get($route->getName()); |
There was a problem hiding this comment.
not sure where the previous comments went, but we need to find a solution for this. i don't know what we can do - the unfortunate thing is that we can't find annotations at cache:warmup time or we could just precompile all this and not care much about performance.
There was a problem hiding this comment.
Just for reference, this is how I solved this before: driebit/DriebitHttpCacheBundle@c238a86. Unfortunately, I don’t think this solution will suffice here.
|
thanks a lot for updating. yeah i think this way it can work. what about my question:
about our performance worry: Router::getGenerator is setting up the UrlGenerator with a fresh collection and then caches it. so having the whole collection around seems normal. but i am not sure if its that costly to get the collection on the fly. for configured routes, this will all have been compiled when updating the cache after editing a routing file, no? |
|
Performance: is symfony/symfony#7368 (comment) not actual anymore? |
|
@dbu Separate annotation may be clearest, yes, but you lose the possibility to combine route names with wildcard parameters. So for instance, to invalidate all |
|
@ddeboer the separate annotation should not be less expressive then one that mixes up things, no? @lepiaf stof gave an idea how to do handle the route collection without creating a performance problem: #23 (comment) - do you want to work on that? |
|
ping |
| protected function preFillParams($routeName) | ||
| { | ||
| $params = array(); | ||
| $route = $this->router->getRouteCollection()->get($routeName->getName()); |
There was a problem hiding this comment.
please don't do this. Using ->getRouteCollection() at runtime kills your performance, because it triggers the loading of routes each time
|
Hi, I don't have time to work on it now. Can you open an issue and let somone else to work on it? |
|
we can just leave this open for now and see if somebody wants to work on it. |
|
@ddeboer what do we do with this one? i would tend to drop it, it seems very complicated. if there is somebody needing it, an option could be to add an annotation to ban on the static prefix of a route so there would be no need to integrate that deeply with the routing. |
|
Closing this for now. @lepiaf If you feel this is still necessary and have the time to work on it, please re-open. 😄 |
PR for #160
I have to replace UrlGenerator with Router because I need to know if a route has parameters, and then replace all token with a wildcard.
I also change method scope to protected. It make possible to fit for special need.