diff --git a/composer.json b/composer.json index e8c468995a9..d538484a417 100644 --- a/composer.json +++ b/composer.json @@ -20,7 +20,8 @@ "php-webdriver/webdriver" : "^1.8.2", "phan/phan": ">=2.3.0", "phpmd/phpmd": "~2.8", - "phpstan/phpstan": "0.12.17" + "phpstan/phpstan": "0.12.17", + "slevomat/coding-standard": "^6.0" }, "scripts": { "pre-install-cmd": "mkdir -p project/libraries" diff --git a/composer.lock b/composer.lock index 3fe5236a596..1d41ccdd7b0 100644 --- a/composer.lock +++ b/composer.lock @@ -3706,6 +3706,47 @@ "homepage": "https://github.com/sebastianbergmann/version", "time": "2016-10-03T07:35:21+00:00" }, + { + "name": "slevomat/coding-standard", + "version": "6.1.1", + "source": { + "type": "git", + "url": "https://github.com/slevomat/coding-standard.git", + "reference": "0a7934d7ecdfe402079027513daa3b7e881f315d" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/slevomat/coding-standard/zipball/0a7934d7ecdfe402079027513daa3b7e881f315d", + "reference": "0a7934d7ecdfe402079027513daa3b7e881f315d", + "shasum": "" + }, + "require": { + "php": "^7.1", + "phpstan/phpdoc-parser": "0.3.5 - 0.4.2", + "squizlabs/php_codesniffer": "^3.5.3" + }, + "require-dev": { + "dealerdirect/phpcodesniffer-composer-installer": "0.5.0", + "jakub-onderka/php-parallel-lint": "1.0.0", + "phing/phing": "2.16.2", + "phpstan/phpstan": "0.11.19|0.12.5", + "phpstan/phpstan-phpunit": "0.11.2|0.12.6", + "phpstan/phpstan-strict-rules": "0.11.1|0.12.1", + "phpunit/phpunit": "7.5.18|8.5.2" + }, + "type": "phpcodesniffer-standard", + "autoload": { + "psr-4": { + "SlevomatCodingStandard\\": "SlevomatCodingStandard" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "description": "Slevomat Coding Standard for PHP_CodeSniffer complements Consistence Coding Standard by providing sniffs with additional checks.", + "time": "2020-01-23T15:37:30+00:00" + }, { "name": "squizlabs/php_codesniffer", "version": "3.5.3", diff --git a/php/libraries/Database.class.inc b/php/libraries/Database.class.inc index aba643a0279..3d9d2b046ce 100644 --- a/php/libraries/Database.class.inc +++ b/php/libraries/Database.class.inc @@ -1,4 +1,5 @@ _trackChanges = $trackChanges; $this->_databaseName = $database; @@ -219,7 +220,7 @@ class Database /** * Determines whether the database connection is alive * - * @return boolean + * @return bool * @access public */ function isConnected(): bool @@ -241,8 +242,8 @@ class Database * This will insert a row. HTML from any field in the row will be automatically * escaped to avoid injection. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row * * @return void */ @@ -258,8 +259,8 @@ class Database * automatically escaped. This should only be called when we know the source of * the input is trustworthy and must contain HTML. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row * * @return void */ @@ -274,8 +275,8 @@ class Database * This will insert a row. HTML from any field in the row will be automatically * escaped to avoid injection. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row * * @return void */ @@ -292,8 +293,8 @@ class Database * value for any unique key, the row where the duplicate is present will be * updated. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the row * * @return bool */ @@ -310,8 +311,8 @@ class Database * value for any unique key, the row where the duplicate is present will be * updated. HTML from any field in the row will *not* be automatically escaped. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the row * * @return bool */ @@ -332,9 +333,9 @@ class Database * * This function alone does not guarantee safety. * - * @param array $set The array to be inserted as a new row + * @param array $set The array to be inserted as a new row * - * @return array A copy of $set with the HTML characters escaped + * @return array A copy of $set with the HTML characters escaped */ private function _HTMLEscapeArray(array $set): array { @@ -359,24 +360,31 @@ class Database * * Inserts a single row into the specified table, containing the values specified * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row - * @param bool $autoescape determines whether the values to be set - * should automatically have the html escaped - * @param bool $ignore determines whether the insert throws an - * error or is discarded when value exists in DB - * @param bool $onDuplicateUpdate determines whether the row should be updated - * upon unique key duplication + * @param string $table the table into which to + * insert the row + * @param array $set the values with which to + * fill the new row + * @param bool $autoescape determines whether the + * values to be set + * should automatically + * have the html escaped + * @param bool $ignore determines whether the insert + * throws an + * error or is discarded + * when value exists in DB + * @param bool $onDuplicateUpdate determines whether the row + * should be updated + * upon unique key duplication * * @return bool */ private function _realinsert( string $table, - array $set, - bool $autoescape = true, - bool $ignore = false, - bool $onDuplicateUpdate = false - ) : bool { + array $set, + bool $autoescape = true, + bool $ignore = false, + bool $onDuplicateUpdate = false + ): bool { if ($ignore && $onDuplicateUpdate) { throw new DatabaseException( 'The Database::_realinsert() function does not accept both ignore @@ -447,8 +455,8 @@ class Database * Replaces into the table such if there already exists a row with * the same primary key it will be replaced by the new row * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row * * @return void * @access public @@ -479,9 +487,10 @@ class Database * Updates a single row in the specified table. This will automatically escape * any HTML in the data being inserted for security. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row - * @param array $i_where the selection filter, joined as a boolean and + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row + * @param array $i_where the selection filter, joined as a + * boolean and * * @return void */ @@ -497,9 +506,11 @@ class Database * escape and should be used with caution, only when you know you need to * insert HTML and know that you can trust it. * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row - * @param array $i_where the selection filter, joined as a boolean and + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the + * new row + * @param array $i_where the selection filter, joined as + * a boolean and * * @return void */ @@ -517,14 +528,18 @@ class Database * * Updates a single row in the specified table * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row - * @param array $i_where the selection filter, joined as a boolean and - * @param bool $autoescape determines whether the values to be set should - * automatically have the html escaped + * @param string $table the table into which to insert + * the row + * @param array $set the values with which to fill + * the new row + * @param array $i_where the selection filter, joined as + * a boolean and + * @param bool $autoescape determines whether the values to + * be set should + * automatically have the html escaped * * @return bool Always true. An exception should be thrown if something goes - * wrong. + * wrong. FIXME This should probably be void. * @access public */ private function _realupdate( @@ -590,8 +605,9 @@ class Database * * Deletes a single row in the specified table * - * @param string $table the table into which to insert the row - * @param array $where the selection filter, joined as a boolean and + * @param string $table the table into which to insert the row + * @param array $where the selection filter, joined as + * a boolean and * * @return void * @access public @@ -662,7 +678,7 @@ class Database * * @return PDOStatement A PDO prepared statement representing $query */ - function prepare($query) + function prepare(string $query): PDOStatement { return $this->_PDO->prepare($query); } @@ -671,19 +687,22 @@ class Database * Executes a previously prepared statements using the variable * bindings given. * - * @param PDOStatement $prepared The prepared statement - * @param array $params The values to bind to the statement - * while executing it - * @param array $options A list of key=>value pairs. - * - nofetch : to prevent the fetchAll - * function to be excuted. Useful for - * insert or update prepared statements. - * - * @return array An array of rows in the format ColName => Value after - * executing the statement. + * @param PDOStatement $prepared The prepared statement + * @param array $params The values to bind to the statement + * while executing it + * @param array $options A list of key=>value pairs. + * - nofetch : to prevent the + * fetchAll function to be + * excuted. Useful for insert + * or update prepared + * statements. + * + * @return array> An array of rows in the format + * ColName => Value after + * executing the statement. */ function execute( - $prepared, + PDOStatement $prepared, array $params, array $options = array() ): array { @@ -711,10 +730,12 @@ class Database /** * Runs an SQL select statement as a prepared query * - * @param string $query The SQL SELECT query to be run - * @param array $params Values to use for binding to the prepared statement + * @param string $query The SQL SELECT query to be run + * @param array $params Values to use for binding to the + * prepared statement * - * @return array An array of arrays containing the data. + * @return array> An array of arrays containing + * the data. * The outside (non-associative array contains 1 element per * row returned by the query, and each element is an associative * array representing the row in the format ColumnName => Value @@ -730,17 +751,19 @@ class Database * Runs an SQL select statement as a prepared query and re-indexes * the results using the given unique non-nullable key. * - * @param string $query The SQL SELECT query to be run - * @param array $params Values to use for binding to the prepared statement - * @param string $uniqueKey Key to use when re-indexing, this key must be a - * single column, must be unique and must not be - * nullable + * @param string $query The SQL SELECT query to be run + * @param array $params Values to use for binding to the + * prepared statement + * @param string $uniqueKey Key to use when re-indexing, this + * key must be a single column and + * must be unique * * @throws LorisException If the supplied key is empty or null * @throws DatabaseException If the key is not part of the query itself * @throws DatabaseException If the key is not unique within the resulting set * - * @return array An array of arrays containing the data. The outermost array is + * @return array> An array of arrays containing + * the data. The outermost array is * associative and uses the supplied $uniqueKey parameter as * a key for each of the sub-arrays with the format * rowPrimaryKey=>rowValuesArray. Each nested array represents @@ -752,7 +775,7 @@ class Database string $query, array $params, string $uniqueKey - ) { + ): array { if (is_null($uniqueKey) || empty($uniqueKey)) { throw new LorisException( @@ -805,13 +828,15 @@ class Database * associative array. Automatically adds a limit clause to the query being * run for efficiency. * - * @param string $query The SQL SELECT query to be run - * @param array $params Values to use for binding to prepared statement + * @param string $query The SQL SELECT query to be run + * @param array $params Values to use for binding to prepared + * statement * - * @return ?array Associative array of form ColumnName => Value for each column - * in the first row of the query + * @return array|null Associative array of form + * ColumnName => Value for each column in the + * first row of the query, or null. */ - function pselectRow(string $query, array $params) : ?array + function pselectRow(string $query, array $params): ?array { $rows = $this->pselect($query . " LIMIT 2", $params); if (count($rows) > 1) { @@ -824,18 +849,19 @@ class Database /** * Runs a query as a prepared statement and returns the values of the - * column given in the select statement. If multiple columns are given, an error - * is thrown. + * column given in the select statement. If multiple columns are given, + * an error is thrown. * - * @param string $query The SQL SELECT query to be run - * @param array $params Values to use for binding to prepared statement + * @param string $query The SQL SELECT query to be run + * @param array $params Values to use for binding to prepared + * statement * * @throws DatabaseException if the query selected more than one column * - * @return array Associative array of form rowID=>value containing all values - * for the only column of the select statement + * @return array Format: rowID=>value containing + * all values for the only column of the select statement */ - function pselectCol(string $query, array $params) + function pselectCol(string $query, array $params): array { $unprocessed = $this->pselect($query, $params); @@ -861,25 +887,26 @@ class Database * the results using the given unique non-nullable key in the same * format as the pselectCol() function. * - * @param string $query The SQL SELECT query to be run - * @param array $params Values to use for binding to the prepared statement - * @param string $uniqueKey Key to use when re-indexing, this key must be a - * single column, must be unique and must not be - * nullable + * @param string $query The SQL SELECT query to be run + * @param array $params Values to use for binding to the + * prepared statement + * @param string $uniqueKey Key to use when re-indexing, this + * key must be a single column and + * must be unique * * @throws LorisException If the supplied key is empty or null * @throws DatabaseException If the key is not part of the query itself or * if there are not exactly 2 columns selected * @throws DatabaseException If the key is not unique within the resulting set * - * @return array Associative array of form uniqueKey=>value containing all - * value for the non-uniqueKey element of the select statement + * @return array Format: uniqueKey=>value containing all + * values for the non-unique key element of the select statement */ function pselectColWithIndexKey( string $query, array $params, string $uniqueKey - ) { + ): array { if (is_null($uniqueKey) || empty($uniqueKey)) { throw new LorisException( @@ -946,8 +973,9 @@ class Database * returning an array of a single element) so that we can properly enforce * types. * - * @param string $query The SQL statement to run - * @param array $params Values to use for binding in the prepared statement + * @param string $query The SQL statement to run + * @param array $params Values to use for binding in the + * prepared statement * * @return mixed The value returned by the query: array, string, or false. */ @@ -966,9 +994,9 @@ class Database * Sets each hash element into the format key='value', and then * implodes the resultant array with the specified glue * - * @param string $glue The glue to pass to php's implode function - * @param array $dataArray The array with keys to implode - * @param string $clause The type of clause set_|where_ + * @param string $glue The glue to pass to implode() + * @param array $dataArray The array with keys to implode + * @param string $clause The type of clause set_|where_ * * @return string */ @@ -998,15 +1026,17 @@ class Database * Helper function to generate the string for the WHERE part of an update * or delete query. Generates the string in a prepared statement format. * - * @param string $glue The glue used to combine parts of the - * dataArray (ie " AND "). - * @param array $dataArray The array representing the WHERE condition - * to be glued together into a string. - * @param array|null $exec_vals The values which are being bound to the - * query. Used to generate the prepared variable - * name. - * @param string $prefix A prefix to apply to prepared variable - * names. + * @param string $glue The glue used to combine parts + * of the dataArray (ie " AND "). + * @param array $dataArray The array representing the + * WHERE condition to be glued + * together into a string. + * @param array|null $exec_vals The values which are being + * bound to the query. + * Used to generate the prepared + * variable name. + * @param string $prefix A prefix to apply to prepared + * variable names. * * @return string A string that can be used to generate a prepared statement * with appropriate variable names generated for data @@ -1057,11 +1087,12 @@ class Database * Determines the difference between the old values and the new, * then saves a reference to that change * - * @param string $table the table into which to insert the row - * @param array $set the values with which to fill the new row - * @param string $where the selection filter, joined as a boolean and - * @param string $type The type of change being tracked (*I*nsert, - * *U*pdate or *D*elete) + * @param string $table the table into which to insert the row + * @param array $set the values with which to fill the new row + * @param string $where the selection filter, joined as a + * boolean and + * @param string $type The type of change being tracked + * (*I*nsert,*U*pdate or *D*elete) * * @return void As a side-effect populates history table */ @@ -1184,10 +1215,13 @@ class Database /** * Print a query if showDatabaseQueries defined in config file * - * @param string $query The query to replace - * @param array $params The prepared statement parameters used for this query - * They will be replaced in the print statement so that the - * user knows what parameters were used. + * @param string $query The query to replace + * @param array $params The prepared statement parameters + * used for this query. + * They will be replaced in the print + * statement so that + * the user knows what parameters + * were used. * * @return void As a side-effect, prints to the screen if config option is * enabled @@ -1214,9 +1248,9 @@ class Database * without mocking every single query that needs to be used in * that test. * - * @param string $tableName The table name to fake - * @param array $rowData An array of data to be inserted into - * the fake table. + * @param string $tableName The table name to fake + * @param array $rowData An array of data to be inserted into + * the fake table. * * @return void */ @@ -1261,7 +1295,7 @@ class Database * * @param string $test_name The table to check for. * - * @return boolean true if the table exists + * @return bool true if the table exists */ function tableExists(string $test_name): bool { @@ -1292,7 +1326,7 @@ class Database * @param string $test_name The table to check for a column. * @param string $column The column name to check the table for. * - * @return boolean true if the table has a the given column + * @return bool true if the table has a the given column */ function columnExists(string $test_name, string $column): bool { diff --git a/test/StrictTypesCS.xml b/test/StrictTypesCS.xml new file mode 100644 index 00000000000..471e08162a2 --- /dev/null +++ b/test/StrictTypesCS.xml @@ -0,0 +1,36 @@ + + + An iterative effort to enforce the use of strict typing in LORIS + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/run-php-linter.sh b/test/run-php-linter.sh index 2359f607e8d..984fc55c1de 100755 --- a/test/run-php-linter.sh +++ b/test/run-php-linter.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash set -euo pipefail + # Run PHP -l on everything to ensure there's no syntax # errors. find docs modules htdocs php src tools \ @@ -28,6 +29,7 @@ declare -a tools_list=( 'single_use/Cleanup_multiple_firstVisits.php' 'single_use/Convert_LorisMenuID_to_ModuleID.php' ) + # And on all PHP files in this array declare -a test_list=( 'integrationtests/LorisIntegrationTest.class.inc' @@ -42,6 +44,13 @@ vendor/bin/phpcs --standard=test/LorisCS.xml --extensions=php,inc \ "${test_list[@]/#/test/}" \ || exit $?; +# Ensure strict typing is used in these files +declare -a strict_libraries=( + 'Database.class.inc' +) + +vendor/bin/phpcs --standard=test/StrictTypesCS.xml --extensions=php,inc "${strict_libraries[@]/#/php/libraries/}" || exit $?; + # Run PHPCS on src/ directory using a different ruleset conforming to PSR2. vendor/bin/phpcs --standard=test/SrcCS.xml --extensions=php/php src/ || exit $?;