[ticket/14168] Move phpbb_unlink() into attachment delete class

PHPBB3-14168

phpBB/config/default/container/services_files.yml

@@ -6,7 +6,9 @@ services:
             - @config
             - @dbal.conn
             - @dispatcher
+            - @filesystem
             - @attachment.resync
+            - %core.root_path%
 
     attachment.resync:
         class: phpbb\attachment\resync

phpBB/includes/functions_admin.php

@@ -1243,27 +1243,19 @@ function update_posted_info(&$topic_ids)
 
 /**
 * Delete attached file
+*
+* @deprecated 3.2.0-a1 (To be removed: 3.4.0)
 */
 function phpbb_unlink($filename, $mode = 'file', $entry_removed = false)
 {
-	global $db, $phpbb_root_path, $config;
+	global $phpbb_container;;
 
-	// Because of copying topics or modifications a physical filename could be assigned more than once. If so, do not remove the file itself.
+	/** @var \phpbb\attachment\delete $attachment_delete */
-	$sql = 'SELECT COUNT(attach_id) AS num_entries
+	$attachment_delete = $phpbb_container->get('attachment.delete');
-		FROM ' . ATTACHMENTS_TABLE . "
+	$unlink = $attachment_delete->unlink_attachment($filename, $mode, $entry_removed);
-		WHERE physical_filename = '" . $db->sql_escape(utf8_basename($filename)) . "'";
+	unset($attachment_delete);
-	$result = $db->sql_query($sql);
-	$num_entries = (int) $db->sql_fetchfield('num_entries');
-	$db->sql_freeresult($result);
 
-	// Do not remove file if at least one additional entry with the same name exist.
+	return $unlink;
-	if (($entry_removed && $num_entries > 0) || (!$entry_removed && $num_entries > 1))
-	{
-		return false;
-	}
-
-	$filename = ($mode == 'thumbnail') ? 'thumb_' . utf8_basename($filename) : utf8_basename($filename);
-	return @unlink($phpbb_root_path . $config['upload_path'] . '/' . $filename);
 }
 
 /**

phpBB/phpbb/attachment/delete.php

@@ -16,6 +16,7 @@ namespace phpbb\attachment;
 use \phpbb\config\config;
 use \phpbb\db\driver\driver_interface;
 use \phpbb\event\dispatcher;
+use \phpbb\filesystem\filesystem;
 
 /**
  * Attachment delete class
@@ -23,18 +24,24 @@ use \phpbb\event\dispatcher;
 
 class delete
 {
-	/** @var \phpbb\config\config */
+	/** @var config */
 	protected $config;
 
-	/** @var \phpbb\db\driver\driver_interface */
+	/** @var driver_interface */
 	protected $db;
 
 	/** @var \phpbb\event\dispatcher */
 	protected $dispatcher;
 
-	/** @var \phpbb\attachment\resync */
+	/** @var filesystem  */
+	protected $filesystem;
+
+	/** @var resync */
 	protected $resync;
 
+	/** @var string phpBB root path */
+	protected $phpbb_root_path;
+
 	/** @var array Attachement IDs */
 	protected $ids;
 
@@ -65,14 +72,18 @@ class delete
 	 * @param config $config
 	 * @param driver_interface $db
 	 * @param dispatcher $dispatcher
+	 * @param filesystem $filesystem
 	 * @param resync $resync
+	 * @param string $phpbb_root_path
 	 */
-	public function __construct(config $config, driver_interface $db, dispatcher $dispatcher, resync $resync)
+	public function __construct(config $config, driver_interface $db, dispatcher $dispatcher, filesystem $filesystem, resync $resync, $phpbb_root_path)
 	{
 		$this->config = $config;
 		$this->db = $db;
 		$this->dispatcher = $dispatcher;
+		$this->filesystem = $filesystem;
 		$this->resync = $resync;
+		$this->phpbb_root_path = $phpbb_root_path;
 	}
 
 	/**
@@ -152,7 +163,7 @@ class delete
 		}
 
 		// Delete attachments from filesystem
-		$this->delete_attachments_from_filesystem();
+		$this->remove_from_filesystem();
 
 		// If we do not resync, we do not need to adjust any message, post, topic or user entries
 		if (!$resync)
@@ -319,13 +330,13 @@ class delete
 	/**
 	 * Delete attachments from filesystem
 	 */
-	protected function delete_attachments_from_filesystem()
+	protected function remove_from_filesystem()
 	{
 		$space_removed = $files_removed = 0;
 
 		foreach ($this->physical as $file_ary)
 		{
-			if (phpbb_unlink($file_ary['filename'], 'file', true) && !$file_ary['is_orphan'])
+			if ($this->unlink_attachment($file_ary['filename'], 'file', true) && !$file_ary['is_orphan'])
 			{
 				// Only non-orphaned files count to the file size
 				$space_removed += $file_ary['filesize'];
@@ -334,7 +345,7 @@ class delete
 
 			if ($file_ary['thumbnail'])
 			{
-				phpbb_unlink($file_ary['filename'], 'thumbnail', true);
+				$this->unlink_attachment($file_ary['filename'], 'thumbnail', true);
 			}
 		}
 
@@ -376,4 +387,47 @@ class delete
 			$this->config->increment('num_files', $files_removed * (-1), false);
 		}
 	}
+
+	/**
+	 * Delete attachment from filesystem
+	 *
+	 * @param string $filename Filename of attachment
+	 * @param string $mode Delete mode
+	 * @param bool $entry_removed Whether entry was removed. Defaults to false
+	 * @return bool True if file was removed, false if not
+	 */
+	public function unlink_attachment($filename, $mode = 'file', $entry_removed = false)
+	{
+		// Because of copying topics or modifications a physical filename could be assigned more than once. If so, do not remove the file itself.
+		$sql = 'SELECT COUNT(attach_id) AS num_entries
+		FROM ' . ATTACHMENTS_TABLE . "
+		WHERE physical_filename = '" . $this->db->sql_escape(utf8_basename($filename)) . "'";
+		$result = $this->db->sql_query($sql);
+		$num_entries = (int) $this->db->sql_fetchfield('num_entries');
+		$this->db->sql_freeresult($result);
+
+		// Do not remove file if at least one additional entry with the same name exist.
+		if (($entry_removed && $num_entries > 0) || (!$entry_removed && $num_entries > 1))
+		{
+			return false;
+		}
+
+		$filename = ($mode == 'thumbnail') ? 'thumb_' . utf8_basename($filename) : utf8_basename($filename);
+		$filepath = $this->phpbb_root_path . $this->config['upload_path'] . '/' . $filename;
+
+		try
+		{
+			if ($this->filesystem->exists($filepath))
+			{
+				$this->filesystem->remove($this->phpbb_root_path . $this->config['upload_path'] . '/' . $filename);
+				return true;
+			}
+		}
+		catch (\phpbb\filesystem\exception\filesystem_exception $exception)
+		{
+			// Fail is covered by return statement below
+		}
+
+		return false;
+	}
 }

tests/attachment/delete_test.php

@@ -21,12 +21,17 @@ 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;
 
 	/** @var \phpbb\attachment\delete */
 	protected $attachment_delete;
 
+	protected $phpbb_root_path;
+
 	public function getDataSet()
 	{
 		return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/resync.xml');
@@ -34,7 +39,7 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 
 	public function setUp()
 	{
-		global $db;
+		global $db, $phpbb_root_path;
 
 		parent::setUp();
 
@@ -42,7 +47,15 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 		$this->db = $this->new_dbal();
 		$db = $this->db;
 		$this->resync = new \phpbb\attachment\resync($this->db);
-		$this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->resync);
+		$this->filesystem = $this->getMock('\phpbb\filesystem\filesystem', array('remove', 'exists'));
+		$this->filesystem->expects($this->any())
+			->method('remove')
+			->willReturn(false);
+		$this->filesystem->expects($this->any())
+			->method('exists')
+			->willReturn(true);
+		$this->phpbb_root_path = $phpbb_root_path;
+		$this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $phpbb_root_path);
 	}
 
 	public function data_attachment_delete()
@@ -55,7 +68,7 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 			array('attach', array(1,2), true, 2),
 			array('post', 5, false, 0),
 			array('topic', 5, false, 0),
-			array('topic', 1, true, 2),
+			array('topic', 1, true, 3),
 			array('user', 1, false, 0),
 		);
 	}
@@ -74,4 +87,40 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case
 
 		$this->assertSame($expected, $this->attachment_delete->delete($mode, $ids, $resync));
 	}
+
+	public function data_attachment_unlink()
+	{
+		return array(
+			array(true, true, true),
+			array(true, false, false),
+			array(true, true, false, true),
+		);
+	}
+
+	/**
+	 * @dataProvider data_attachment_unlink
+	 */
+	public function test_attachment_delete_success($remove_success, $exists_success, $expected, $throw_exception = false)
+	{
+		$this->filesystem = $this->getMock('\phpbb\filesystem\filesystem', array('remove', 'exists'));
+		if ($throw_exception)
+		{
+			$this->filesystem->expects($this->any())
+				->method('remove')
+				->willThrowException(new \phpbb\filesystem\exception\filesystem_exception);;
+		}
+		else
+		{
+			$this->filesystem->expects($this->any())
+				->method('remove')
+				->willReturn($remove_success);
+		}
+
+		$this->filesystem->expects($this->any())
+			->method('exists')
+			->willReturn($exists_success);
+
+		$this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $this->phpbb_root_path);
+		$this->assertSame($expected, $this->attachment_delete->unlink_attachment('foobar'));
+	}
 }

tests/attachment/fixtures/resync.xml

@@ -6,12 +6,16 @@
         <column>in_message</column>
         <column>is_orphan</column>
         <column>attach_comment</column>
+        <column>physical_filename</column>
+        <column>thumbnail</column>
         <row>
             <value>1</value>
             <value>1</value>
             <value>0</value>
             <value>0</value>
             <value>foo</value>
+            <value>foo</value>
+            <value>0</value>
         </row>
         <row>
             <value>1</value>
@@ -19,6 +23,17 @@
             <value>1</value>
             <value>0</value>
             <value>foo2</value>
+            <value>foo2</value>
+            <value>0</value>
+        </row>
+        <row>
+            <value>1</value>
+            <value>1</value>
+            <value>1</value>
+            <value>0</value>
+            <value>foo2</value>
+            <value>foo2</value>
+            <value>1</value>
         </row>
     </table>
     <table name="phpbb_posts">

tests/content_visibility/delete_post_test.php

@@ -312,7 +312,7 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case
 		$lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx);
 		$lang = new \phpbb\language\language($lang_loader);
 		$user = new \phpbb\user($lang, '\phpbb\datetime');
-		$attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db));
+		$attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path);
 
 		$phpbb_dispatcher = new phpbb_mock_event_dispatcher();
 

tests/functions_user/delete_user_test.php

@@ -25,7 +25,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case
 	{
 		parent::setUp();
 
-		global $cache, $config, $db, $phpbb_dispatcher, $phpbb_container;
+		global $cache, $config, $db, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path;
 
 		$db = $this->db = $this->new_dbal();
 		$config = new \phpbb\config\config(array(
@@ -36,7 +36,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case
 		$phpbb_dispatcher = new phpbb_mock_event_dispatcher();
 		$phpbb_container = new phpbb_mock_container_builder();
 		$phpbb_container->set('notification_manager', new phpbb_mock_notification_manager());
-		$phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db)));
+		$phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path));
 		$phpbb_container->set(
 			'auth.provider.db',
 			new phpbb_mock_auth_provider()

tests/privmsgs/delete_user_pms_test.php

@@ -85,13 +85,13 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case
 	*/
 	public function test_delete_user_pms($delete_user, $remaining_privmsgs, $remaining_privmsgs_to)
 	{
-		global $db, $phpbb_container;
+		global $db, $phpbb_container, $phpbb_root_path;
 
 		$db = $this->new_dbal();
 
 		$phpbb_container = new phpbb_mock_container_builder();
 		$phpbb_container->set('notification_manager', new phpbb_mock_notification_manager());
-		$phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\attachment\resync($db)));
+		$phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path));
 
 		phpbb_delete_user_pms($delete_user);