Permalink
Browse files

Do not add closure routes to cache. Fixes #4301

  • Loading branch information...
1 parent be7dfa5 commit c8fa0a9bae450686cb217033f374d5c4b4e02356 @zombor zombor committed Jun 26, 2012
Showing with 23 additions and 1 deletion.
  1. +14 −1 classes/kohana/route.php
  2. +9 −0 tests/kohana/RouteTest.php
View
@@ -155,8 +155,16 @@ public static function cache($save = FALSE)
{
if ($save === TRUE)
{
+ $routes = array();
+ foreach (Route::$_routes as $name => $route)
+ {
+ if ( ! $route->is_closure())
+ {
+ $routes[$name] = $route;
+ }
+ }
// Cache all defined routes
- Kohana::cache('Route::cache()', Route::$_routes);
+ Kohana::cache('Route::cache()', $routes);
}
else
{
@@ -545,4 +553,9 @@ public function uri(array $params = NULL)
return $uri;
}
+ protected function is_closure()
+ {
+ return is_object($this->_callback) AND get_class($this->_callback) == 'Closure';
+ }
+
} // End Route
@@ -114,6 +114,15 @@ public function test_cache_stores_route_objects()
$this->assertEquals($routes, Route::all());
}
+ public function test_cache_does_not_cache_lambdas()
+ {
+ Route::set('lambda', function($uri) {} );
+
+ Route::cache(TRUE);
+ $this->assertTrue(Route::cache());
+ $this->assertArrayNotHasKey('lambda', Route::all());
+ }
+
/**
* Route::cache() should return FALSE if cached routes could not be found
*

7 comments on commit c8fa0a9

Member

Zeelot replied Aug 6, 2012

@zombor this was never merged into 3.3/develop, what should we do there, where the routes can have many filters, some of which, could be lambda's?

Contributor

zombor replied Aug 6, 2012

I'm not really that familiar with 3.3 routes. It seems filters should not be cached at all? How could they be?

Member

Zeelot replied Aug 6, 2012

I'm not familiar with route caching ;) we could just not cache anything with a filter? Is the issue with any callback or only lambdas? Because you can probably cache anything that has regular callbacks (I believe the caching issue is specific to lambdas).

Is caching done when the route is created or later on? Because filters are chained on after creating the object (like defaults())

Contributor

zombor replied Aug 6, 2012

Route caching basically just serializes the computed route array. Anything that isn't serializable (callable things) will cause an error.

Caching happens when Route::cache() is called. It either loads the cached routes and returns them, or caches them and returns them.

Now that I look at my diff above, I'm not sure it's correct. I think that caching should just fail if there's a non-cacheable entry in the route array.

Member

Zeelot replied Aug 6, 2012

I agree (I think). Caching part of the array wouldn't really work. But should we really go through and make sure everything is cachable? Shouldn't we just let the fatal error come up? The dev should know that as soon as he writes a route that isn't cachable (never necessary) he should simply stop caching routes.

Contributor

zombor replied Aug 6, 2012

Fine with me. This should probably be reverted then. The stock error message is not very friendly, maybe we can try/catch and convert it into a nicer exception (if it's not a fatal)?

Member

Zeelot replied Aug 6, 2012

sure (I think the lambda one is fatal, though... not sure). I've reopened http://dev.kohanaframework.org/issues/4301 and I can revert this + add the try/catch (unless you have time to do it before me)

Please sign in to comment.