[ticket/13904] Improve test coverage of filespec class

PHPBB3-13904

phpBB/config/default/container/services_files.yml

@@ -14,6 +14,7 @@ services:
             - @filesystem
             - @language
             - @php_ini
+            - @upload_imagesize
             - %core.root_path%
             - @mimetype.guesser
             - @plupload

phpBB/phpbb/files/filespec.php

@@ -75,6 +75,9 @@ class filespec
 	/** @var \bantu\IniGetWrapper\IniGetWrapper ini_get() wrapper class */
 	protected $php_ini;
 
+	/** @var \fastImageSize\fastImageSize */
+	protected $imagesize;
+
 	/** @var language Language class */
 	protected $language;
 
@@ -97,11 +100,12 @@ class filespec
 	 * @param \phpbb\mimetype\guesser	$mimetype_guesser Mime type guesser
 	 * @param \phpbb\plupload\plupload	$plupload Plupload
 	 */
-	public function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, \bantu\IniGetWrapper\IniGetWrapper $php_ini, $phpbb_root_path, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
+	public function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, \bantu\IniGetWrapper\IniGetWrapper $php_ini, \fastImageSize\fastImageSize $imagesize, $phpbb_root_path, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
 	{
 		$this->filesystem = $phpbb_filesystem;
 		$this->language = $language;
 		$this->php_ini = $php_ini;
+		$this->imagesize = $imagesize;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->plupload = $plupload;
 		$this->mimetype_guesser = $mimetype_guesser;
@@ -500,10 +504,7 @@ class filespec
 		{
 			$this->width = $this->height = 0;
 
-			// Get imagesize class
-			$imagesize = new \fastImageSize\fastImageSize();
-
-			$this->image_info = $imagesize->getImageSize($this->destination_file, $this->mimetype);
+			$this->image_info = $this->imagesize->getImageSize($this->destination_file, $this->mimetype);
 
 			if ($this->image_info !== false)
 			{

tests/files/upload_test.php

@@ -59,6 +59,7 @@ class phpbb_files_upload_test extends phpbb_test_case
 			$this->filesystem,
 			$this->language,
 			$this->php_ini,
+			new \fastImageSize\fastImageSize(),
 			$phpbb_root_path,
 			new \phpbb\mimetype\guesser(array(
 				'mimetype.extension_guesser' => new \phpbb\mimetype\extension_guesser(),

tests/functional/fileupload_remote_test.php

@@ -57,7 +57,7 @@ class phpbb_functional_fileupload_remote_test extends phpbb_functional_test_case
 		$this->php_ini = new \bantu\IniGetWrapper\IniGetWrapper;
 
 		$container = new phpbb_mock_container_builder();
-		$container->set('files.filespec', new \phpbb\files\filespec($this->filesystem, $this->language, $this->php_ini, $this->phpbb_root_path));
+		$container->set('files.filespec', new \phpbb\files\filespec($this->filesystem, $this->language, $this->php_ini, new \fastImageSize\fastImageSize(), $this->phpbb_root_path));
 		$this->factory = new \phpbb\files\factory($container);
 		$container->set('files.factory', $this->factory);
 		$container->set('files.types.remote', new \phpbb\files\types\remote($this->factory, $this->language, $this->php_ini, $this->request, $phpbb_root_path));

tests/upload/filespec_test.php

@@ -93,7 +93,7 @@ class phpbb_filespec_test extends phpbb_test_case
 			'error' => '',
 		);
 
-		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, $this->phpbb_root_path, $this->mimetype_guesser);
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, new \fastImageSize\fastImageSize(), $this->phpbb_root_path, $this->mimetype_guesser);
 		return $filespec->set_upload_ary(array_merge($upload_ary, $override));
 	}
 
@@ -114,7 +114,7 @@ class phpbb_filespec_test extends phpbb_test_case
 
 	public function test_empty_upload_ary()
 	{
-		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, $this->phpbb_root_path, $this->mimetype_guesser);
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, new \fastImageSize\fastImageSize(), $this->phpbb_root_path, $this->mimetype_guesser);
 		$this->assertInstanceOf('\phpbb\files\filespec', $filespec->set_upload_ary(array()));
 		$this->assertTrue($filespec->init_error());
 	}
@@ -262,7 +262,7 @@ class phpbb_filespec_test extends phpbb_test_case
 	 */
 	public function test_clean_filename_avatar($filename, $expected, $mode, $prefix = '', $user_id = '')
 	{
-		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, $this->phpbb_root_path, $this->mimetype_guesser);
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, new \fastImageSize\fastImageSize(), $this->phpbb_root_path, $this->mimetype_guesser);
 
 		if ($filename)
 		{
@@ -403,6 +403,117 @@ class phpbb_filespec_test extends phpbb_test_case
 		$this->assertFalse($filespec->move_file('foo'));
 	}
 
+	public function data_move_file_copy()
+	{
+		return array(
+			array('gif_copy', true, false, array()),
+			array('gif_copy', true, true, array()),
+			array('foo_bar', false, false, array('GENERAL_UPLOAD_ERROR')),
+			array('foo_bar', false, true, array('GENERAL_UPLOAD_ERROR')),
+		);
+	}
+
+	/**
+	 * @dataProvider data_move_file_copy
+	 */
+	public function test_move_file_copy($tmp_name, $move_success, $safe_mode_on, $expected_error)
+	{
+		// Initialise a blank filespec object for use with trivial methods
+		$upload_ary = array(
+			'name' => 'gif_moved',
+			'type' => 'image/gif',
+			'size' => '',
+			'tmp_name' => $this->path . 'copies/' . $tmp_name,
+			'error' => '',
+		);
+
+		$php_ini = $this->getMockBuilder('\bantu\IniGetWrapper\IniGetWrapper')
+			->getMock();
+		$php_ini->expects($this->any())
+			->method('getBool')
+			->with($this->anything())
+			->willReturn($safe_mode_on);
+		$upload = new phpbb_mock_fileupload();
+		$upload->max_filesize = self::UPLOAD_MAX_FILESIZE;
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, $php_ini, new \fastImageSize\fastImagesize,  '', $this->mimetype_guesser);
+		$filespec->set_upload_ary($upload_ary);
+		$filespec->local = false;
+		$filespec->extension = 'gif';
+		$filespec->set_upload_namespace($upload);
+
+		$this->assertEquals($move_success, $filespec->move_file($this->path . 'copies'));
+		$this->assertEquals($filespec->file_moved, file_exists($this->path . 'copies/gif_moved'));
+		$this->assertSame($expected_error, $filespec->error);
+	}
+
+	public function data_move_file_imagesize()
+	{
+		return array(
+			array(
+				array(
+					'width'		=> 2,
+					'height'	=> 2,
+					'type'		=> IMAGETYPE_GIF,
+				),
+				array()
+			),
+			array(
+				array(
+					'width'		=> 2,
+					'height'	=> 2,
+					'type'		=> -1,
+				),
+				array('Image file type -1 for mimetype image/gif not supported.')
+			),
+			array(
+				array(
+					'width'		=> 0,
+					'height'	=> 0,
+					'type'		=> IMAGETYPE_GIF,
+				),
+				array('The image file you tried to attach is invalid.')
+			),
+			array(
+				false,
+				array('It was not possible to determine the dimensions of the image. Please verify that the URL you entered is correct.')
+			)
+		);
+	}
+
+	/**
+	 * @dataProvider data_move_file_imagesize
+	 */
+	public function test_move_file_imagesize($imagesize_return, $expected_error)
+	{
+		// Initialise a blank filespec object for use with trivial methods
+		$upload_ary = array(
+			'name' => 'gif_moved',
+			'type' => 'image/gif',
+			'size' => '',
+			'tmp_name' => $this->path . 'copies/gif_copy',
+			'error' => '',
+		);
+
+		$imagesize = $this->getMockBuilder('\fastImageSize\fastImageSize')
+			->getMock();
+		$imagesize->expects($this->any())
+			->method('getImageSize')
+			->with($this->anything())
+			->willReturn($imagesize_return);
+
+		$upload = new phpbb_mock_fileupload();
+		$upload->max_filesize = self::UPLOAD_MAX_FILESIZE;
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, $imagesize,  '', $this->mimetype_guesser);
+		$filespec->set_upload_ary($upload_ary);
+		$filespec->local = false;
+		$filespec->extension = 'gif';
+		$filespec->set_upload_namespace($upload);
+
+		$this->assertEquals(true, $filespec->move_file($this->path . 'copies'));
+		$this->assertEquals($filespec->file_moved, file_exists($this->path . 'copies/gif_moved'));
+		$this->assertSame($expected_error, $filespec->error);
+	}
+
 	/**
 	* @dataProvider clean_filename_variables
 	*/
@@ -419,7 +530,7 @@ class phpbb_filespec_test extends phpbb_test_case
 
 	public function test_is_uploaded()
 	{
-		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, $this->phpbb_root_path, null);
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, new \bantu\IniGetWrapper\IniGetWrapper, new \fastImageSize\fastImageSize(), $this->phpbb_root_path, null);
 		$reflection_filespec = new ReflectionClass($filespec);
 		$plupload_property = $reflection_filespec->getProperty('plupload');
 		$plupload_property->setAccessible(true);

tests/upload/fileupload_test.php

@@ -72,6 +72,7 @@ class phpbb_fileupload_test extends phpbb_test_case
 			$this->filesystem,
 			$this->language,
 			$this->php_ini,
+			new \fastImageSize\fastImageSize(),
 			$phpbb_root_path,
 			new \phpbb\mimetype\guesser(array(
 				'mimetype.extension_guesser' => new \phpbb\mimetype\extension_guesser(),
@@ -131,7 +132,7 @@ class phpbb_fileupload_test extends phpbb_test_case
 		$upload = new \phpbb\files\upload($this->filesystem, $this->factory, $this->language, $this->php_ini, $this->request, $this->phpbb_root_path);
 		$upload->set_allowed_extensions(array('jpg'))
 			->set_max_filesize(1000);
-		$file = new \phpbb\files\filespec($this->filesystem, $this->language, $this->php_ini, $this->phpbb_root_path);
+		$file = new \phpbb\files\filespec($this->filesystem, $this->language, $this->php_ini, new \fastImageSize\fastImageSize(), $this->phpbb_root_path);
 		$file->set_upload_ary(array(
 				'size'	=> 50,
 				'tmp_name'	=> dirname(__FILE__) . '/fixture/disallowed',