Merge pull request #5248 from rubencm/ticket/15692

[ticket/15692] Move checks if file exists from adapter to storage

phpBB/includes/functions_download.php

@@ -223,9 +223,6 @@ function send_file_to_browser($attachment, $category)
 		}
 	}
 
-	// Close the db connection before sending the file etc.
-	file_gc(false);
-
 	if (!set_modified_headers($attachment['filetime'], $user->browser))
 	{
 		if ($size)
@@ -238,6 +235,9 @@ function send_file_to_browser($attachment, $category)
 
 		$fp = $storage->read_stream($filename);
 
+		// Close the db connection before sending the file etc.
+		file_gc(false);
+
 		if ($fp !== false)
 		{
 			$output = fopen('php://output', 'w+b');

phpBB/language/en/common.php

@@ -745,6 +745,7 @@ $lang = array_merge($lang, array(
 	'STORAGE_CANNOT_CREATE_DIR'		=> 'Can not create directory.',
 	'STORAGE_CANNOT_OPEN_FILE'		=> 'Can not open file.',
 	'STORAGE_CANNOT_CREATE_FILE'	=> 'Can not create file.',
+	'STORAGE_INVALID_RESOURCE'		=> 'Resource is invalid.',
 
 	'TB'				=> 'TB',
 	'TERMS_LINK'		=> 'Terms',

phpBB/phpbb/db/migration/data/v330/storage_track.php

@@ -86,7 +86,14 @@ class storage_track extends \phpbb\db\migration\container_aware_migration
 			$ext		= substr(strrchr($filename, '.'), 1);
 			$filename	= (int) $filename;
 
-			$storage->track_file($this->config['avatar_salt'] . '_' . ($avatar_group ? 'g' : '') . $filename . '.' . $ext);
+			try
+			{
+				$storage->track_file($this->config['avatar_salt'] . '_' . ($avatar_group ? 'g' : '') . $filename . '.' . $ext);
+			}
+			catch (\phpbb\storage\exception\exception $e)
+			{
+				// If file don't exist, don't track it
+			}
 		}
 		$this->db->sql_freeresult($result);
 	}
@@ -103,11 +110,25 @@ class storage_track extends \phpbb\db\migration\container_aware_migration
 
 		while ($row = $this->db->sql_fetchrow($result))
 		{
-			$storage->track_file($row['physical_filename']);
+			try
+			{
+				$storage->track_file($row['physical_filename']);
+			}
+			catch (\phpbb\storage\exception\exception $e)
+			{
+				// If file don't exist, don't track it
+			}
 
 			if ($row['thumbnail'] == 1)
 			{
-				$storage->track_file('thumb_' . $row['physical_filename']);
+				try
+				{
+					$storage->track_file('thumb_' . $row['physical_filename']);
+				}
+				catch (\phpbb\storage\exception\exception $e)
+				{
+					// If file don't exist, don't track it
+				}
 			}
 		}
 		$this->db->sql_freeresult($result);
@@ -125,8 +146,16 @@ class storage_track extends \phpbb\db\migration\container_aware_migration
 
 		while ($row = $this->db->sql_fetchrow($result))
 		{
-			$storage->track_file($row['filename']);
+			try
+			{
+				$storage->track_file($row['filename']);
+			}
+			catch (\phpbb\storage\exception\exception $e)
+			{
+				// If file don't exist, don't track it
+			}
 		}
+
 		$this->db->sql_freeresult($result);
 	}
 }

phpBB/phpbb/storage/adapter/adapter_interface.php

@@ -28,8 +28,7 @@ interface adapter_interface
 	 * @param string	path		The file to be written to.
 	 * @param string	content		The data to write into the file.
 	 *
-	 * @throws \phpbb\storage\exception\exception		When the file already exists
-	 * 													When the file cannot be written
+	 * @throws \phpbb\storage\exception\exception		When the file cannot be written
 	 */
 	public function put_contents($path, $content);
 
@@ -38,8 +37,7 @@ interface adapter_interface
 	 *
 	 * @param string	$path	The file to read
 	 *
-	 * @throws \phpbb\storage\exception\exception	When the file doesn't exist
-	 * 													When cannot read file contents
+	 * @throws \phpbb\storage\exception\exception	When cannot read file contents
 	 *
 	 * @return string	Returns file contents
 	 *
@@ -70,8 +68,7 @@ interface adapter_interface
 	 * @param string	$path_orig	The original file/direcotry
 	 * @param string	$path_dest	The target file/directory
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target exists
-	 * 													When file/directory cannot be renamed
+	 * @throws \phpbb\storage\exception\exception		When file/directory cannot be renamed
 	 */
 	public function rename($path_orig, $path_dest);
 
@@ -81,8 +78,7 @@ interface adapter_interface
 	 * @param string	$path_orig	The original filename
 	 * @param string	$path_dest	The target filename
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target exists
-	 * 													When the file cannot be copied
+	 * @throws \phpbb\storage\exception\exception		When the file cannot be copied
 	 */
 	public function copy($path_orig, $path_dest);
 

phpBB/phpbb/storage/adapter/local.php

@@ -118,11 +118,6 @@ class local implements adapter_interface, stream_interface
 	{
 		$this->ensure_directory_exists($path);
 
-		if ($this->exists($path))
-		{
-			throw new exception('STORAGE_FILE_EXISTS', $path);
-		}
-
 		try
 		{
 			$this->filesystem->dump_file($this->root_path . $this->get_path($path) . $this->get_filename($path), $content);
@@ -138,11 +133,6 @@ class local implements adapter_interface, stream_interface
 	 */
 	public function get_contents($path)
 	{
-		if (!$this->exists($path))
-		{
-			throw new exception('STORAGE_FILE_NO_EXIST', $path);
-		}
-
 		$content = @file_get_contents($this->root_path . $this->get_path($path) . $this->get_filename($path));
 
 		if ($content === false)
@@ -331,11 +321,6 @@ class local implements adapter_interface, stream_interface
 	{
 		$this->ensure_directory_exists($path);
 
-		if ($this->exists($path))
-		{
-			throw new exception('STORAGE_FILE_EXISTS', $path);
-		}
-
 		$stream = @fopen($this->root_path . $this->get_path($path) . $this->get_filename($path), 'w+b');
 
 		if (!$stream)
@@ -363,7 +348,7 @@ class local implements adapter_interface, stream_interface
 	 */
 	public function file_size($path)
 	{
-		$size = filesize($this->root_path . $this->get_path($path) . $this->get_filename($path));
+		$size = @filesize($this->root_path . $this->get_path($path) . $this->get_filename($path));
 
 		if ($size === null)
 		{
@@ -435,7 +420,7 @@ class local implements adapter_interface, stream_interface
 	 */
 	public function get_link($path)
 	{
-		return generate_board_url() . $this->path . $path;
+		return generate_board_url() . '/' . $this->path . $path;
 	}
 
 	/**

phpBB/phpbb/storage/storage.php

@@ -15,6 +15,7 @@ namespace phpbb\storage;
 
 use phpbb\cache\driver\driver_interface as cache;
 use phpbb\db\driver\driver_interface as db;
+use phpbb\storage\exception\exception;
 
 /**
  * @internal Experimental
@@ -106,6 +107,11 @@ class storage
 	 */
 	public function put_contents($path, $content)
 	{
+		if ($this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_EXISTS', $path);
+		}
+
 		$this->get_adapter()->put_contents($path, $content);
 		$this->track_file($path);
 	}
@@ -123,19 +129,25 @@ class storage
 	 */
 	public function get_contents($path)
 	{
+		if (!$this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path);
+		}
+
 		return $this->get_adapter()->get_contents($path);
 	}
 
 	/**
 	 * Checks the existence of files or directories
 	 *
-	 * @param string	$path	file/directory to check
+	 * @param string	$path		file/directory to check
+	 * @param bool		$full_check	check in the filesystem too
 	 *
-	 * @return bool	Returns true if the file/directory exist, false otherwise.
+	 * @return bool	Returns true if the file/directory exist, false otherwise
 	 */
-	public function exists($path)
+	public function exists($path, $full_check = false)
 	{
-		return $this->get_adapter()->exists($path);
+		return ($this->is_tracked($path) && (!$full_check || $this->get_adapter()->exists($path)));
 	}
 
 	/**
@@ -143,10 +155,16 @@ class storage
 	 *
 	 * @param string	$path	file/directory to remove
 	 *
-	 * @throws \phpbb\storage\exception\exception		When removal fails.
+	 * @throws \phpbb\storage\exception\exception		When removal fails
+	 *													When the file doesn't exist
 	 */
 	public function delete($path)
 	{
+		if (!$this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path);
+		}
+
 		$this->get_adapter()->delete($path);
 		$this->untrack_file($path);
 	}
@@ -157,11 +175,22 @@ class storage
 	 * @param string	$path_orig	The original file/direcotry
 	 * @param string	$path_dest	The target file/directory
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target exists
+	 * @throws \phpbb\storage\exception\exception		When the file doesn't exist
+	 *													When target exists
 	 * 													When file/directory cannot be renamed
 	 */
 	public function rename($path_orig, $path_dest)
 	{
+		if (!$this->exists($path_orig))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path_orig);
+		}
+
+		if ($this->exists($path_dest))
+		{
+			throw new exception('STORAGE_FILE_EXISTS', $path_dest);
+		}
+
 		$this->get_adapter()->rename($path_orig, $path_dest);
 		$this->track_rename($path_orig, $path_dest);
 	}
@@ -172,11 +201,22 @@ class storage
 	 * @param string	$path_orig	The original filename
 	 * @param string	$path_dest	The target filename
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target exists
+	 * @throws \phpbb\storage\exception\exception		When the file doesn't exist
+	 *													When target exists
 	 * 													When the file cannot be copied
 	 */
 	public function copy($path_orig, $path_dest)
 	{
+		if (!$this->exists($path_orig))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path_orig);
+		}
+
+		if ($this->exists($path_dest))
+		{
+			throw new exception('STORAGE_FILE_EXISTS', $path_dest);
+		}
+
 		$this->get_adapter()->copy($path_orig, $path_dest);
 		$this->track_file($path_dest);
 	}
@@ -186,12 +226,18 @@ class storage
 	 *
 	 * @param string	$path	File to read
 	 *
-	 * @throws \phpbb\storage\exception\exception		When unable to open file
+	 * @throws \phpbb\storage\exception\exception		When the file doesn't exist
+	 *													When unable to open file
 	 *
 	 * @return resource	Returns a file pointer
 	 */
 	public function read_stream($path)
 	{
+		if (!$this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path);
+		}
+
 		$stream = null;
 		$adapter = $this->get_adapter();
 
@@ -216,10 +262,21 @@ class storage
 	 * @param string	$path		The target file
 	 * @param resource	$resource	The resource
 	 *
-	 * @throws \phpbb\storage\exception\exception		When target file cannot be created
+	 * @throws \phpbb\storage\exception\exception		When the file exist
+	 *													When target file cannot be created
 	 */
 	public function write_stream($path, $resource)
 	{
+		if ($this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_EXISTS', $path);
+		}
+
+		if (!is_resource($resource))
+		{
+			throw new exception('STORAGE_INVALID_RESOURCE');
+		}
+
 		$adapter = $this->get_adapter();
 
 		if ($adapter instanceof stream_interface)
@@ -242,6 +299,11 @@ class storage
 	 */
 	public function track_file($path, $update = false)
 	{
+		if (!$this->get_adapter()->exists($path))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path);
+		}
+
 		$sql_ary = array(
 			'file_path'		=> $path,
 			'storage'		=> $this->get_name(),
@@ -256,7 +318,10 @@ class storage
 
 		if (!$row)
 		{
-			$file = $this->file_info($path);
+			// Don't call the file_info method, because it check's if the file is tracked
+			// and is not (for now). This method check if the file exists using the adapter
+			// at the beginning.
+			$file = new file_info($this->get_adapter(), $path);
 			$sql_ary['filesize'] = $file->size;
 
 			$sql = 'INSERT INTO ' . $this->storage_table . $this->db->sql_build_array('INSERT', $sql_ary);
@@ -295,6 +360,29 @@ class storage
 		$this->cache->destroy('_storage_' . $this->get_name() . '_numfiles');
 	}
 
+	/**
+	 * Check if a file is tracked
+	 *
+	 * @param string	$path	The file
+	 *
+	 * @return bool	True if file is tracked
+	 */
+	public function is_tracked($path)
+	{
+		$sql_ary = array(
+			'file_path'		=> $path,
+			'storage'		=> $this->get_name(),
+		);
+
+		$sql = 'SELECT file_id 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);
+
+		return ($row) ? true : false;
+	}
+
 	/**
 	 * Rename tracked file
 	 *
@@ -316,11 +404,17 @@ class storage
 	 * @param string	$path	The file
 	 *
 	 * @throws \phpbb\storage\exception\not_implemented	When the adapter doesnt implement the method
+	 *													When the file doesn't exist
 	 *
 	 * @return \phpbb\storage\file_info	Returns file_info object
 	 */
 	public function file_info($path)
 	{
+		if (!$this->exists($path))
+		{
+			throw new exception('STORAGE_FILE_NO_EXIST', $path);
+		}
+
 		return new file_info($this->get_adapter(), $path);
 	}
 

tests/attachment/delete_test.php

@@ -21,9 +21,6 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 	/** @var \phpbb\db\driver\driver_interface */
 	protected $db;
 
-	/** @var \phpbb\filesystem\filesystem */
-	protected $filesystem;
-
 	/** @var \phpbb\attachment\resync */
 	protected $resync;
 
@@ -47,22 +44,11 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 		$cache = $this->createMock('\phpbb\cache\driver\driver_interface');
 		$this->config = new \phpbb\config\config(array());
 		$this->db = $this->new_dbal();
-		$db_mock = $this->createMock('\phpbb\db\driver\driver_interface');
 		$this->resync = new \phpbb\attachment\resync($this->db);
-		$this->filesystem = $this->createMock('\phpbb\filesystem\filesystem', array('remove', 'exists'));
-		$this->filesystem->expects($this->any())
-			->method('remove')
-			->willReturn(false);
-		$this->filesystem->expects($this->any())
+		$this->storage = $this->createMock('\phpbb\storage\storage');
+		$this->storage->expects($this->any())
 			->method('exists')
 			->willReturn(true);
-		$adapter = new \phpbb\storage\adapter\local($this->filesystem, new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
-		$adapter->configure(['path' => 'files']);
-		$adapter_factory_mock = $this->createMock('\phpbb\storage\adapter_factory');
-		$adapter_factory_mock->expects($this->any())
-			->method('get')
-			->willReturn($adapter);
-		$this->storage = new \phpbb\storage\storage($db_mock, $cache, $adapter_factory_mock, '', '');
 		$this->dispatcher = new \phpbb_mock_event_dispatcher();
 		$this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->dispatcher, $this->resync, $this->storage);
 	}

tests/attachment/upload_test.php

@@ -86,9 +86,7 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case
 		));
 		$config = $this->config;
 		$this->db = $this->new_dbal();
-		$db_mock = $this->createMock('\phpbb\db\driver\driver_interface');
 		$this->cache = new \phpbb\cache\service(new \phpbb\cache\driver\dummy(), $this->config, $this->db, $phpbb_root_path, $phpEx);
-		$cache_mock = $this->createMock('\phpbb\cache\driver\driver_interface');
 		$this->request = $this->createMock('\phpbb\request\request');
 
 		$this->filesystem = new \phpbb\filesystem\filesystem();
@@ -105,13 +103,10 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case
 		$this->mimetype_guesser = new \phpbb\mimetype\guesser($guessers);
 		$this->plupload = new \phpbb\plupload\plupload($phpbb_root_path, $this->config, $this->request, new \phpbb\user($this->language, '\phpbb\datetime'), $this->php_ini, $this->mimetype_guesser);
 
-		$adapter = new \phpbb\storage\adapter\local($this->filesystem, new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
-		$adapter->configure(['path' => 'files']);
-		$adapter_factory_mock = $this->createMock('\phpbb\storage\adapter_factory');
-		$adapter_factory_mock->expects($this->any())
-			->method('get')
-			->willReturn($adapter);
-		$this->storage = new \phpbb\storage\storage($db_mock, $cache_mock, $adapter_factory_mock, '', '');
+		$this->storage = $this->createMock('\phpbb\storage\storage');
+		$this->storage->expects($this->any())
+			->method('free_space')
+			->willReturn(1024*1024); // 1gb
 
 		$factory_mock = $this->getMockBuilder('\phpbb\files\factory')
 			->disableOriginalConstructor()

tests/avatar/manager_test.php

@@ -30,25 +30,16 @@ class phpbb_avatar_manager_test extends \phpbb_database_test_case
 		global $phpbb_root_path, $phpEx;
 
 		// Mock phpbb_container
-
-		$cache = $this->createMock('\phpbb\cache\driver\driver_interface');
 		$phpbb_container = $this->createMock('Symfony\Component\DependencyInjection\ContainerInterface');
 		$phpbb_container->expects($this->any())
 			->method('get')
 			->will($this->returnArgument(0));
 
-		$filesystem = new \phpbb\filesystem\filesystem();
-		$adapter = new \phpbb\storage\adapter\local($filesystem, new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
-		$adapter->configure(['path' => 'images/avatars/upload']);
-		$db = $this->createMock('\phpbb\db\driver\driver_interface');
-		$adapter_factory_mock = $this->createMock('\phpbb\storage\adapter_factory');
-		$adapter_factory_mock->expects($this->any())
-			->method('get')
-			->willReturn($adapter);
-		$storage = new \phpbb\storage\storage($db, $cache, $adapter_factory_mock, '', '');
+		$storage = $this->createMock('\phpbb\storage\storage');
 
 		// Prepare dependencies for avatar manager and driver
 		$this->config = new \phpbb\config\config(array());
+		$cache = $this->createMock('\phpbb\cache\driver\driver_interface');
 		$path_helper =  new \phpbb\path_helper(
 			new \phpbb\symfony_request(
 				new phpbb_mock_request()

tests/notification/submit_post_base.php

@@ -56,7 +56,6 @@ abstract class phpbb_notification_submit_post_base extends phpbb_database_test_c
 		// Database
 		$this->db = $this->new_dbal();
 		$db = $this->db;
-		$db_mock = $this->createMock('\phpbb\db\driver\driver_interface');
 
 		// Auth
 		$auth = $this->createMock('\phpbb\auth\auth');

tests/privmsgs/delete_user_pms_test.php

@@ -87,21 +87,12 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case
 	{
 		global $db, $phpbb_container, $phpbb_root_path;
 
-		$cache = $this->createMock('\phpbb\cache\driver\driver_interface');
-
 		$db = $this->new_dbal();
-		$db_mock = $this->createMock('\phpbb\db\driver\driver_interface');
 
 		$phpbb_container = new phpbb_mock_container_builder();
 		$phpbb_container->set('notification_manager', new phpbb_mock_notification_manager());
 
-		$adapter = new \phpbb\storage\adapter\local(new \phpbb\filesystem\filesystem(), new \FastImageSize\FastImageSize(), new \phpbb\mimetype\guesser(array(new \phpbb\mimetype\extension_guesser)), $phpbb_root_path);
-		$adapter->configure(['path' => 'files']);
-		$adapter_factory_mock = $this->createMock('\phpbb\storage\adapter_factory');
-		$adapter_factory_mock->expects($this->any())
-			->method('get')
-			->willReturn($adapter);
-		$storage = new \phpbb\storage\storage($db_mock, $cache, $adapter_factory_mock, '', '');
+		$storage =  $this->createMock('\phpbb\storage\storage');
 
 		// Works as a workaround for tests
 		$phpbb_container->set('attachment.manager', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb_mock_event_dispatcher(), new \phpbb\attachment\resync($db), $storage));