From a080173010c60fdc5b143c8341d18fcbb195f889 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 18:05:39 +0100 Subject: [PATCH 1/6] [ticket/13358] Add file_downloader as replacement for get_remote_file PHPBB3-13358 --- phpBB/config/services.yml | 5 ++ phpBB/phpbb/file_downloader.php | 137 ++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 phpBB/phpbb/file_downloader.php diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 975f2f7580..88f010751a 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -109,6 +109,11 @@ services: filesystem: class: phpbb\filesystem + file_downloader: + class: phpbb\file_downloader + arguments: + - @user + http_kernel: class: Symfony\Component\HttpKernel\HttpKernel arguments: diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php new file mode 100644 index 0000000000..0f33ae9941 --- /dev/null +++ b/phpBB/phpbb/file_downloader.php @@ -0,0 +1,137 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb; + +class file_downloader +{ + /** @var \phpbb\user */ + protected $user; + + /** @var string Error string */ + public $error_string = ''; + + /** @var int Error number */ + public $error_number = 0; + + /** + * Constructor + * + * @param \phpbb\user $user phpBB user object + */ + public function __construct(user $user) + { + $this->user = $user; + } + + /** + * Retrieve contents from remotely stored file + * + * @param string $host File host + * @param string $directory Directory file is in + * @param string $filename Filename of file to retrieve + * @param int $port Port to connect to; default: 80 + * @param int $timeout Connection timeout in seconds; default: 6 + * + * @return mixed File data as string if file can be read and there is no + * timeout, false if there were errors or the connection timed out + */ + function get($host, $directory, $filename, $port = 80, $timeout = 6) + { + if ($socket = @fsockopen($host, $port, $this->error_number, $this->error_string, $timeout)) + { + @fputs($socket, "GET $directory/$filename HTTP/1.0\r\n"); + @fputs($socket, "HOST: $host\r\n"); + @fputs($socket, "Connection: close\r\n\r\n"); + + $timer_stop = time() + $timeout; + stream_set_timeout($socket, $timeout); + + $file_info = ''; + $get_info = false; + + while (!@feof($socket)) + { + if ($get_info) + { + $file_info .= @fread($socket, 1024); + } + else + { + $line = @fgets($socket, 1024); + if ($line == "\r\n") + { + $get_info = true; + } + else if (stripos($line, '404 not found') !== false) + { + $this->error_string = $this->user->lang('FILE_NOT_FOUND', $filename); + return false; + } + } + + $stream_meta_data = stream_get_meta_data($socket); + + if (!empty($stream_meta_data['timed_out']) || time() >= $timer_stop) + { + $this->error_string = $this->user->lang['FSOCK_TIMEOUT']; + return false; + } + } + @fclose($socket); + } + else + { + if ($this->error_string) + { + $this->error_string = utf8_convert_message($this->error_string); + return false; + } + else + { + $this->error_string = $this->user->lang['FSOCK_DISABLED']; + return false; + } + } + + return $file_info; + } + + /** + * Set error string + * + * @param string $error_string Error string + * + * @return self + */ + public function set_error_string(&$error_string) + { + $this->error_string = &$error_string; + + return $this; + } + + /** + * Set error number + * + * @param int $error_number Error number + * + * @return self + */ + public function set_error_number(&$error_number) + { + $this->error_number = &$error_number; + + return $this; + } +} From 2793f9c078272b178250ed4e0812219b9c5c1676 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 18:07:36 +0100 Subject: [PATCH 2/6] [ticket/13358] Add file_downloader to version_helper PHPBB3-13358 --- phpBB/config/services.yml | 1 + phpBB/phpbb/version_helper.php | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 88f010751a..8a58a078f8 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -186,4 +186,5 @@ services: arguments: - @cache - @config + - @file_downloader - @user diff --git a/phpBB/phpbb/version_helper.php b/phpBB/phpbb/version_helper.php index c3c3602944..3b455ec5ba 100644 --- a/phpBB/phpbb/version_helper.php +++ b/phpBB/phpbb/version_helper.php @@ -50,6 +50,9 @@ class version_helper /** @var \phpbb\config\config */ protected $config; + /** @var \phpbb\file_downloader */ + protected $file_downloader; + /** @var \phpbb\user */ protected $user; @@ -58,12 +61,14 @@ class version_helper * * @param \phpbb\cache\service $cache * @param \phpbb\config\config $config + * @param \phpbb\file_downloader $file_downloader * @param \phpbb\user $user */ - public function __construct(\phpbb\cache\service $cache, \phpbb\config\config $config, \phpbb\user $user) + public function __construct(\phpbb\cache\service $cache, \phpbb\config\config $config, file_downloader $file_downloader, \phpbb\user $user) { $this->cache = $cache; $this->config = $config; + $this->file_downloader = $file_downloader; $this->user = $user; if (defined('PHPBB_QA')) @@ -250,7 +255,9 @@ class version_helper else if ($info === false || $force_update) { $errstr = $errno = ''; - $info = get_remote_file($this->host, $this->path, $this->file, $errstr, $errno); + $this->file_downloader->set_error_number($errno) + ->set_error_string($errstr); + $info = $this->file_downloader->get($this->host, $this->path, $this->file); if (!empty($errstr)) { From f6e7a94bd55c1c3b7a8aaed370a728c58ac34ea6 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 18:08:22 +0100 Subject: [PATCH 3/6] [ticket/13358] Use file_downloader in get_remote_file() get_remote_file() has been marked as deprecated. PHPBB3-13358 --- phpBB/includes/functions_admin.php | 65 ++++-------------------------- 1 file changed, 8 insertions(+), 57 deletions(-) diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index 5ddaf31cf5..6f95bc13bd 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -2982,68 +2982,19 @@ function get_database_size() /** * Retrieve contents from remotely stored file +* +* @deprecated 3.1.2 Use file_downloader instead */ function get_remote_file($host, $directory, $filename, &$errstr, &$errno, $port = 80, $timeout = 6) { - global $user; + global $phpbb_container; - if ($fsock = @fsockopen($host, $port, $errno, $errstr, $timeout)) - { - @fputs($fsock, "GET $directory/$filename HTTP/1.0\r\n"); - @fputs($fsock, "HOST: $host\r\n"); - @fputs($fsock, "Connection: close\r\n\r\n"); + // Get file downloader and assign $errstr and $errno + $file_downloader = $phpbb_container->get('file_downloader') + ->set_error_string($errstr) + ->set_error_number($errno); - $timer_stop = time() + $timeout; - stream_set_timeout($fsock, $timeout); - - $file_info = ''; - $get_info = false; - - while (!@feof($fsock)) - { - if ($get_info) - { - $file_info .= @fread($fsock, 1024); - } - else - { - $line = @fgets($fsock, 1024); - if ($line == "\r\n") - { - $get_info = true; - } - else if (stripos($line, '404 not found') !== false) - { - $errstr = $user->lang('FILE_NOT_FOUND', $filename); - return false; - } - } - - $stream_meta_data = stream_get_meta_data($fsock); - - if (!empty($stream_meta_data['timed_out']) || time() >= $timer_stop) - { - $errstr = $user->lang['FSOCK_TIMEOUT']; - return false; - } - } - @fclose($fsock); - } - else - { - if ($errstr) - { - $errstr = utf8_convert_message($errstr); - return false; - } - else - { - $errstr = $user->lang['FSOCK_DISABLED']; - return false; - } - } - - return $file_info; + return $file_downloader->get($host, $directory, $filename, $port, $timeout); } /* From 352648f173e7b132544bf3eaa494184bec6d5aa2 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 21:34:02 +0100 Subject: [PATCH 4/6] [ticket/13358] Fix tests and use exceptions instead of user object PHPBB3-13358 --- phpBB/config/services.yml | 2 -- phpBB/phpbb/file_downloader.php | 25 ++++++------------------- phpBB/phpbb/version_helper.php | 8 +++++++- tests/version/version_fetch_test.php | 1 + tests/version/version_test.php | 3 +++ 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 8a58a078f8..8667cbbf84 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -111,8 +111,6 @@ services: file_downloader: class: phpbb\file_downloader - arguments: - - @user http_kernel: class: Symfony\Component\HttpKernel\HttpKernel diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index 0f33ae9941..2d5d9a7516 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -15,25 +15,12 @@ namespace phpbb; class file_downloader { - /** @var \phpbb\user */ - protected $user; - /** @var string Error string */ public $error_string = ''; /** @var int Error number */ public $error_number = 0; - /** - * Constructor - * - * @param \phpbb\user $user phpBB user object - */ - public function __construct(user $user) - { - $this->user = $user; - } - /** * Retrieve contents from remotely stored file * @@ -45,6 +32,9 @@ class file_downloader * * @return mixed File data as string if file can be read and there is no * timeout, false if there were errors or the connection timed out + * + * @throws \RuntimeException If data can't be retrieved and no error + * message is returned */ function get($host, $directory, $filename, $port = 80, $timeout = 6) { @@ -75,8 +65,7 @@ class file_downloader } else if (stripos($line, '404 not found') !== false) { - $this->error_string = $this->user->lang('FILE_NOT_FOUND', $filename); - return false; + throw new \RuntimeException(array('FILE_NOT_FOUND', $filename)); } } @@ -84,8 +73,7 @@ class file_downloader if (!empty($stream_meta_data['timed_out']) || time() >= $timer_stop) { - $this->error_string = $this->user->lang['FSOCK_TIMEOUT']; - return false; + throw new \RuntimeException('FSOCK_TIMEOUT'); } } @fclose($socket); @@ -99,8 +87,7 @@ class file_downloader } else { - $this->error_string = $this->user->lang['FSOCK_DISABLED']; - return false; + throw new \RuntimeException('FSOCK_DISABLED'); } } diff --git a/phpBB/phpbb/version_helper.php b/phpBB/phpbb/version_helper.php index 3b455ec5ba..d7f1f02678 100644 --- a/phpBB/phpbb/version_helper.php +++ b/phpBB/phpbb/version_helper.php @@ -257,7 +257,13 @@ class version_helper $errstr = $errno = ''; $this->file_downloader->set_error_number($errno) ->set_error_string($errstr); - $info = $this->file_downloader->get($this->host, $this->path, $this->file); + try { + $info = $this->file_downloader->get($this->host, $this->path, $this->file); + } + catch (\RuntimeException $exception) + { + throw new \RuntimeException(call_user_func_array(array($this->user, 'lang'), $exception->getMessage())); + } if (!empty($errstr)) { diff --git a/tests/version/version_fetch_test.php b/tests/version/version_fetch_test.php index 05eac58a52..cfc87183cf 100644 --- a/tests/version/version_fetch_test.php +++ b/tests/version/version_fetch_test.php @@ -33,6 +33,7 @@ class phpbb_version_helper_fetch_test extends phpbb_test_case new \phpbb\config\config(array( 'version' => '3.1.0', )), + new \phpbb\file_downloader(), new \phpbb\user('\phpbb\datetime') ); } diff --git a/tests/version/version_test.php b/tests/version/version_test.php index ba31c79a79..528f1602d6 100644 --- a/tests/version/version_test.php +++ b/tests/version/version_test.php @@ -30,6 +30,7 @@ class phpbb_version_helper_test extends phpbb_test_case new \phpbb\config\config(array( 'version' => '3.1.0', )), + new \phpbb\file_downloader(), new \phpbb\user('\phpbb\datetime') ); } @@ -208,6 +209,7 @@ class phpbb_version_helper_test extends phpbb_test_case new \phpbb\config\config(array( 'version' => $current_version, )), + new \phpbb\file_downloader(), new \phpbb\user('\phpbb\datetime'), )) ->getMock() @@ -318,6 +320,7 @@ class phpbb_version_helper_test extends phpbb_test_case new \phpbb\config\config(array( 'version' => $current_version, )), + new \phpbb\file_downloader(), new \phpbb\user('\phpbb\datetime'), )) ->getMock() From 171837eefe2f8e17597629b108e2aa30c0f4055f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 23:16:22 +0100 Subject: [PATCH 5/6] [ticket/13358] Do not pass variables by reference PHPBB3-13358 --- phpBB/includes/functions_admin.php | 10 ++++++---- phpBB/phpbb/file_downloader.php | 28 ++++++++++++---------------- phpBB/phpbb/version_helper.php | 8 +++----- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index 6f95bc13bd..0b9ea23fe7 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -2990,11 +2990,13 @@ function get_remote_file($host, $directory, $filename, &$errstr, &$errno, $port global $phpbb_container; // Get file downloader and assign $errstr and $errno - $file_downloader = $phpbb_container->get('file_downloader') - ->set_error_string($errstr) - ->set_error_number($errno); + $file_downloader = $phpbb_container->get('file_downloader'); - return $file_downloader->get($host, $directory, $filename, $port, $timeout); + $file_data = $file_downloader->get($host, $directory, $filename, $port, $timeout); + $errstr = $file_downloader->get_error_string(); + $errno = $file_downloader->get_error_number(); + + return $file_data; } /* diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index 2d5d9a7516..2a2b8dbab5 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -38,6 +38,10 @@ class file_downloader */ function get($host, $directory, $filename, $port = 80, $timeout = 6) { + // Set default values for error variables + $this->error_number = 0; + $this->error_string = ''; + if ($socket = @fsockopen($host, $port, $this->error_number, $this->error_string, $timeout)) { @fputs($socket, "GET $directory/$filename HTTP/1.0\r\n"); @@ -95,30 +99,22 @@ class file_downloader } /** - * Set error string + * Get error string * - * @param string $error_string Error string - * - * @return self + * @return string Error string */ - public function set_error_string(&$error_string) + public function get_error_string() { - $this->error_string = &$error_string; - - return $this; + return $this->error_string; } /** - * Set error number + * Get error number * - * @param int $error_number Error number - * - * @return self + * @return int Error number */ - public function set_error_number(&$error_number) + public function get_error_number() { - $this->error_number = &$error_number; - - return $this; + return $this->error_number; } } diff --git a/phpBB/phpbb/version_helper.php b/phpBB/phpbb/version_helper.php index d7f1f02678..38050d8ad7 100644 --- a/phpBB/phpbb/version_helper.php +++ b/phpBB/phpbb/version_helper.php @@ -254,9 +254,6 @@ class version_helper } else if ($info === false || $force_update) { - $errstr = $errno = ''; - $this->file_downloader->set_error_number($errno) - ->set_error_string($errstr); try { $info = $this->file_downloader->get($this->host, $this->path, $this->file); } @@ -264,10 +261,11 @@ class version_helper { throw new \RuntimeException(call_user_func_array(array($this->user, 'lang'), $exception->getMessage())); } + $error_string = $this->file_downloader->get_error_string(); - if (!empty($errstr)) + if (!empty($error_string)) { - throw new \RuntimeException($errstr); + throw new \RuntimeException($error_string); } $info = json_decode($info, true); From 6d3ac29aeef9e1222ddcaeab066ca8325d147fee Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 21 Nov 2014 23:30:47 +0100 Subject: [PATCH 6/6] [ticket/13358] Use protected and public keywords where applicable PHPBB3-13358 --- phpBB/phpbb/file_downloader.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/file_downloader.php b/phpBB/phpbb/file_downloader.php index 2a2b8dbab5..d717b394d5 100644 --- a/phpBB/phpbb/file_downloader.php +++ b/phpBB/phpbb/file_downloader.php @@ -16,10 +16,10 @@ namespace phpbb; class file_downloader { /** @var string Error string */ - public $error_string = ''; + protected $error_string = ''; /** @var int Error number */ - public $error_number = 0; + protected $error_number = 0; /** * Retrieve contents from remotely stored file @@ -36,7 +36,7 @@ class file_downloader * @throws \RuntimeException If data can't be retrieved and no error * message is returned */ - function get($host, $directory, $filename, $port = 80, $timeout = 6) + public function get($host, $directory, $filename, $port = 80, $timeout = 6) { // Set default values for error variables $this->error_number = 0;