[ticket/13904] Remove phpbb_root_path global from filespec class

PHPBB3-13904

phpBB/config/default/container/services_files.yml

@@ -13,6 +13,7 @@ services:
         arguments:
             - @filesystem
             - @language
+            - %core.root_path%
             - @mimetype.guesser
             - @plupload
 
@@ -24,3 +25,4 @@ services:
             - @files.factory
             - @language
             - @request
+            - %core.root_path%

phpBB/phpbb/files/filespec.php

@@ -86,20 +86,25 @@ class filespec
 	/** @var \phpbb\language\language Language class */
 	protected $language;
 
+	/** @var string phpBB root path */
+	protected $phpbb_root_path;
+
 	/**
 	 * File upload class
 	 *
-	 * @param \phpbb\filesystem\filesystem_interface	$phpbb_filesystem
+	 * @param \phpbb\filesystem\filesystem_interface	$phpbb_filesystem Filesystem
-	 * @param \phpbb\language\language					$language
+	 * @param \phpbb\language\language					$language Language
-	 * @param \phpbb\mimetype\guesser					$mimetype_guesser
+	 * @param string									$phpbb_root_path phpBB root path
-	 * @param \phpbb\plupload\plupload					$plupload
+	 * @param \phpbb\mimetype\guesser					$mimetype_guesser Mime type guesser
+	 * @param \phpbb\plupload\plupload					$plupload Plupload
 	 */
-	function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
+	function __construct(\phpbb\filesystem\filesystem_interface $phpbb_filesystem, language $language, $phpbb_root_path, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null)
 	{
 		$this->plupload = $plupload;
 		$this->mimetype_guesser = $mimetype_guesser;
 		$this->filesystem = $phpbb_filesystem;
 		$this->language = $language;
+		$this->phpbb_root_path = $phpbb_root_path;
 	}
 
 	/**
@@ -390,8 +395,6 @@ class filespec
 	 */
 	function move_file($destination, $overwrite = false, $skip_image_check = false, $chmod = false)
 	{
-		global $phpbb_root_path;
-
 		if (sizeof($this->error))
 		{
 			return false;
@@ -400,7 +403,7 @@ class filespec
 		$chmod = ($chmod === false) ? CHMOD_READ | CHMOD_WRITE : $chmod;
 
 		// We need to trust the admin in specifying valid upload directories and an attacker not being able to overwrite it...
-		$this->destination_path = $phpbb_root_path . $destination;
+		$this->destination_path = $this->phpbb_root_path . $destination;
 
 		// Check if the destination path exist...
 		if (!file_exists($this->destination_path))

tests/functional/fileupload_remote_test.php

@@ -53,7 +53,7 @@ class phpbb_functional_fileupload_remote_test extends phpbb_functional_test_case
 		$this->request = $this->getMock('\phpbb\request\request');
 
 		$container = new phpbb_mock_container_builder();
-		$container->set('files.filespec', new \phpbb\files\filespec($this->filesystem, $this->language));
+		$container->set('files.filespec', new \phpbb\files\filespec($this->filesystem, $this->language, $this->phpbb_root_path));
 		$this->factory = new \phpbb\files\factory($container);
 		$container->set('files.factory', $this->factory);
 		$this->phpbb_root_path = $phpbb_root_path;

tests/upload/filespec_test.php

@@ -28,6 +28,9 @@ class phpbb_filespec_test extends phpbb_test_case
 	/** @var \phpbb\language\language */
 	protected $language;
 
+	/** @var string phpBB root path */
+	protected $phpbb_root_path;
+
 	protected function setUp()
 	{
 		// Global $config required by unique_id
@@ -76,6 +79,7 @@ class phpbb_filespec_test extends phpbb_test_case
 		$this->language = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx));
 
 		$this->filesystem = new \phpbb\filesystem\filesystem();
+		$this->phpbb_root_path = $phpbb_root_path;
 	}
 
 	private function get_filespec($override = array())
@@ -89,7 +93,7 @@ class phpbb_filespec_test extends phpbb_test_case
 			'error' => '',
 		);
 
-		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, $this->mimetype_guesser);
+		$filespec = new \phpbb\files\filespec($this->filesystem, $this->language, $this->phpbb_root_path, $this->mimetype_guesser);
 		return $filespec->set_upload_ary(array_merge($upload_ary, $override));
 	}
 
@@ -297,8 +301,7 @@ class phpbb_filespec_test extends phpbb_test_case
 	{
 		// Global $phpbb_root_path and $phpEx are required by phpbb_chmod
 		global $phpbb_root_path, $phpEx;
-		$phpbb_root_path = '';
+		$this->phpbb_root_path = '';
-		$phpEx = 'php';
 
 		$upload = new phpbb_mock_fileupload();
 		$upload->max_filesize = self::UPLOAD_MAX_FILESIZE;
@@ -319,7 +322,7 @@ class phpbb_filespec_test extends phpbb_test_case
 			$this->assertEquals($error, $filespec->error[0]);
 		}
 
-		$phpEx = '';
+		$this->phpbb_root_path = $phpbb_root_path;
 	}
 
 	/**

tests/upload/fileupload_test.php

@@ -58,6 +58,7 @@ class phpbb_fileupload_test extends phpbb_test_case
 		$this->container->set('files.filespec', new \phpbb\files\filespec(
 			$this->filesystem,
 			$this->language,
+			$phpbb_root_path,
 			new \phpbb\mimetype\guesser(array(
 				'mimetype.extension_guesser' => new \phpbb\mimetype\extension_guesser(),
 			))));