[ticket/17361] Use filesystem classes for tempnam and temp dir. Code reviews.

PHPBB-17361

phpBB/config/default/container/services_console.yml

@@ -299,6 +299,7 @@ services:
             - '@cache'
             - '@language'
             - '@storage.attachment'
+            - '@filesystem.temp'
             - '%core.root_path%'
             - '%core.php_ext%'
         tags:

phpBB/phpbb/console/command/thumbnail/generate.php

@@ -13,8 +13,10 @@
 
 namespace phpbb\console\command\thumbnail;
 
+use phpbb\attachment\attachment_category;
 use phpbb\cache\service;
 use phpbb\db\driver\driver_interface;
+use phpbb\filesystem\temp;
 use phpbb\language\language;
 use phpbb\storage\storage;
 use phpbb\user;
@@ -22,6 +24,7 @@ use Symfony\Component\Console\Command\Command as symfony_command;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
+use Symfony\Component\Filesystem\Filesystem as symfony_filesystem;
 
 class generate extends \phpbb\console\command\command
 {
@@ -41,11 +44,21 @@ class generate extends \phpbb\console\command\command
 	 */
 	protected $language;
 
+	/**
+	 * @var symfony_filesystem
+	 */
+	protected $symfony_filesystem;
+
 	/**
 	 * @var storage
 	 */
 	protected $storage;
 
+	/**
+	 * @var temp
+	 */
+	protected $temp;
+
 	/**
 	* phpBB root path
 	* @var string
@@ -67,15 +80,18 @@ class generate extends \phpbb\console\command\command
 	* @param service $cache The cache service
 	* @param language $language Language
 	* @param storage $storage Storage
+	* @param temp $temp Temp
 	* @param string $phpbb_root_path Root path
 	* @param string $php_ext PHP extension
 	*/
-	public function __construct(user $user, driver_interface $db, service $cache, language $language, storage $storage, string $phpbb_root_path, string $php_ext)
+	public function __construct(user $user, driver_interface $db, service $cache, language $language, storage $storage, temp $temp, string $phpbb_root_path, string $php_ext)
 	{
 		$this->db = $db;
 		$this->cache = $cache;
+		$this->symfony_filesystem = new symfony_filesystem();
 		$this->language = $language;
 		$this->storage = $storage;
+		$this->temp = $temp;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 
@@ -145,10 +161,10 @@ class generate extends \phpbb\console\command\command
 		$thumbnail_created = array();
 		while ($row = $this->db->sql_fetchrow($result))
 		{
-			if (isset($extensions[$row['extension']]['display_cat']) && $extensions[$row['extension']]['display_cat'] == \phpbb\attachment\attachment_category::IMAGE)
+			if (isset($extensions[$row['extension']]['display_cat']) && $extensions[$row['extension']]['display_cat'] == attachment_category::IMAGE)
 			{
-				$source = tempnam(sys_get_temp_dir(), 'thumbnail_source');
+				$source = $this->symfony_filesystem->tempnam($this->temp->get_dir(), 'thumbnail_source');
-				$destination = tempnam(sys_get_temp_dir(), 'thumbnail_destination');
+				$destination = $this->symfony_filesystem->tempnam($this->temp->get_dir(), 'thumbnail_destination');
 
 				file_put_contents($source, $this->storage->read($row['physical_filename']));
 

phpBB/phpbb/console/command/thumbnail/recreate.php

@@ -29,6 +29,7 @@ class recreate extends \phpbb\console\command\command
 	/**
 	 * Constructor
 	 *
+	 * @param user $user User
 	 * @param language $language Language
 	 */
 	public function __construct(user $user, language $language)

phpBB/phpbb/db/migration/data/v400/storage_backup_data.php

@@ -36,11 +36,11 @@ class storage_backup_data extends migration
 		$methods = ['sql', 'sql.gz', 'sql.bz2'];
 
 		$dir = $this->phpbb_root_path . 'store/';
-		$dh = @opendir($dir);
+		$handle = @opendir($dir);
 
-		if ($dh)
+		if ($handle)
 		{
-			while (($file = readdir($dh)) !== false)
+			while (($file = readdir($handle)) !== false)
 			{
 				if (preg_match('#^backup_(\d{10,})_(?:[a-z\d]{16}|[a-z\d]{32})\.(sql(?:\.(?:gz|bz2))?)$#i', $file, $matches))
 				{
@@ -58,7 +58,7 @@ class storage_backup_data extends migration
 
 			}
 
-			closedir($dh);
+			closedir($handle);
 		}
 	}
 }

phpBB/phpbb/filesystem/temp.php

@@ -13,8 +13,20 @@
 
 namespace phpbb\filesystem;
 
+use phpbb\filesystem\exception\filesystem_exception;
+
 class temp
 {
+	/**
+	 * @var filesystem
+	 */
+	protected $filesystem;
+
+	/**
+	 * @var string
+	 */
+	protected $cache_temp_dir;
+
 	/**
 	* @var string	Temporary directory path
 	*/
@@ -23,23 +35,10 @@ class temp
 	/**
 	 * Constructor
 	 */
-	public function __construct($filesystem, $cache_temp_dir)
+	public function __construct(filesystem $filesystem, string $cache_temp_dir)
 	{
-		$tmp_dir = (function_exists('sys_get_temp_dir')) ? sys_get_temp_dir() : '';
+		$this->filesystem = $filesystem;
-
+		$this->cache_temp_dir = $cache_temp_dir;
-		// Prevent trying to write to system temp dir in case of open_basedir
-		// restrictions being in effect
-		if (empty($tmp_dir) || !@file_exists($tmp_dir) || !@is_writable($tmp_dir))
-		{
-			$tmp_dir = $cache_temp_dir;
-
-			if (!is_dir($tmp_dir))
-			{
-				$filesystem->mkdir($tmp_dir, 0777);
-			}
-		}
-
-		$this->temp_dir = helper::realpath($tmp_dir);
 	}
 
 	/**
@@ -49,6 +48,30 @@ class temp
 	 */
 	public function get_dir()
 	{
+		if ($this->temp_dir === null)
+		{
+			$tmp_dir = (function_exists('sys_get_temp_dir')) ? sys_get_temp_dir() : '';
+
+			// Prevent trying to write to system temp dir in case of open_basedir
+			// restrictions being in effect
+			if (empty($tmp_dir) || !@file_exists($tmp_dir) || !@is_writable($tmp_dir))
+			{
+				$tmp_dir = $this->cache_temp_dir;
+
+				if (!is_dir($tmp_dir))
+				{
+					$this->filesystem->mkdir($tmp_dir, 0777);
+				}
+			}
+
+			$this->temp_dir = helper::realpath($tmp_dir);
+
+			if ($this->temp_dir === false)
+			{
+				throw new filesystem_exception('FILESYSTEM_CANNOT_CREATE_DIRECTORY', $tmp_dir);
+			}
+		}
+
 		return $this->temp_dir;
 	}
 }

phpBB/phpbb/storage/file_tracker.php

@@ -19,7 +19,6 @@ use phpbb\storage\exception\storage_exception;
 
 class file_tracker
 {
-
 	/**
 	 * @var db
 	 */
@@ -59,11 +58,11 @@ class file_tracker
 	 */
 	public function track_file(string $storage, string $path, int $size): void
 	{
-		$sql_ary = array(
+		$sql_ary = [
 			'file_path'		=> $path,
 			'storage'		=> $storage,
 			'filesize'		=> $size,
-		);
+		];
 
 		$sql = 'INSERT INTO ' . $this->storage_table . $this->db->sql_build_array('INSERT', $sql_ary);
 		$this->db->sql_query($sql);
@@ -80,10 +79,10 @@ class file_tracker
 	 */
 	public function untrack_file(string $storage, $path): void
 	{
-		$sql_ary = array(
+		$sql_ary = [
 			'file_path'		=> $path,
 			'storage'		=> $storage,
-		);
+		];
 
 		$sql = 'DELETE FROM ' . $this->storage_table . '
 			WHERE ' . $this->db->sql_build_array('DELETE', $sql_ary);
@@ -103,13 +102,14 @@ class file_tracker
 	 */
 	public function is_tracked(string $storage, string $path): bool
 	{
-		$sql_ary = array(
+		$sql_ary = [
 			'file_path'		=> $path,
 			'storage'		=> $storage,
-		);
+		];
 
-		$sql = 'SELECT file_id FROM ' .  $this->storage_table . '
+		$sql = 'SELECT file_id
-				WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary);
+			FROM ' .  $this->storage_table . '
+			WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary);
 		$result = $this->db->sql_query($sql);
 		$row = $this->db->sql_fetchrow($result);
 		$this->db->sql_freeresult($result);
@@ -128,13 +128,14 @@ class file_tracker
 	 */
 	public function file_size(string $storage, string $path): int
 	{
-		$sql_ary = array(
+		$sql_ary = [
 			'file_path'		=> $path,
 			'storage'		=> $storage,
-		);
+		];
 
-		$sql = 'SELECT filesize FROM ' .  $this->storage_table . '
+		$sql = 'SELECT filesize
-				WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary);
+			FROM ' .  $this->storage_table . '
+			WHERE ' . $this->db->sql_build_array('SELECT', $sql_ary);
 		$result = $this->db->sql_query($sql);
 		$row = $this->db->sql_fetchrow($result);
 		$this->db->sql_freeresult($result);

tests/console/thumbnail_test.php

@@ -25,6 +25,7 @@ class phpbb_console_command_thumbnail_test extends phpbb_database_test_case
 	protected $cache;
 	protected $user;
 	protected $storage;
+	protected $temp;
 	protected $phpEx;
 	protected $phpbb_root_path;
 	protected $application;
@@ -75,8 +76,13 @@ class phpbb_console_command_thumbnail_test extends phpbb_database_test_case
 			unlink($phpbb_root_path . 'files/' . $path);
 		});
 
+		$this->temp = $this->createMock('\phpbb\filesystem\temp');
+		$this->temp->method('get_dir')->willReturnCallback(function () {
+			return sys_get_temp_dir();
+		});
+
 		$this->application = new Application();
-		$this->application->add(new generate($this->user, $this->db, $this->cache, $this->language, $this->storage, $this->phpbb_root_path, $this->phpEx));
+		$this->application->add(new generate($this->user, $this->db, $this->cache, $this->language, $this->storage, $this->temp, $this->phpbb_root_path, $this->phpEx));
 		$this->application->add(new delete($this->user, $this->db, $this->language, $this->storage));
 		$this->application->add(new recreate($this->user, $this->language));