From 7f61f8b770f58f269336480b260cf88925c6d734 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 26 Dec 2022 16:43:50 +0100 Subject: [PATCH] [ticket/16955] Clean up filesystem and files classes PHPBB3-16955 --- phpBB/phpbb/file_downloader.php | 2 +- phpBB/phpbb/files/factory.php | 2 +- phpBB/phpbb/files/filespec.php | 2 +- phpBB/phpbb/files/filespec_storage.php | 8 +++--- phpBB/phpbb/files/types/base.php | 14 +++++++--- phpBB/phpbb/files/types/form.php | 8 +++--- phpBB/phpbb/files/types/form_storage.php | 8 +++--- phpBB/phpbb/files/types/local.php | 4 +-- phpBB/phpbb/files/types/local_storage.php | 11 ++++---- phpBB/phpbb/files/types/type_interface.php | 2 +- phpBB/phpbb/files/upload.php | 14 +++++----- phpBB/phpbb/filesystem/filesystem.php | 26 +++++++++---------- .../phpbb/filesystem/filesystem_interface.php | 2 +- phpBB/phpbb/filesystem/helper.php | 4 +-- 14 files changed, 58 insertions(+), 49 deletions(-) diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index f55b825d2d..75721b0f05 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -77,7 +77,7 @@ class file_downloader $stream_meta_data = stream_get_meta_data($socket); - if (!empty($stream_meta_data['timed_out']) || time() >= $timer_stop) + if ($stream_meta_data['timed_out'] || time() >= $timer_stop) { throw new \phpbb\exception\runtime_exception('FSOCK_TIMEOUT'); } diff --git a/phpBB/phpbb/files/factory.php b/phpBB/phpbb/files/factory.php index 84b7cc9449..6f253d548f 100644 --- a/phpBB/phpbb/files/factory.php +++ b/phpBB/phpbb/files/factory.php @@ -46,7 +46,7 @@ class factory try { - $service = $this->container->get($name); + $service = $this->container->get($name) ?? false; } catch (\Exception $e) { diff --git a/phpBB/phpbb/files/filespec.php b/phpBB/phpbb/files/filespec.php index d4f523bf27..1ebfdd7cf5 100644 --- a/phpBB/phpbb/files/filespec.php +++ b/phpBB/phpbb/files/filespec.php @@ -329,7 +329,7 @@ class filespec * Get mime type * * @param string $filename Filename that needs to be checked - * @return string Mime type of supplied filename + * @return string|false Mime type of supplied filename or false if mimetype could not be guessed */ public function get_mimetype($filename) { diff --git a/phpBB/phpbb/files/filespec_storage.php b/phpBB/phpbb/files/filespec_storage.php index 72285305a8..a241f6f949 100644 --- a/phpBB/phpbb/files/filespec_storage.php +++ b/phpBB/phpbb/files/filespec_storage.php @@ -99,7 +99,7 @@ class filespec_storage * * @param array $upload_ary Upload ary * - * @return filespec This instance of the filespec class + * @return filespec_storage This instance of the filespec class */ public function set_upload_ary($upload_ary) { @@ -142,7 +142,7 @@ class filespec_storage * * @param upload $namespace Instance of upload class * - * @return filespec This instance of the filespec class + * @return filespec_storage This instance of the filespec class */ public function set_upload_namespace($namespace) { @@ -166,7 +166,7 @@ class filespec_storage * * @param mixed $error Content for error array * - * @return \phpbb\files\filespec This instance of the filespec class + * @return filespec_storage This instance of the filespec class */ public function set_error($error) { @@ -313,7 +313,7 @@ class filespec_storage * Get mime type * * @param string $filename Filename that needs to be checked - * @return string Mime type of supplied filename + * @return string|false Mime type of supplied filename or false if mimetype could not be guessed */ public function get_mimetype($filename) { diff --git a/phpBB/phpbb/files/types/base.php b/phpBB/phpbb/files/types/base.php index 3313ad040b..eb00b07672 100644 --- a/phpBB/phpbb/files/types/base.php +++ b/phpBB/phpbb/files/types/base.php @@ -27,9 +27,10 @@ abstract class base implements type_interface /** * Check if upload exceeds maximum file size * - * @param \phpbb\files\filespec $file Filespec object + * @template filespec_type of \phpbb\files\filespec|\phpbb\files\filespec_storage + * @param filespec_type $file Filespec object * - * @return \phpbb\files\filespec Returns same filespec instance + * @return filespec_type Returns same filespec instance */ public function check_upload_size($file) { @@ -47,7 +48,14 @@ abstract class base implements type_interface $unit = ($unit == 'k') ? 'KB' : (($unit == 'g') ? 'GB' : 'MB'); } - $file->error[] = (empty($max_filesize)) ? $this->language->lang($this->upload->error_prefix . 'PHP_SIZE_NA') : $this->language->lang($this->upload->error_prefix . 'PHP_SIZE_OVERRUN', $max_filesize, $this->language->lang($unit)); + if (empty($max_filesize)) + { + $file->error[] = $this->language->lang($this->upload->error_prefix . 'PHP_SIZE_NA'); + } + else + { + $file->error[] = $this->language->lang($this->upload->error_prefix . 'PHP_SIZE_OVERRUN', $max_filesize, $this->language->lang($unit)); + } } return $file; diff --git a/phpBB/phpbb/files/types/form.php b/phpBB/phpbb/files/types/form.php index a915476191..88d0fab9ad 100644 --- a/phpBB/phpbb/files/types/form.php +++ b/phpBB/phpbb/files/types/form.php @@ -18,7 +18,7 @@ use phpbb\files\factory; use phpbb\files\filespec; use phpbb\language\language; use phpbb\plupload\plupload; -use phpbb\request\request_interface; +use phpbb\request\request; class form extends base { @@ -28,7 +28,7 @@ class form extends base /** @var plupload */ protected $plupload; - /** @var request_interface */ + /** @var request */ protected $request; /** @@ -38,9 +38,9 @@ class form extends base * @param language $language Language class * @param IniGetWrapper $php_ini ini_get() wrapper * @param plupload $plupload Plupload - * @param request_interface $request Request object + * @param request $request Request object */ - public function __construct(factory $factory, language $language, IniGetWrapper $php_ini, plupload $plupload, request_interface $request) + public function __construct(factory $factory, language $language, IniGetWrapper $php_ini, plupload $plupload, request $request) { $this->factory = $factory; $this->language = $language; diff --git a/phpBB/phpbb/files/types/form_storage.php b/phpBB/phpbb/files/types/form_storage.php index 3024dc83e7..03b9f56119 100644 --- a/phpBB/phpbb/files/types/form_storage.php +++ b/phpBB/phpbb/files/types/form_storage.php @@ -18,7 +18,7 @@ use phpbb\files\factory; use phpbb\files\filespec; use phpbb\language\language; use phpbb\plupload\plupload; -use phpbb\request\request_interface; +use phpbb\request\request; class form_storage extends base { @@ -28,7 +28,7 @@ class form_storage extends base /** @var plupload */ protected $plupload; - /** @var request_interface */ + /** @var request */ protected $request; /** @@ -38,9 +38,9 @@ class form_storage extends base * @param language $language Language class * @param IniGetWrapper $php_ini ini_get() wrapper * @param plupload $plupload Plupload - * @param request_interface $request Request object + * @param request $request Request object */ - public function __construct(factory $factory, language $language, IniGetWrapper $php_ini, plupload $plupload, request_interface $request) + public function __construct(factory $factory, language $language, IniGetWrapper $php_ini, plupload $plupload, request $request) { $this->factory = $factory; $this->language = $language; diff --git a/phpBB/phpbb/files/types/local.php b/phpBB/phpbb/files/types/local.php index ca526c1a01..7bcac0bae0 100644 --- a/phpBB/phpbb/files/types/local.php +++ b/phpBB/phpbb/files/types/local.php @@ -65,8 +65,8 @@ class local extends base $upload = $this->get_upload_ary($source_file, $filedata); /** @var filespec $file */ - $file = $this->factory->get('filespec') - ->set_upload_ary($upload) + $file = $this->factory->get('filespec'); + $file->set_upload_ary($upload) ->set_upload_namespace($this->upload); if ($file->init_error()) diff --git a/phpBB/phpbb/files/types/local_storage.php b/phpBB/phpbb/files/types/local_storage.php index f68d51199e..4329eaf133 100644 --- a/phpBB/phpbb/files/types/local_storage.php +++ b/phpBB/phpbb/files/types/local_storage.php @@ -15,7 +15,7 @@ namespace phpbb\files\types; use bantu\IniGetWrapper\IniGetWrapper; use phpbb\files\factory; -use phpbb\files\filespec; +use phpbb\files\filespec_storage; use phpbb\language\language; use phpbb\request\request_interface; @@ -67,15 +67,15 @@ class local_storage extends base * @param string $source_file Filename of source file * @param array|bool $filedata Array with filedata or false * - * @return filespec Object "filespec" is returned, all further operations can be done with this object + * @return filespec_storage Object "filespec_storage" is returned, all further operations can be done with this object */ protected function local_upload($source_file, $filedata = false) { $upload = $this->get_upload_ary($source_file, $filedata); - /** @var filespec $file */ - $file = $this->factory->get('filespec_storage') - ->set_upload_ary($upload) + /** @var filespec_storage $file */ + $file = $this->factory->get('filespec_storage'); + $file->set_upload_ary($upload) ->set_upload_namespace($this->upload); if ($file->init_error()) @@ -85,6 +85,7 @@ class local_storage extends base } // PHP Upload file size check + /** @var filespec_storage $file */ $file = $this->check_upload_size($file); if (count($file->error)) { diff --git a/phpBB/phpbb/files/types/type_interface.php b/phpBB/phpbb/files/types/type_interface.php index e07078349a..8a20a8eac7 100644 --- a/phpBB/phpbb/files/types/type_interface.php +++ b/phpBB/phpbb/files/types/type_interface.php @@ -21,7 +21,7 @@ interface type_interface * Handle upload for upload types. Arguments passed to this method will be * handled by the upload type classes themselves. * - * @return \phpbb\files\filespec|bool Filespec instance if upload is + * @return \phpbb\files\filespec_storage|\phpbb\files\filespec|bool Filespec instance if upload is * successful or false if not */ public function upload(); diff --git a/phpBB/phpbb/files/upload.php b/phpBB/phpbb/files/upload.php index 1577e67739..9623adec99 100644 --- a/phpBB/phpbb/files/upload.php +++ b/phpBB/phpbb/files/upload.php @@ -14,7 +14,7 @@ namespace phpbb\files; use phpbb\language\language; -use phpbb\request\request_interface; +use phpbb\request\request; /** * File upload class @@ -55,7 +55,7 @@ class upload /** @var language Language class */ protected $language; - /** @var request_interface Request class */ + /** @var request Request class */ protected $request; /** @@ -64,9 +64,9 @@ class upload * @param factory $factory Files factory * @param language $language Language class * @param \bantu\IniGetWrapper\IniGetWrapper $php_ini ini_get() wrapper - * @param request_interface $request Request class + * @param request $request Request class */ - public function __construct(factory $factory, language $language, \bantu\IniGetWrapper\IniGetWrapper $php_ini, request_interface $request) + public function __construct(factory $factory, language $language, \bantu\IniGetWrapper\IniGetWrapper $php_ini, request $request) { $this->factory = $factory; $this->language = $language; @@ -194,7 +194,7 @@ class upload * * @param string $errorcode Error code to assign * - * @return string Error string + * @return string|false Error string or false if error code is not supported * @access public */ public function assign_internal_error($errorcode) @@ -250,7 +250,7 @@ class upload /** * Perform common file checks * - * @param filespec_storage $file Instance of filespec class + * @param filespec_storage|filespec $file Instance of filespec class */ public function common_checks($file) { @@ -296,7 +296,7 @@ class upload /** * Check for allowed dimension * - * @param filespec_storage $file Instance of filespec class + * @param filespec_storage|filespec $file Instance of filespec class * * @return bool True if dimensions are valid or no constraints set, false * if not diff --git a/phpBB/phpbb/filesystem/filesystem.php b/phpBB/phpbb/filesystem/filesystem.php index 2931286811..67b25907b3 100644 --- a/phpBB/phpbb/filesystem/filesystem.php +++ b/phpBB/phpbb/filesystem/filesystem.php @@ -329,7 +329,7 @@ class filesystem implements filesystem_interface /** * {@inheritdoc} */ - public function phpbb_chmod($files, $perms = null, $recursive = false, $force_chmod_link = false) + public function phpbb_chmod($file, $perms = null, $recursive = false, $force_chmod_link = false) { if (is_null($perms)) { @@ -374,26 +374,26 @@ class filesystem implements filesystem_interface { try { - foreach ($this->to_iterator($files) as $file) + foreach ($this->to_iterator($file) as $current_file) { - $file_uid = @fileowner($file); - $file_gid = @filegroup($file); + $file_uid = @fileowner($current_file); + $file_gid = @filegroup($current_file); // Change owner if ($file_uid !== $this->chmod_info['common_owner']) { - $this->chown($file, $this->chmod_info['common_owner'], $recursive); + $this->chown($current_file, $this->chmod_info['common_owner'], $recursive); } // Change group if ($file_gid !== $this->chmod_info['common_group']) { - $this->chgrp($file, $this->chmod_info['common_group'], $recursive); + $this->chgrp($current_file, $this->chmod_info['common_group'], $recursive); } clearstatcache(); - $file_uid = @fileowner($file); - $file_gid = @filegroup($file); + $file_uid = @fileowner($current_file); + $file_gid = @filegroup($current_file); } } catch (filesystem_exception $e) @@ -431,9 +431,9 @@ class filesystem implements filesystem_interface case 'owner': try { - $this->chmod($files, $perms, $recursive, $force_chmod_link); + $this->chmod($file, $perms, $recursive, $force_chmod_link); clearstatcache(); - if ($this->is_readable($files) && $this->is_writable($files)) + if ($this->is_readable($file) && $this->is_writable($file)) { break; } @@ -445,9 +445,9 @@ class filesystem implements filesystem_interface case 'group': try { - $this->chmod($files, $perms, $recursive, $force_chmod_link); + $this->chmod($file, $perms, $recursive, $force_chmod_link); clearstatcache(); - if ((!($perms & self::CHMOD_READ) || $this->is_readable($files, $recursive)) && (!($perms & self::CHMOD_WRITE) || $this->is_writable($files, $recursive))) + if ((!($perms & self::CHMOD_READ) || $this->is_readable($file, $recursive)) && (!($perms & self::CHMOD_WRITE) || $this->is_writable($file, $recursive))) { break; } @@ -458,7 +458,7 @@ class filesystem implements filesystem_interface } case 'other': default: - $this->chmod($files, $perms, $recursive, $force_chmod_link); + $this->chmod($file, $perms, $recursive, $force_chmod_link); break; } } diff --git a/phpBB/phpbb/filesystem/filesystem_interface.php b/phpBB/phpbb/filesystem/filesystem_interface.php index fa7950dec3..0e7967dcfb 100644 --- a/phpBB/phpbb/filesystem/filesystem_interface.php +++ b/phpBB/phpbb/filesystem/filesystem_interface.php @@ -241,7 +241,7 @@ interface filesystem_interface * * @param ?string $path Path to resolve * - * @return string Resolved path + * @return string|false Resolved path or false if path could not be resolved */ public function realpath($path); diff --git a/phpBB/phpbb/filesystem/helper.php b/phpBB/phpbb/filesystem/helper.php index 0843e077ad..9d1c19057e 100644 --- a/phpBB/phpbb/filesystem/helper.php +++ b/phpBB/phpbb/filesystem/helper.php @@ -69,7 +69,7 @@ class helper * Try to resolve real path when PHP's realpath failes to do so * * @param string $path - * @return bool|string + * @return string|false */ protected static function phpbb_own_realpath($path) { @@ -175,7 +175,7 @@ class helper * * @param string $path Path to resolve * - * @return string Resolved path + * @return string|false Resolved path or false if path could not be resolved */ public static function realpath($path) {