From 948023078bdab21db27e680ec93357d77a4fe231 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 27 Dec 2022 14:13:23 +0100 Subject: [PATCH] [ticket/16955] Clean up code in db classes PHPBB3-16955 --- phpBB/phpbb/db/doctrine/oci8/connection.php | 2 +- phpBB/phpbb/db/doctrine/table_helper.php | 2 +- phpBB/phpbb/db/doctrine/type_converter.php | 6 +- phpBB/phpbb/db/driver/driver.php | 91 ++++++++++++++++++- phpBB/phpbb/db/driver/driver_interface.php | 8 +- phpBB/phpbb/db/driver/factory.php | 12 ++- phpBB/phpbb/db/driver/mssql_base.php | 19 +--- phpBB/phpbb/db/driver/mssql_odbc.php | 52 +++++------ phpBB/phpbb/db/driver/mssqlnative.php | 48 ++++------ phpBB/phpbb/db/driver/mysql_base.php | 35 ++----- phpBB/phpbb/db/driver/mysqli.php | 45 +++------ phpBB/phpbb/db/driver/oracle.php | 59 +++++------- phpBB/phpbb/db/driver/postgres.php | 74 ++++----------- phpBB/phpbb/db/driver/sqlite3.php | 61 ++++--------- phpBB/phpbb/db/extractor/factory.php | 17 ++-- phpBB/phpbb/db/extractor/mssql_extractor.php | 4 +- phpBB/phpbb/db/extractor/oracle_extractor.php | 6 +- .../data/v310/soft_delete_mod_convert.php | 4 +- .../data/v31x/remove_duplicate_migrations.php | 8 +- phpBB/phpbb/db/migration/tool/config.php | 8 +- phpBB/phpbb/db/migration/tool/config_text.php | 4 +- phpBB/phpbb/db/migration/tool/module.php | 2 +- phpBB/phpbb/db/migration/tool/permission.php | 19 ++-- .../db/migration/tool/tool_interface.php | 2 +- phpBB/phpbb/db/migrator.php | 13 +-- phpBB/phpbb/db/tools/doctrine.php | 3 +- 26 files changed, 277 insertions(+), 327 deletions(-) diff --git a/phpBB/phpbb/db/doctrine/oci8/connection.php b/phpBB/phpbb/db/doctrine/oci8/connection.php index 6f7f0aa3b9..34ed744e9d 100644 --- a/phpBB/phpbb/db/doctrine/oci8/connection.php +++ b/phpBB/phpbb/db/doctrine/oci8/connection.php @@ -68,7 +68,7 @@ class connection implements DriverConnection /** * {@inheritDoc} */ - public function lastInsertId($name = null): ?string + public function lastInsertId($name = null) { return $this->wrapped->lastInsertId($name); } diff --git a/phpBB/phpbb/db/doctrine/table_helper.php b/phpBB/phpbb/db/doctrine/table_helper.php index f6fde0fd2d..b14571c9f8 100644 --- a/phpBB/phpbb/db/doctrine/table_helper.php +++ b/phpBB/phpbb/db/doctrine/table_helper.php @@ -21,7 +21,7 @@ class table_helper * * @param array $column_data Column data. * - * @return array A pair of type and array of column options. + * @return array{string, array} A pair of type and array of column options. */ public static function convert_column_data(array $column_data, string $dbms_layer): array { diff --git a/phpBB/phpbb/db/doctrine/type_converter.php b/phpBB/phpbb/db/doctrine/type_converter.php index c03c3bd2c8..61da22eaf1 100644 --- a/phpBB/phpbb/db/doctrine/type_converter.php +++ b/phpBB/phpbb/db/doctrine/type_converter.php @@ -54,7 +54,7 @@ class type_converter * * @param string $type Legacy type name * - * @return array Pair of type name and options. + * @return array{string, array} Pair of type name and options. */ public static function convert(string $type, string $dbms): array { @@ -73,7 +73,7 @@ class type_converter * @param string $type Legacy type name. * @param int $length Type length. * - * @return array Pair of type name and options. + * @return array{string, array} Pair of type name and options. */ private static function mapWithLength(string $type, int $length): array { @@ -107,7 +107,7 @@ class type_converter * * @param string $type Type name. * - * @return array Pair of type name and an array of options. + * @return array{string, array} Pair of type name and an array of options. */ private static function mapType(string $type, string $dbms): array { diff --git a/phpBB/phpbb/db/driver/driver.php b/phpBB/phpbb/db/driver/driver.php index cd9f1f058e..a16c6260f4 100644 --- a/phpBB/phpbb/db/driver/driver.php +++ b/phpBB/phpbb/db/driver/driver.php @@ -31,6 +31,9 @@ abstract class driver implements driver_interface var $html_hold = ''; var $sql_report = ''; + /** @var string Last query text */ + protected $last_query_text = ''; + var $persistency = false; var $user = ''; var $server = ''; @@ -279,6 +282,13 @@ abstract class driver implements driver_interface return $result; } + /** + * Close sql connection + * + * @return bool False if failure + */ + abstract protected function _sql_close(): bool; + /** * {@inheritDoc} */ @@ -296,6 +306,18 @@ abstract class driver implements driver_interface return $this->_sql_query_limit($query, $total, $offset, $cache_ttl); } + /** + * Build LIMIT query + * + * @param string $query The SQL query to execute + * @param int $total The number of rows to select + * @param int $offset + * @param int $cache_ttl Either 0 to avoid caching or + * the time in seconds which the result shall be kept in cache + * @return mixed Buffered, seekable result handle, false on error + */ + abstract protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0); + /** * {@inheritDoc} */ @@ -404,6 +426,18 @@ abstract class driver implements driver_interface return $this->_sql_like_expression('LIKE \'' . $this->sql_escape($expression) . '\''); } + /** + * Build LIKE expression + * + * @param string $expression Base expression + * + * @return string LIKE expression + */ + protected function _sql_like_expression(string $expression): string + { + return $expression; + } + /** * {@inheritDoc} */ @@ -415,6 +449,18 @@ abstract class driver implements driver_interface return $this->_sql_not_like_expression('NOT LIKE \'' . $this->sql_escape($expression) . '\''); } + /** + * Build NOT LIKE expression + * + * @param string $expression Base expression + * + * @return string NOT LIKE expression + */ + protected function _sql_not_like_expression(string $expression): string + { + return $expression; + } + /** * {@inheritDoc} */ @@ -510,12 +556,22 @@ abstract class driver implements driver_interface return $result; } + /** + * SQL Transaction + * + * @param string $status Should be one of the following strings: + * begin, commit, rollback + * + * @return bool Success/failure of the transaction query + */ + abstract protected function _sql_transaction(string $status = 'begin'): bool; + /** * {@inheritDoc} */ - function sql_build_array($query, $assoc_ary = false) + function sql_build_array($query, $assoc_ary = []) { - if (!is_array($assoc_ary)) + if (!count($assoc_ary)) { return false; } @@ -836,6 +892,18 @@ abstract class driver implements driver_interface return $sql; } + /** + * Build db-specific query data + * + * @param string $stage Query stage, can be 'FROM' or 'WHERE' + * @param string|array $data A string containing the CROSS JOIN query or an array of WHERE clauses + * + * @return string|array The db-specific query fragment + */ + protected function _sql_custom_build(string $stage, $data) + { + return $data; + } protected function _process_boolean_tree_first($operations_ary) { @@ -1017,7 +1085,7 @@ abstract class driver implements driver_interface global $msg_long_text; $msg_long_text = $message; - trigger_error(false, E_USER_ERROR); + trigger_error('', E_USER_ERROR); } trigger_error($message, E_USER_ERROR); @@ -1031,6 +1099,13 @@ abstract class driver implements driver_interface return $this->sql_error_returned; } + /** + * Return sql error array + * + * @return array{message: string, code: int|string} SQL error array with message and error code + */ + abstract protected function _sql_error(): array; + /** * {@inheritDoc} */ @@ -1216,6 +1291,16 @@ abstract class driver implements driver_interface return true; } + /** + * Build db-specific report + * + * @param string $mode 'start' to add to report, 'fromcache' to output it + * @param string $query Query to add to sql report + * + * @return void + */ + abstract protected function _sql_report(string $mode, string $query = ''): void; + /** * {@inheritDoc} */ diff --git a/phpBB/phpbb/db/driver/driver_interface.php b/phpBB/phpbb/db/driver/driver_interface.php index 6d95f61c16..69a22b3f41 100644 --- a/phpBB/phpbb/db/driver/driver_interface.php +++ b/phpBB/phpbb/db/driver/driver_interface.php @@ -180,7 +180,7 @@ interface driver_interface * Return on error or display error message * * @param bool $fail Should we return on errors, or stop - * @return null + * @return void */ public function sql_return_on_error($fail = false); @@ -190,9 +190,9 @@ interface driver_interface * @param string $query Should be on of the following strings: * INSERT, INSERT_SELECT, UPDATE, SELECT, DELETE * @param array $assoc_ary Array with "column => value" pairs - * @return string A SQL statement like "c1 = 'a' AND c2 = 'b'" + * @return string|false A SQL statement like "c1 = 'a' AND c2 = 'b'", false on invalid assoc_ary */ - public function sql_build_array($query, $assoc_ary = array()); + public function sql_build_array($query, $assoc_ary = []); /** * Fetch all rows @@ -299,7 +299,7 @@ interface driver_interface * Add to query count * * @param bool $cached Is this query cached? - * @return null + * @return void */ public function sql_add_num_queries($cached = false); diff --git a/phpBB/phpbb/db/driver/factory.php b/phpBB/phpbb/db/driver/factory.php index b2a5707120..d282aa78e6 100644 --- a/phpBB/phpbb/db/driver/factory.php +++ b/phpBB/phpbb/db/driver/factory.php @@ -49,7 +49,9 @@ class factory implements driver_interface { if ($this->driver === null) { - $this->driver = $this->container->get('dbal.conn.driver'); + /** @var driver_interface $driver */ + $driver = $this->container->get('dbal.conn.driver'); + $this->driver = $driver; } return $this->driver; @@ -238,13 +240,13 @@ class factory implements driver_interface */ public function sql_return_on_error($fail = false) { - return $this->get_driver()->sql_return_on_error($fail); + $this->get_driver()->sql_return_on_error($fail); } /** * {@inheritdoc} */ - public function sql_build_array($query, $assoc_ary = array()) + public function sql_build_array($query, $assoc_ary = []) { return $this->get_driver()->sql_build_array($query, $assoc_ary); } @@ -326,7 +328,7 @@ class factory implements driver_interface */ public function sql_add_num_queries($cached = false) { - return $this->get_driver()->sql_add_num_queries($cached); + $this->get_driver()->sql_add_num_queries($cached); } /** @@ -374,7 +376,7 @@ class factory implements driver_interface */ public function sql_freeresult($query_id = false) { - return $this->get_driver()->sql_freeresult($query_id); + $this->get_driver()->sql_freeresult($query_id); } /** diff --git a/phpBB/phpbb/db/driver/mssql_base.php b/phpBB/phpbb/db/driver/mssql_base.php index c48f7d42a6..747df00bee 100644 --- a/phpBB/phpbb/db/driver/mssql_base.php +++ b/phpBB/phpbb/db/driver/mssql_base.php @@ -43,19 +43,17 @@ abstract class mssql_base extends \phpbb\db\driver\driver } /** - * Build LIKE expression - * @access private + * {@inheritDoc} */ - function _sql_like_expression($expression) + protected function _sql_like_expression(string $expression): string { return $expression . " ESCAPE '\\'"; } /** - * Build NOT LIKE expression - * @access private + * {@inheritDoc} */ - function _sql_not_like_expression($expression) + protected function _sql_not_like_expression(string $expression): string { return $expression . " ESCAPE '\\'"; } @@ -68,15 +66,6 @@ abstract class mssql_base extends \phpbb\db\driver\driver return 'CONVERT(BIGINT, ' . $expression . ')'; } - /** - * Build db-specific query data - * @access private - */ - function _sql_custom_build($stage, $data) - { - return $data; - } - /** * {@inheritDoc} */ diff --git a/phpBB/phpbb/db/driver/mssql_odbc.php b/phpBB/phpbb/db/driver/mssql_odbc.php index 06cdce7a15..211b98fc7e 100644 --- a/phpBB/phpbb/db/driver/mssql_odbc.php +++ b/phpBB/phpbb/db/driver/mssql_odbc.php @@ -24,7 +24,6 @@ namespace phpbb\db\driver; */ class mssql_odbc extends \phpbb\db\driver\mssql_base { - var $last_query_text = ''; var $connect_error = ''; /** @@ -112,31 +111,27 @@ class mssql_odbc extends \phpbb\db\driver\mssql_base if ($raw) { - return $this->sql_server_version; + return (string) $this->sql_server_version; } return ($this->sql_server_version) ? 'MSSQL (ODBC)
' . $this->sql_server_version : 'MSSQL (ODBC)'; } /** - * SQL Transaction - * @access private + * {@inheritDoc} */ - function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': - return @odbc_exec($this->db_connect_id, 'BEGIN TRANSACTION'); - break; + return (bool) @odbc_exec($this->db_connect_id, 'BEGIN TRANSACTION'); case 'commit': - return @odbc_exec($this->db_connect_id, 'COMMIT TRANSACTION'); - break; + return (bool) @odbc_exec($this->db_connect_id, 'COMMIT TRANSACTION'); case 'rollback': - return @odbc_exec($this->db_connect_id, 'ROLLBACK TRANSACTION'); - break; + return (bool) @odbc_exec($this->db_connect_id, 'ROLLBACK TRANSACTION'); } return true; @@ -209,9 +204,9 @@ class mssql_odbc extends \phpbb\db\driver\mssql_base } /** - * Build LIMIT query - */ - function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + * {@inheritDoc} + */ + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -303,23 +298,19 @@ class mssql_odbc extends \phpbb\db\driver\mssql_base if ($cache && !is_object($query_id) && $cache->sql_exists($query_id)) { - return $cache->sql_freeresult($query_id); + $cache->sql_freeresult($query_id); } - - if (isset($this->open_queries[(int) $query_id])) + else if (isset($this->open_queries[(int) $query_id])) { unset($this->open_queries[(int) $query_id]); - return odbc_free_result($query_id); + odbc_free_result($query_id); } - - return false; } /** - * return sql error array - * @access private + * {@inheritDoc} */ - function _sql_error() + protected function _sql_error(): array { if (function_exists('odbc_errormsg')) { @@ -340,19 +331,18 @@ class mssql_odbc extends \phpbb\db\driver\mssql_base } /** - * Close sql connection - * @access private - */ - function _sql_close() + * {@inheritDoc} + */ + protected function _sql_close(): bool { - return @odbc_close($this->db_connect_id); + @odbc_close($this->db_connect_id); + return true; } /** - * Build db-specific report - * @access private + * {@inheritDoc} */ - function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { switch ($mode) { diff --git a/phpBB/phpbb/db/driver/mssqlnative.php b/phpBB/phpbb/db/driver/mssqlnative.php index 30ef9d9bc4..62a5f51772 100644 --- a/phpBB/phpbb/db/driver/mssqlnative.php +++ b/phpBB/phpbb/db/driver/mssqlnative.php @@ -23,10 +23,12 @@ namespace phpbb\db\driver; class mssqlnative extends \phpbb\db\driver\mssql_base { var $m_insert_id = null; - var $last_query_text = ''; var $query_options = array(); var $connect_error = ''; + /** @var string|false Last error result or false if no last error set */ + private $last_error_result = false; + /** * {@inheritDoc} */ @@ -92,24 +94,20 @@ class mssqlnative extends \phpbb\db\driver\mssql_base } /** - * SQL Transaction - * @access private + * {@inheritDoc} */ - function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': return sqlsrv_begin_transaction($this->db_connect_id); - break; case 'commit': return sqlsrv_commit($this->db_connect_id); - break; case 'rollback': return sqlsrv_rollback($this->db_connect_id); - break; } return true; } @@ -182,9 +180,9 @@ class mssqlnative extends \phpbb\db\driver\mssql_base } /** - * Build LIMIT query - */ - function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + * {@inheritDoc} + */ + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -280,7 +278,7 @@ class mssqlnative extends \phpbb\db\driver\mssql_base if ($result_id) { $row = sqlsrv_fetch_array($result_id); - $id = $row[0]; + $id = isset($row[0]) ? (int) $row[0] : false; sqlsrv_free_stmt($result_id); return $id; } @@ -304,23 +302,19 @@ class mssqlnative extends \phpbb\db\driver\mssql_base if ($cache && !is_object($query_id) && $cache->sql_exists($query_id)) { - return $cache->sql_freeresult($query_id); + $cache->sql_freeresult($query_id); } - - if (isset($this->open_queries[(int) $query_id])) + else if (isset($this->open_queries[(int) $query_id])) { unset($this->open_queries[(int) $query_id]); - return sqlsrv_free_stmt($query_id); + sqlsrv_free_stmt($query_id); } - - return false; } /** - * return sql error array - * @access private + * {@inheritDoc} */ - function _sql_error() + protected function _sql_error(): array { if (function_exists('sqlsrv_errors')) { @@ -342,7 +336,7 @@ class mssqlnative extends \phpbb\db\driver\mssql_base } else { - $error = (isset($this->last_error_result) && $this->last_error_result) ? $this->last_error_result : array(); + $error = $this->last_error_result ?: ''; } $error = array( @@ -362,19 +356,17 @@ class mssqlnative extends \phpbb\db\driver\mssql_base } /** - * Close sql connection - * @access private - */ - function _sql_close() + * {@inheritDoc} + */ + protected function _sql_close(): bool { return @sqlsrv_close($this->db_connect_id); } /** - * Build db-specific report - * @access private + * {@inheritDoc} */ - function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { switch ($mode) { diff --git a/phpBB/phpbb/db/driver/mysql_base.php b/phpBB/phpbb/db/driver/mysql_base.php index 5e0b359134..481d4c7992 100644 --- a/phpBB/phpbb/db/driver/mysql_base.php +++ b/phpBB/phpbb/db/driver/mysql_base.php @@ -27,9 +27,9 @@ abstract class mysql_base extends \phpbb\db\driver\driver } /** - * Build LIMIT query - */ - function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + * {@inheritDoc} + */ + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -103,34 +103,13 @@ abstract class mysql_base extends \phpbb\db\driver\driver } /** - * Build LIKE expression - * @access private + * {@inheritDoc} */ - function _sql_like_expression($expression) + protected function _sql_custom_build(string $stage, $data) { - return $expression; - } - - /** - * Build NOT LIKE expression - * @access private - */ - function _sql_not_like_expression($expression) - { - return $expression; - } - - /** - * Build db-specific query data - * @access private - */ - function _sql_custom_build($stage, $data) - { - switch ($stage) + if ($stage === 'FROM' && is_string($data)) { - case 'FROM': - $data = '(' . $data . ')'; - break; + $data = '(' . $data . ')'; } return $data; diff --git a/phpBB/phpbb/db/driver/mysqli.php b/phpBB/phpbb/db/driver/mysqli.php index 1b7f6252f6..e2f6d20033 100644 --- a/phpBB/phpbb/db/driver/mysqli.php +++ b/phpBB/phpbb/db/driver/mysqli.php @@ -69,7 +69,7 @@ class mysqli extends \phpbb\db\driver\mysql_base if ($this->db_connect_id && $this->dbname != '') { // Disable loading local files on client side - @mysqli_options($this->db_connect_id, MYSQLI_OPT_LOCAL_INFILE, false); + @mysqli_options($this->db_connect_id, MYSQLI_OPT_LOCAL_INFILE, 0); /* * As of PHP 8.1 MySQLi default error mode is set to MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT @@ -143,32 +143,28 @@ class mysqli extends \phpbb\db\driver\mysql_base } } - return ($raw) ? $this->sql_server_version : 'MySQL(i) ' . $this->sql_server_version; + return ($raw) ? (string) $this->sql_server_version : 'MySQL(i) ' . $this->sql_server_version; } /** - * SQL Transaction - * @access private + * {@inheritDoc} */ - function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': return @mysqli_autocommit($this->db_connect_id, false); - break; case 'commit': $result = @mysqli_commit($this->db_connect_id); @mysqli_autocommit($this->db_connect_id, true); return $result; - break; case 'rollback': $result = @mysqli_rollback($this->db_connect_id); @mysqli_autocommit($this->db_connect_id, true); return $result; - break; } return true; @@ -293,7 +289,7 @@ class mysqli extends \phpbb\db\driver\mysql_base */ function sql_nextid() { - return ($this->db_connect_id) ? @mysqli_insert_id($this->db_connect_id) : false; + return ($this->db_connect_id) ? (int) @mysqli_insert_id($this->db_connect_id) : false; } /** @@ -310,20 +306,12 @@ class mysqli extends \phpbb\db\driver\mysql_base if ($cache && !is_object($query_id) && $cache->sql_exists($query_id)) { - return $cache->sql_freeresult($query_id); + $cache->sql_freeresult($query_id); } - - if (!$query_id) + else if ($query_id && $query_id !== true) { - return false; + mysqli_free_result($query_id); } - - if ($query_id === true) - { - return true; - } - - return mysqli_free_result($query_id); } /** @@ -335,10 +323,9 @@ class mysqli extends \phpbb\db\driver\mysql_base } /** - * return sql error array - * @access private + * {@inheritDoc} */ - function _sql_error() + protected function _sql_error(): array { if ($this->db_connect_id) { @@ -366,19 +353,17 @@ class mysqli extends \phpbb\db\driver\mysql_base } /** - * Close sql connection - * @access private - */ - function _sql_close() + * {@inheritDoc} + */ + protected function _sql_close(): bool { return @mysqli_close($this->db_connect_id); } /** - * Build db-specific report - * @access private + * {@inheritDoc} */ - function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { static $test_prof; diff --git a/phpBB/phpbb/db/driver/oracle.php b/phpBB/phpbb/db/driver/oracle.php index 04af0a0a9c..b777dcfc72 100644 --- a/phpBB/phpbb/db/driver/oracle.php +++ b/phpBB/phpbb/db/driver/oracle.php @@ -18,9 +18,11 @@ namespace phpbb\db\driver; */ class oracle extends \phpbb\db\driver\driver { - var $last_query_text = ''; var $connect_error = ''; + /** @var array|false Last error result or false if no last error set */ + private $last_error_result = false; + /** * {@inheritDoc} */ @@ -107,24 +109,20 @@ class oracle extends \phpbb\db\driver\driver } /** - * SQL Transaction - * @access private + * {@inheritDoc} */ - function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': return true; - break; case 'commit': return @oci_commit($this->db_connect_id); - break; case 'rollback': return @oci_rollback($this->db_connect_id); - break; } return true; @@ -465,9 +463,9 @@ class oracle extends \phpbb\db\driver\driver } /** - * Build LIMIT query + * {@inheritDoc} */ - function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -621,16 +619,13 @@ class oracle extends \phpbb\db\driver\driver if ($cache && !is_object($query_id) && $cache->sql_exists($query_id)) { - return $cache->sql_freeresult($query_id); + $cache->sql_freeresult($query_id); } - - if (isset($this->open_queries[(int) $query_id])) + else if (isset($this->open_queries[(int) $query_id])) { unset($this->open_queries[(int) $query_id]); - return oci_free_statement($query_id); + oci_free_statement($query_id); } - - return false; } /** @@ -642,28 +637,21 @@ class oracle extends \phpbb\db\driver\driver } /** - * Build LIKE expression - * @access private + * {@inheritDoc} */ - function _sql_like_expression($expression) + protected function _sql_like_expression(string $expression): string { return $expression . " ESCAPE '\\'"; } /** - * Build NOT LIKE expression - * @access private + * {@inheritDoc} */ - function _sql_not_like_expression($expression) + protected function _sql_not_like_expression(string $expression): string { return $expression . " ESCAPE '\\'"; } - function _sql_custom_build($stage, $data) - { - return $data; - } - function _sql_bit_and($column_name, $bit, $compare = '') { return 'BITAND(' . $column_name . ', ' . (1 << $bit) . ')' . (($compare) ? ' ' . $compare : ''); @@ -675,10 +663,9 @@ class oracle extends \phpbb\db\driver\driver } /** - * return sql error array - * @access private + * {@inheritDoc} */ - function _sql_error() + protected function _sql_error(): array { if (function_exists('oci_error')) { @@ -692,7 +679,7 @@ class oracle extends \phpbb\db\driver\driver } else { - $error = (isset($this->last_error_result) && $this->last_error_result) ? $this->last_error_result : array(); + $error = $this->last_error_result ?: ['message' => '', 'code' => '']; } } else @@ -707,19 +694,17 @@ class oracle extends \phpbb\db\driver\driver } /** - * Close sql connection - * @access private - */ - function _sql_close() + * {@inheritDoc} + */ + protected function _sql_close(): bool { return @oci_close($this->db_connect_id); } /** - * Build db-specific report - * @access private + * {@inheritDoc} */ - function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { switch ($mode) { diff --git a/phpBB/phpbb/db/driver/postgres.php b/phpBB/phpbb/db/driver/postgres.php index 3ee4b2b00e..63ebdbf994 100644 --- a/phpBB/phpbb/db/driver/postgres.php +++ b/phpBB/phpbb/db/driver/postgres.php @@ -20,7 +20,6 @@ namespace phpbb\db\driver; class postgres extends \phpbb\db\driver\driver { var $multi_insert = true; - var $last_query_text = ''; var $connect_error = ''; /** @@ -137,28 +136,24 @@ class postgres extends \phpbb\db\driver\driver } } - return ($raw) ? $this->sql_server_version : 'PostgreSQL ' . $this->sql_server_version; + return ($raw) ? (string) $this->sql_server_version : 'PostgreSQL ' . $this->sql_server_version; } /** - * SQL Transaction - * @access private + * {@inheritDoc} */ - function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': - return @pg_query($this->db_connect_id, 'BEGIN'); - break; + return @pg_query($this->db_connect_id, 'BEGIN') !== false; case 'commit': - return @pg_query($this->db_connect_id, 'COMMIT'); - break; + return @pg_query($this->db_connect_id, 'COMMIT') !== false; case 'rollback': - return @pg_query($this->db_connect_id, 'ROLLBACK'); - break; + return @pg_query($this->db_connect_id, 'ROLLBACK') !== false; } return true; @@ -233,18 +228,9 @@ class postgres extends \phpbb\db\driver\driver } /** - * Build db-specific query data - * @access private + * {@inheritDoc} */ - function _sql_custom_build($stage, $data) - { - return $data; - } - - /** - * Build LIMIT query - */ - function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -385,16 +371,13 @@ class postgres extends \phpbb\db\driver\driver $safe_query_id = $this->clean_query_id($query_id); if ($cache && !is_object($query_id) && $cache->sql_exists($safe_query_id)) { - return $cache->sql_freeresult($safe_query_id); + $cache->sql_freeresult($safe_query_id); } - - if (isset($this->open_queries[$safe_query_id])) + else if (isset($this->open_queries[$safe_query_id])) { unset($this->open_queries[$safe_query_id]); - return pg_free_result($query_id); + pg_free_result($query_id); } - - return false; } /** @@ -405,24 +388,6 @@ class postgres extends \phpbb\db\driver\driver return @pg_escape_string($msg); } - /** - * Build LIKE expression - * @access private - */ - function _sql_like_expression($expression) - { - return $expression; - } - - /** - * Build NOT LIKE expression - * @access private - */ - function _sql_not_like_expression($expression) - { - return $expression; - } - /** * {@inheritDoc} */ @@ -440,10 +405,9 @@ class postgres extends \phpbb\db\driver\driver } /** - * return sql error array - * @access private + * {@inheritDoc} */ - function _sql_error() + protected function _sql_error(): array { // pg_last_error only works when there is an established connection. // Connection errors have to be tracked by us manually. @@ -463,10 +427,9 @@ class postgres extends \phpbb\db\driver\driver } /** - * Close sql connection - * @access private - */ - function _sql_close() + * {@inheritDoc} + */ + protected function _sql_close(): bool { // Released resources are already closed, return true in this case if (!is_resource($this->db_connect_id)) @@ -477,10 +440,9 @@ class postgres extends \phpbb\db\driver\driver } /** - * Build db-specific report - * @access private + * {@inheritDoc} */ - function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { switch ($mode) { diff --git a/phpBB/phpbb/db/driver/sqlite3.php b/phpBB/phpbb/db/driver/sqlite3.php index 61b87d86b5..0be8b14104 100644 --- a/phpBB/phpbb/db/driver/sqlite3.php +++ b/phpBB/phpbb/db/driver/sqlite3.php @@ -83,27 +83,20 @@ class sqlite3 extends \phpbb\db\driver\driver } /** - * SQL Transaction - * - * @param string $status Should be one of the following strings: - * begin, commit, rollback - * @return bool Success/failure of the transaction query + * {@inheritDoc} */ - protected function _sql_transaction($status = 'begin') + protected function _sql_transaction(string $status = 'begin'): bool { switch ($status) { case 'begin': return $this->dbo->exec('BEGIN IMMEDIATE'); - break; case 'commit': return $this->dbo->exec('COMMIT'); - break; case 'rollback': return @$this->dbo->exec('ROLLBACK'); - break; } return true; @@ -188,16 +181,9 @@ class sqlite3 extends \phpbb\db\driver\driver } /** - * Build LIMIT query - * - * @param string $query The SQL query to execute - * @param int $total The number of rows to select - * @param int $offset - * @param int $cache_ttl Either 0 to avoid caching or - * the time in seconds which the result shall be kept in cache - * @return mixed Buffered, seekable result handle, false on error + * {@inheritDoc} */ - protected function _sql_query_limit($query, $total, $offset = 0, $cache_ttl = 0) + protected function _sql_query_limit(string $query, int $total, int $offset = 0, int $cache_ttl = 0) { $this->query_result = false; @@ -263,12 +249,13 @@ class sqlite3 extends \phpbb\db\driver\driver if ($cache && !is_object($query_id) && $cache->sql_exists($query_id)) { - return $cache->sql_freeresult($query_id); + $cache->sql_freeresult($query_id); + return; } if ($query_id) { - return @$query_id->finalize(); + @$query_id->finalize(); } } @@ -315,11 +302,9 @@ class sqlite3 extends \phpbb\db\driver\driver } /** - * return sql error array - * - * @return array + * {@inheritDoc} */ - protected function _sql_error() + protected function _sql_error(): array { if (class_exists('SQLite3', false) && isset($this->dbo)) { @@ -340,24 +325,9 @@ class sqlite3 extends \phpbb\db\driver\driver } /** - * Build db-specific query data - * - * @param string $stage Available stages: FROM, WHERE - * @param mixed $data A string containing the CROSS JOIN query or an array of WHERE clauses - * - * @return string The db-specific query fragment + * {@inheritDoc} */ - protected function _sql_custom_build($stage, $data) - { - return $data; - } - - /** - * Close sql connection - * - * @return bool False if failure - */ - protected function _sql_close() + protected function _sql_close(): bool { return $this->dbo->close(); } @@ -365,12 +335,13 @@ class sqlite3 extends \phpbb\db\driver\driver /** * Build db-specific report * - * @param string $mode Available modes: display, start, stop, + * @param string $mode Available modes: display, start, stop, * add_select_row, fromcache, record_fromcache - * @param string $query The Query that should be explained - * @return mixed Either a full HTML page, boolean or null + * @param string $query The Query that should be explained + * + * @return void Either writes HTML to html_hold or outputs a full HTML page */ - protected function _sql_report($mode, $query = '') + protected function _sql_report(string $mode, string $query = ''): void { switch ($mode) { diff --git a/phpBB/phpbb/db/extractor/factory.php b/phpBB/phpbb/db/extractor/factory.php index f27aae720f..7499baba09 100644 --- a/phpBB/phpbb/db/extractor/factory.php +++ b/phpBB/phpbb/db/extractor/factory.php @@ -51,25 +51,30 @@ class factory // Return the appropriate DB extractor if ($this->db instanceof \phpbb\db\driver\mssql_base) { - return $this->container->get('dbal.extractor.extractors.mssql_extractor'); + $extractor = $this->container->get('dbal.extractor.extractors.mssql_extractor'); } else if ($this->db instanceof \phpbb\db\driver\mysql_base) { - return $this->container->get('dbal.extractor.extractors.mysql_extractor'); + $extractor = $this->container->get('dbal.extractor.extractors.mysql_extractor'); } else if ($this->db instanceof \phpbb\db\driver\oracle) { - return $this->container->get('dbal.extractor.extractors.oracle_extractor'); + $extractor = $this->container->get('dbal.extractor.extractors.oracle_extractor'); } else if ($this->db instanceof \phpbb\db\driver\postgres) { - return $this->container->get('dbal.extractor.extractors.postgres_extractor'); + $extractor = $this->container->get('dbal.extractor.extractors.postgres_extractor'); } else if ($this->db instanceof \phpbb\db\driver\sqlite3) { - return $this->container->get('dbal.extractor.extractors.sqlite3_extractor'); + $extractor = $this->container->get('dbal.extractor.extractors.sqlite3_extractor'); + } + else + { + throw new \InvalidArgumentException('Invalid database driver given'); } - throw new \InvalidArgumentException('Invalid database driver given'); + /** @var \phpbb\db\extractor\extractor_interface $extractor */ + return $extractor; } } diff --git a/phpBB/phpbb/db/extractor/mssql_extractor.php b/phpBB/phpbb/db/extractor/mssql_extractor.php index 64922c1124..1fa921bc3d 100644 --- a/phpBB/phpbb/db/extractor/mssql_extractor.php +++ b/phpBB/phpbb/db/extractor/mssql_extractor.php @@ -194,12 +194,12 @@ class mssql_extractor extends base_extractor * Extracts data from database table (for MSSQL Native driver) * * @param string $table_name name of the database table - * @return null + * @return void * @throws extractor_not_initialized_exception when calling this function before init_extractor() */ protected function write_data_mssqlnative($table_name) { - if (!$this->is_initialized) + if (!$this->is_initialized || !$this->db instanceof \phpbb\db\driver\mssqlnative) { throw new extractor_not_initialized_exception(); } diff --git a/phpBB/phpbb/db/extractor/oracle_extractor.php b/phpBB/phpbb/db/extractor/oracle_extractor.php index bc43a37b10..879e377043 100644 --- a/phpBB/phpbb/db/extractor/oracle_extractor.php +++ b/phpBB/phpbb/db/extractor/oracle_extractor.php @@ -184,12 +184,12 @@ class oracle_extractor extends base_extractor FROM $table_name"; $result = $this->db->sql_query($sql); - $i_num_fields = ocinumcols($result); + $i_num_fields = oci_num_fields($result); for ($i = 0; $i < $i_num_fields; $i++) { - $ary_type[$i] = ocicolumntype($result, $i + 1); - $ary_name[$i] = ocicolumnname($result, $i + 1); + $ary_type[$i] = oci_field_type($result, $i + 1); + $ary_name[$i] = oci_field_name($result, $i + 1); } while ($row = $this->db->sql_fetchrow($result)) diff --git a/phpBB/phpbb/db/migration/data/v310/soft_delete_mod_convert.php b/phpBB/phpbb/db/migration/data/v310/soft_delete_mod_convert.php index 407bd39ce6..969b1e227e 100644 --- a/phpBB/phpbb/db/migration/data/v310/soft_delete_mod_convert.php +++ b/phpBB/phpbb/db/migration/data/v310/soft_delete_mod_convert.php @@ -122,6 +122,8 @@ class soft_delete_mod_convert extends container_aware_migration */ protected function get_content_visibility() { - return $this->container->get('content.visibility'); + /** @var \phpbb\content_visibility $content_visibility */ + $content_visibility = $this->container->get('content.visibility'); + return $content_visibility; } } diff --git a/phpBB/phpbb/db/migration/data/v31x/remove_duplicate_migrations.php b/phpBB/phpbb/db/migration/data/v31x/remove_duplicate_migrations.php index 7f8acc0cd5..17f4152f05 100644 --- a/phpBB/phpbb/db/migration/data/v31x/remove_duplicate_migrations.php +++ b/phpBB/phpbb/db/migration/data/v31x/remove_duplicate_migrations.php @@ -49,17 +49,21 @@ class remove_duplicate_migrations extends \phpbb\db\migration\migration $this->db->sql_freeresult($result); + /** + * @var string $name + * @var array $migration + */ foreach ($migration_state as $name => $migration) { $prepended_name = ($name[0] == '\\' ? '' : '\\') . $name; $prefixless_name = $name[0] == '\\' ? substr($name, 1) : $name; - if ($prepended_name != $name && isset($migration_state[$prepended_name]) && $migration_state[$prepended_name]['migration_depends_on'] == $migration_state[$name]['migration_depends_on']) + if ($prepended_name != $name && isset($migration_state[$prepended_name]) && $migration_state[$prepended_name]['migration_depends_on'] == $migration['migration_depends_on']) { $duplicate_migrations[] = $name; unset($migration_state[$prepended_name]); } - else if ($prefixless_name != $name && isset($migration_state[$prefixless_name]) && $migration_state[$prefixless_name]['migration_depends_on'] == $migration_state[$name]['migration_depends_on']) + else if ($prefixless_name != $name && isset($migration_state[$prefixless_name]) && $migration_state[$prefixless_name]['migration_depends_on'] == $migration['migration_depends_on']) { $duplicate_migrations[] = $prefixless_name; unset($migration_state[$prefixless_name]); diff --git a/phpBB/phpbb/db/migration/tool/config.php b/phpBB/phpbb/db/migration/tool/config.php index a351c4858e..b5404bdba5 100644 --- a/phpBB/phpbb/db/migration/tool/config.php +++ b/phpBB/phpbb/db/migration/tool/config.php @@ -47,7 +47,7 @@ class config implements \phpbb\db\migration\tool\tool_interface * @param mixed $config_value The value of the config setting * @param bool $is_dynamic True if it is dynamic (changes very often) * and should not be stored in the cache, false if not. - * @return null + * @return void */ public function add($config_name, $config_value, $is_dynamic = false) { @@ -65,7 +65,7 @@ class config implements \phpbb\db\migration\tool\tool_interface * @param string $config_name The name of the config setting you would * like to update * @param mixed $config_value The value of the config setting - * @return null + * @return void * @throws \phpbb\db\migration\exception */ public function update($config_name, $config_value) @@ -87,7 +87,7 @@ class config implements \phpbb\db\migration\tool\tool_interface * @param string $config_name The name of the config setting you would * like to update * @param mixed $config_value The value of the config setting - * @return null + * @return void * @throws \phpbb\db\migration\exception */ public function update_if_equals($compare, $config_name, $config_value) @@ -105,7 +105,7 @@ class config implements \phpbb\db\migration\tool\tool_interface * * @param string $config_name The name of the config setting you would * like to remove - * @return null + * @return void */ public function remove($config_name) { diff --git a/phpBB/phpbb/db/migration/tool/config_text.php b/phpBB/phpbb/db/migration/tool/config_text.php index 5fe9a25b70..eb3d5b3385 100644 --- a/phpBB/phpbb/db/migration/tool/config_text.php +++ b/phpBB/phpbb/db/migration/tool/config_text.php @@ -45,7 +45,7 @@ class config_text implements \phpbb\db\migration\tool\tool_interface * @param string $config_name The name of the config_text setting * you would like to add * @param mixed $config_value The value of the config_text setting - * @return null + * @return void */ public function add($config_name, $config_value) { @@ -81,7 +81,7 @@ class config_text implements \phpbb\db\migration\tool\tool_interface * * @param string $config_name The name of the config_text setting you would * like to remove - * @return null + * @return void */ public function remove($config_name) { diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index f33172a708..7c8f80444e 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -162,7 +162,7 @@ class module implements \phpbb\db\migration\tool\tool_interface * Optionally you may not send 'modes' and it will insert all of the * modules in that info file. * path, specify that here - * @return null + * @return void * @throws \phpbb\db\migration\exception */ public function add($class, $parent = 0, $data = array()) diff --git a/phpBB/phpbb/db/migration/tool/permission.php b/phpBB/phpbb/db/migration/tool/permission.php index 8628c38f5b..91838a4a47 100644 --- a/phpBB/phpbb/db/migration/tool/permission.php +++ b/phpBB/phpbb/db/migration/tool/permission.php @@ -21,7 +21,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface /** @var \phpbb\auth\auth */ protected $auth; - /** @var \includes\acp\auth\auth_admin */ + /** @var \auth_admin */ protected $auth_admin; /** @var \phpbb\cache\service */ @@ -115,7 +115,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface * @param bool $global True for checking a global permission setting, * False for a local permission setting * @param int|false $copy_from If set, contains the id of the permission from which to copy the new one. - * @return null + * @return void */ public function add($auth_option, $global = true, $copy_from = false) { @@ -189,7 +189,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface * @param string $auth_option The name of the permission (auth) option * @param bool $global True for checking a global permission setting, * False for a local permission setting - * @return null + * @return void */ public function remove($auth_option, $global = true) { @@ -266,7 +266,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface * @param string $role_type The type (u_, m_, a_) * @param string $role_description Description of the new role * - * @return null + * @return int|null Inserted SQL id or false if role already exists */ public function role_add($role_name, $role_type, $role_description = '') { @@ -292,7 +292,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface $sql = 'INSERT INTO ' . ACL_ROLES_TABLE . ' ' . $this->db->sql_build_array('INSERT', $sql_ary); $this->db->sql_query($sql); - return $this->db->sql_nextid(); + return (int) $this->db->sql_nextid(); } /** @@ -300,7 +300,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface * * @param string $old_role_name The old role name * @param string $new_role_name The new role name - * @return null + * @return void * @throws \phpbb\db\migration\exception */ public function role_update($old_role_name, $new_role_name) @@ -320,7 +320,7 @@ class permission implements \phpbb\db\migration\tool\tool_interface * Remove a permission role * * @param string $role_name The role name to remove - * @return null + * @return void */ public function role_remove($role_name) { @@ -704,9 +704,6 @@ class permission implements \phpbb\db\migration\tool\tool_interface break; } - if ($call) - { - return call_user_func_array(array(&$this, $call), $arguments); - } + return $call ? call_user_func_array(array(&$this, $call), $arguments) : null; } } diff --git a/phpBB/phpbb/db/migration/tool/tool_interface.php b/phpBB/phpbb/db/migration/tool/tool_interface.php index 07cd2435e4..1b66807665 100644 --- a/phpBB/phpbb/db/migration/tool/tool_interface.php +++ b/phpBB/phpbb/db/migration/tool/tool_interface.php @@ -31,7 +31,7 @@ interface tool_interface * First argument is the original call to the class (e.g. add, remove) * After the first argument, send the original arguments to the function in the original call * - * @return null + * @return mixed|null Return of function call or null if no valid function call found */ public function reverse(); } diff --git a/phpBB/phpbb/db/migrator.php b/phpBB/phpbb/db/migrator.php index ef97438cf9..bdb85a7996 100644 --- a/phpBB/phpbb/db/migrator.php +++ b/phpBB/phpbb/db/migrator.php @@ -90,7 +90,7 @@ class migrator * * 'effectively_installed' set and set to true if the migration was effectively_installed * - * @var array|bool + * @var array|false */ protected $last_run_migration = false; @@ -192,7 +192,7 @@ class migrator * The array contains 'name', 'class' and 'state'. 'effectively_installed' is set * and set to true if the last migration was effectively_installed. * - * @return array + * @return array|false Last run migration information or false if no migration has been run yet */ public function get_last_run_migration() { @@ -296,7 +296,7 @@ class migrator /** * Effectively runs a single update step from the next migration to be applied. * - * @return null + * @return void */ protected function update_do() { @@ -517,7 +517,7 @@ class migrator * Effectively runs a single revert step from the last migration installed * * @param string $migration String migration name to revert (including any that depend on this migration) - * @return null + * @return void */ protected function revert_do($migration) { @@ -651,7 +651,7 @@ class migrator * @param array $steps The steps to run * @param bool|string $state Current state of the migration * @param bool $revert true to revert a data step - * @return bool|string migration state. True if completed, serialized array if not finished + * @return bool|array{result: mixed, step: int} migration state. True if completed, serialized array if not finished * @throws \phpbb\db\migration\exception */ protected function process_data_step($steps, $state, $revert = false) @@ -744,7 +744,8 @@ class migrator * @param array $step Data step from migration * @param mixed $last_result Result to pass to the callable (only for 'custom' method) * @param bool $reverse False to install, True to attempt uninstallation by reversing the call - * @return array Array with parameters for call_user_func_array(), 0 is the callable, 1 is parameters + * @return array|false Array with parameters for call_user_func_array(), 0 is the callable, 1 is parameters; + * false if no callable can be created from data setp * @throws \phpbb\db\migration\exception */ protected function get_callable_from_step(array $step, $last_result = 0, $reverse = false) diff --git a/phpBB/phpbb/db/tools/doctrine.php b/phpBB/phpbb/db/tools/doctrine.php index 0e5e8ffbf4..e956bcff4b 100644 --- a/phpBB/phpbb/db/tools/doctrine.php +++ b/phpBB/phpbb/db/tools/doctrine.php @@ -462,7 +462,8 @@ class doctrine implements tools_interface } catch (Exception $e) { - return $e->getMessage(); + // @todo: check if it makes sense to properly handle the exception + return false; } }