Remove routerCacheDisabled setting

We can use routerCacheFile to cover this. If routerCacheFile is false, then router caching is disabled. Updated the Router::createDispatcher() method so that it is clearer. Updated and tidied up unit tests.
slimphp · May 8, 2016 · f19cf30 · f19cf30
1 parent 7b47671
commit f19cf30
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 134 deletions.
diff --git a/Slim/Container.php b/Slim/Container.php
@@ -58,8 +58,7 @@ class Container extends PimpleContainer implements ContainerInterface
         'determineRouteBeforeAppMiddleware' => false,
         'displayErrorDetails' => false,
         'addContentLengthHeader' => true,
-        'routerCacheDisabled' => true,
-        'routerCacheFile' => '',
+        'routerCacheFile' => false,
     ];
 
     /**

diff --git a/Slim/DefaultServicesProvider.php b/Slim/DefaultServicesProvider.php
@@ -88,15 +88,12 @@ public function register($container)
              * @return RouterInterface
              */
             $container['router'] = function ($container) {
-                if (is_null($container->get('settings')['routerCacheDisabled'])
-                    || $container->get('settings')['routerCacheDisabled']
-                ) {
-                    return new Router;
-                } else {
-                    return (new Router)
-                        ->setCacheDisabled(false)
-                        ->setCacheFile($container->get('settings')['routerCacheFile']);
+                $routerCacheFile = false;
+                if (isset($container->get('settings')['routerCacheFile'])) {
+                    $routerCacheFile = $container->get('settings')['routerCacheFile'];
                 }
+
+                return (new Router)->setCacheFile($routerCacheFile);
             };
         }
 

diff --git a/Slim/Router.php b/Slim/Router.php
@@ -45,18 +45,11 @@ class Router implements RouterInterface
     protected $basePath = '';
 
     /**
-     * Fast route cache is disabled
+     * Path to fast route cache file. Set to false to disable route caching
      *
-     * @var bool
+     * @var string|False
      */
-    protected $cacheDisabled = true;
-
-    /**
-     * Path to fast route cache file
-     *
-     * @var string
-     */
-    protected $cacheFile;
+    protected $cacheFile = false;
 
     /**
      * Routes
@@ -112,37 +105,24 @@ public function setBasePath($basePath)
     }
 
     /**
-     * Enable/disable route cache
-     *
-     * @param bool $cacheDisabled
-     *
-     * @return self
-     */
-    public function setCacheDisabled($cacheDisabled)
-    {
-        $this->cacheDisabled = $cacheDisabled;
-
-        return $this;
-    }
-
-    /**
-     * Set path to fast route cache file
+     * Set path to fast route cache file. If this is false then route caching is disabled.
      *
-     * @param string $cacheFile
+     * @param string|false $cacheFile
      *
      * @return self
      */
     public function setCacheFile($cacheFile)
     {
-        if (!is_string($cacheFile)) {
-            throw new InvalidArgumentException('Router cacheFile must be a string');
+        if (!is_string($cacheFile) && $cacheFile !== false) {
+            throw new InvalidArgumentException('Router cacheFile must be a string or false');
         }
 
-        if (!is_writable(dirname($cacheFile))) {
+        $this->cacheFile = $cacheFile;
+
+        if ($cacheFile !== false && !is_writable(dirname($cacheFile))) {
             throw new RuntimeException('Router cacheFile directory must be writable');
         }
 
-        $this->cacheFile = $cacheFile;
 
         return $this;
     }
@@ -204,29 +184,28 @@ public function dispatch(ServerRequestInterface $request)
      */
     protected function createDispatcher()
     {
-        if (!$this->cacheDisabled && is_null($this->cacheFile)) {
-            throw new RuntimeException('Router cache enabled but cacheFile not set');
+        if ($this->dispatcher) {
+            return $this->dispatcher;
         }
 
-        return $this->dispatcher ?: call_user_func_array(
-            '\FastRoute\\' . (!$this->cacheDisabled ? 'cachedDispatcher' : 'simpleDispatcher'),
-            [
-                function (RouteCollector $routeCollector) {
-                    foreach ($this->getRoutes() as $route) {
-                        $routeCollector->addRoute(
-                            $route->getMethods(),
-                            $route->getPattern(),
-                            $route->getIdentifier()
-                        );
-                    }
-                },
-                [
-                    'routeParser' => $this->routeParser,
-                    'cacheFile' => $this->cacheFile,
-                    'cacheDisabled' => $this->cacheDisabled,
-                ],
-            ]
-        );
+        $routeDefinitionCallback = function (RouteCollector $r) {
+            foreach ($this->getRoutes() as $route) {
+                $r->addRoute($route->getMethods(), $route->getPattern(), $route->getIdentifier());
+            }
+        };
+
+        if ($this->cacheFile) {
+            $this->dispatcher = \FastRoute\cachedDispatcher($routeDefinitionCallback, [
+                'routeParser' => $this->routeParser,
+                'cacheFile' => $this->cacheFile,
+            ]);
+        } else {
+            $this->dispatcher = \FastRoute\simpleDispatcher($routeDefinitionCallback, [
+                'routeParser' => $this->routeParser,
+            ]);
+        }
+
+        return $this->dispatcher;
     }
 
     /**

diff --git a/tests/ContainerTest.php b/tests/ContainerTest.php
@@ -159,59 +159,6 @@ public function testMagicGetMethod()
 
     public function testRouteCacheDisabledByDefault()
     {
-        $this->assertTrue($this->container->get('settings')['routerCacheDisabled']);
-    }
-
-    /**
-     * Get cacheDisabled protected property from Route instance
-     *
-     * @return string
-     */
-    protected function getRouteCacheDisabled()
-    {
-        $router = $this->container['router'];
-        $getCacheDisabled = function ($router) {
-            return $router->cacheDisabled;
-        };
-        $getCacheDisabled = \Closure::bind($getCacheDisabled, null, $this->container['router']);
-        return $getCacheDisabled($router);
-    }
-
-    /**
-     * Get cacheFile protected property from Route instance
-     *
-     * @return string
-     */
-    protected function getRouteCacheFile()
-    {
-        $router = $this->container['router'];
-        $getCacheFile = function ($router) {
-            return $router->cacheFile;
-        };
-        $getCacheFile = \Closure::bind($getCacheFile, null, $this->container['router']);
-        return $getCacheFile($router);
-    }
-
-    /**
-     * Test that cache is not enabled and no cache file is set to Router
-     * when option is disabled from config
-     */
-    public function testRouteCacheFileNotSetWhenCacheDisabled()
-    {
-        $this->container->get('settings')['routerCacheDisabled'] = true;
-        $this->assertTrue($this->getRouteCacheDisabled());
-        $this->assertNull($this->getRouteCacheFile());
-    }
-
-    /**
-     * Test that cache is enabled and cache file is set to Router
-     * when option is enabled from config
-     */
-    public function testRouteCacheSetWhenCacheEnabled()
-    {
-        $this->container->get('settings')['routerCacheDisabled'] = false;
-        $this->container->get('settings')['routerCacheFile'] = 'slim';
-        $this->assertFalse($this->getRouteCacheDisabled());
-        $this->assertEquals('slim', $this->getRouteCacheFile());
+        $this->assertFalse($this->container->get('settings')['routerCacheFile']);
     }
 }
diff --git a/tests/RouterTest.php b/tests/RouterTest.php
@@ -308,9 +308,23 @@ public function testPathForWithModifiedRoutePattern()
     }
 
     /**
-     * Test cacheFile should be a string
+     * Test cacheFile may be set to false
      */
-    public function testSettingInvalidCacheFileNotString()
+    public function testSettingCacheFileToFalse()
+    {
+        $this->router->setCacheFile(false);
+
+        $class = new \ReflectionClass($this->router);
+        $property = $class->getProperty('cacheFile');
+        $property->setAccessible(true);
+
+        $this->assertFalse($property->getValue($this->router));
+    }
+
+    /**
+     * Test cacheFile should be a string or false
+     */
+    public function testSettingInvalidCacheFileValue()
     {
         $this->setExpectedException(
             '\InvalidArgumentException',
@@ -335,35 +349,35 @@ public function testSettingInvalidCacheFileNotExisting()
     }
 
     /**
-     * Test if cache is enabled but cache file is not set
+     * Test route dispatcher is created in case of route cache
      */
-    public function testCacheFileNotSetButCacheEnabled()
+    public function testCreateDispatcherWithRouteCache()
     {
-        $this->setExpectedException(
-            '\RuntimeException',
-            'Router cache enabled but cacheFile not set'
-        );
-
-        $this->router->setCacheDisabled(false);
-
+        $cacheFile = dirname(__FILE__) . '/' . uniqid(microtime(true));
+        $this->router->setCacheFile($cacheFile);
         $class = new \ReflectionClass($this->router);
         $method = $class->getMethod('createDispatcher');
         $method->setAccessible(true);
-        $method->invoke($this->router);
+
+        $dispatcher = $method->invoke($this->router);
+        $this->assertInstanceOf('\FastRoute\Dispatcher', $dispatcher);
+        $this->assertFileExists($cacheFile, 'cache file was not created');
+
+        unlink($cacheFile);
     }
 
     /**
-     * Test route dispatcher is created in case of route cache
+     * Calling createDispatcher as second time should give you back the same
+     * dispatcher as when you called it the first time.
      */
-    public function testCreateDispatcherWithRouteCache()
+    public function testCreateDispatcherReturnsSameDispatcherASecondTime()
     {
-        $cacheFile = dirname(__FILE__) . '/' . uniqid(microtime(true));
-        $this->router->setCacheDisabled(false);
-        $this->router->setCacheFile($cacheFile);
         $class = new \ReflectionClass($this->router);
         $method = $class->getMethod('createDispatcher');
         $method->setAccessible(true);
-        $this->assertInstanceOf('\FastRoute\Dispatcher', $method->invoke($this->router));
-        unlink($cacheFile);
+
+        $dispatcher = $method->invoke($this->router);
+        $dispatcher2 = $method->invoke($this->router);
+        $this->assertSame($dispatcher2, $dispatcher);
     }
 }