Merge pull request #1525 from samsonasik/rem-unneeded-try-catch-fopen

remove unneeded try {} catch {} on @fopen at Cache FileHandler::writeFile()
codeigniter4 · Nov 30, 2018 · ef8619d · ef8619d
2 parents db77587 + 57afc99
commit ef8619d
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 12 deletions.
diff --git a/system/Cache/Exceptions/CacheException.php b/system/Cache/Exceptions/CacheException.php
@@ -2,6 +2,14 @@
 
 class CacheException extends \RuntimeException implements ExceptionInterface
 {
+	/**
+	 * @return \CodeIgniter\Cache\Exceptions\CacheException
+	 */
+	public static function forUnableToWrite(string $path)
+	{
+		return new static(lang('Cache.unableToWrite', [$path]));
+	}
+
 	/**
 	 * @return \CodeIgniter\Cache\Exceptions\CacheException
 	 */

diff --git a/system/Cache/Handlers/FileHandler.php b/system/Cache/Handlers/FileHandler.php
@@ -37,6 +37,7 @@
  */
 
 use CodeIgniter\Cache\CacheInterface;
+use CodeIgniter\Cache\Exceptions\CacheException;
 
 class FileHandler implements CacheInterface
 {
@@ -59,10 +60,14 @@ class FileHandler implements CacheInterface
 
 	public function __construct($config)
 	{
-		$this->prefix = $config->prefix ?: '';
-		$this->path   = ! empty($config->storePath) ? $config->storePath : WRITEPATH . 'cache';
+		$path = ! empty($config->storePath) ? $config->storePath : WRITEPATH . 'cache';
+		if (! is_really_writable($path))
+		{
+			throw CacheException::forUnableToWrite($path);
+		}
 
-		$this->path = rtrim($this->path, '/') . '/';
+		$this->prefix = $config->prefix ?: '';
+		$this->path   = rtrim($path, '/') . '/';
 	}
 
 	//--------------------------------------------------------------------
@@ -329,14 +334,7 @@ protected function getItem(string $key)
 	 */
 	protected function writeFile($path, $data, $mode = 'wb')
 	{
-		try
-		{
-			if (($fp = @fopen($path, $mode)) === false)
-			{
-				return false;
-			}
-		}
-		catch (\ErrorException $e)
+		if (($fp = @fopen($path, $mode)) === false)
 		{
 			return false;
 		}

diff --git a/system/Language/en/Cache.php b/system/Language/en/Cache.php
@@ -15,6 +15,7 @@
  */
 
 return [
+   'unableToWrite'   => 'Cache unable to write to {0}',
    'invalidHandlers' => 'Cache config must have an array of $validHandlers.',
    'noBackup'        => 'Cache config must have a handler and backupHandler set.',
    'handlerNotFound' => 'Cache config has an invalid handler or backup handler specified.',

diff --git a/tests/system/Cache/Handlers/FileHandlerTest.php b/tests/system/Cache/Handlers/FileHandlerTest.php
@@ -65,6 +65,15 @@ public function testNew()
 		$this->assertInstanceOf(FileHandler::class, $this->fileHandler);
 	}
 
+	/**
+	 * @expectedException \CodeIgniter\Cache\Exceptions\CacheException
+	 */
+	public function testNewWithNonWritablePath()
+	{
+		chmod($this->config->storePath, 0444);
+		new FileHandler($this->config);
+	}
+
 	public function testSetDefaultPath()
 	{
 		//Initialize path

diff --git a/user_guide_src/source/libraries/caching.rst b/user_guide_src/source/libraries/caching.rst
@@ -212,7 +212,7 @@ File-based Caching
 Unlike caching from the Output Class, the driver file-based caching
 allows for pieces of view files to be cached. Use this with care, and
 make sure to benchmark your application, as a point can come where disk
-I/O will negate positive gains by caching.
+I/O will negate positive gains by caching. This require a writable cache directory to be really writable (0777).
 
 =================
 Memcached Caching