From d83368a475103da04af8af962db4a522f894f60a Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 17 Nov 2022 16:00:44 +0100 Subject: [PATCH 01/22] MDL-76362 mustache: Override parent method to avoid PHP notices Parent method checks baseDir that is null in this case. This shows notices under PHP 8.1 --- lib/classes/output/mustache_filesystem_loader.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/classes/output/mustache_filesystem_loader.php b/lib/classes/output/mustache_filesystem_loader.php index f90623ce47a..e79b479fed5 100644 --- a/lib/classes/output/mustache_filesystem_loader.php +++ b/lib/classes/output/mustache_filesystem_loader.php @@ -53,4 +53,16 @@ class mustache_filesystem_loader extends \Mustache_Loader_FilesystemLoader { // Call the Moodle template finder. return mustache_template_finder::get_template_filepath($name); } + + /** + * Only check if baseDir is a directory and requested templates are files if + * baseDir is using the filesystem stream wrapper. + * + * Always check path for mustache_filesystem_loader. + * + * @return bool Whether to check `is_dir` and `file_exists` + */ + protected function shouldCheckPath() { + return true; + } } From 2dd7290ccbd4057192ba0d695411f62e9f3a2b4d Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Thu, 17 Nov 2022 16:04:24 +0100 Subject: [PATCH 02/22] MDL-76362 various: Avoid passing nulls to functions that don't allow nulls PHP 8.1 is more strict on the parameter type. Functions such as trim(), strlen(), str_replace(), etc show notice when null is passed as an argument --- .../tool/analytics/classes/output/helper.php | 2 +- admin/tool/behat/cli/run.php | 2 +- admin/tool/brickfield/classes/manager.php | 2 +- .../dataprivacy/classes/data_registry.php | 2 +- .../classes/external/purpose_exporter.php | 2 +- admin/tool/log/classes/helper/reader.php | 4 +-- .../classes/local/remote_resource.php | 4 +-- analytics/classes/model.php | 2 +- backup/moodle2/backup_custom_fields.php | 2 +- .../helper/restore_decode_processor.class.php | 2 +- ...store_structure_parser_processor.class.php | 2 +- backup/util/xml/output/xml_output.class.php | 2 +- backup/util/xml/xml_writer.class.php | 2 +- badges/classes/backpack_api_mapping.php | 4 +-- cache/classes/helper.php | 2 +- .../external/course_summary_exporter.php | 2 +- course/format/classes/base.php | 2 +- .../output/local/content/frontpagesection.php | 2 +- course/user.php | 2 +- customfield/classes/field.php | 2 +- enrol/lti/classes/data_connector.php | 2 +- .../local/ltiadvantage/lib/http_client.php | 4 +-- .../singleview/classes/local/screen/user.php | 2 +- h5p/ajax.php | 2 +- h5p/classes/api.php | 2 +- h5p/classes/editor_framework.php | 2 +- lib/accesslib.php | 4 +-- .../FilesystemSkipPassedListLocator.php | 2 +- lib/classes/component.php | 2 +- lib/classes/date.php | 2 +- lib/dml/pgsql_native_moodle_database.php | 2 +- lib/evalmath/evalmath.class.php | 2 +- lib/evalmath/readme_moodle.txt | 3 ++- lib/filebrowser/file_info.php | 2 +- lib/filelib.php | 26 +++++++++---------- lib/filestorage/file_storage.php | 4 +-- lib/filestorage/file_system_filedir.php | 2 +- lib/filestorage/tgz_extractor.php | 2 +- lib/form/classes/filetypes_util.php | 2 +- lib/form/passwordunmask.php | 2 +- lib/formslib.php | 2 +- lib/html2text/lib.php | 2 +- lib/mathslib.php | 2 +- lib/navigationlib.php | 2 +- lib/outputcomponents.php | 2 +- lib/outputlib.php | 2 +- lib/setuplib.php | 1 + lib/tablelib.php | 2 +- lib/tests/behat/behat_accessibility.php | 2 +- lib/weblib.php | 9 ++++--- media/player/youtube/classes/plugin.php | 4 +-- mod/data/classes/preset.php | 4 +-- mod/data/classes/search/entry.php | 2 +- mod/data/field/latlong/field.class.php | 6 ++--- mod/data/field/number/field.class.php | 2 +- mod/data/lib.php | 10 +++---- mod/h5pactivity/classes/xapi/handler.php | 2 +- mod/lti/lib.php | 2 +- mod/lti/locallib.php | 4 +-- .../accessrule/seb/classes/quiz_settings.php | 2 +- mod/survey/classes/privacy/provider.php | 2 +- mod/workshop/classes/portfolio_caller.php | 2 +- .../local/request/moodle_content_writer.php | 2 +- .../classes/tests/request/content_writer.php | 2 +- question/engine/renderer.php | 2 +- question/type/calculated/db/upgradelib.php | 6 ++--- .../type/calculatedmulti/db/upgradelib.php | 6 ++--- question/type/multianswer/renderer.php | 2 +- question/type/numerical/questiontype.php | 2 +- 69 files changed, 105 insertions(+), 102 deletions(-) diff --git a/admin/tool/analytics/classes/output/helper.php b/admin/tool/analytics/classes/output/helper.php index 42025621144..c4628af2866 100644 --- a/admin/tool/analytics/classes/output/helper.php +++ b/admin/tool/analytics/classes/output/helper.php @@ -45,7 +45,7 @@ class helper { // Form field is PARAM_ALPHANUMEXT and we are sending fully qualified class names // as option names, but replacing the backslash for a string that is really unlikely // to ever be part of a class name. - return str_replace('\\', '__', $class); + return str_replace('\\', '__', $class ?? ''); } /** diff --git a/admin/tool/behat/cli/run.php b/admin/tool/behat/cli/run.php index afaddd8b88d..454299e7612 100644 --- a/admin/tool/behat/cli/run.php +++ b/admin/tool/behat/cli/run.php @@ -437,7 +437,7 @@ function print_combined_run_output($processes, $stoponfail = false) { $process->stop(0); } - $strlentoprint = strlen($update); + $strlentoprint = strlen($update ?? ''); // If not enough dots printed on line then just print. if ($strlentoprint < $remainingprintlen) { diff --git a/admin/tool/brickfield/classes/manager.php b/admin/tool/brickfield/classes/manager.php index 013a4b3f129..97fff856690 100644 --- a/admin/tool/brickfield/classes/manager.php +++ b/admin/tool/brickfield/classes/manager.php @@ -205,7 +205,7 @@ class manager { * @return string */ public static function get_contenthash(?string $content = null): string { - return sha1($content); + return sha1($content ?? ''); } /** diff --git a/admin/tool/dataprivacy/classes/data_registry.php b/admin/tool/dataprivacy/classes/data_registry.php index 75670f4953c..fcfe0d25e2c 100644 --- a/admin/tool/dataprivacy/classes/data_registry.php +++ b/admin/tool/dataprivacy/classes/data_registry.php @@ -47,7 +47,7 @@ class data_registry { * @return string[] */ public static function var_names_from_context($classname, $pluginname = '') { - $pluginname = trim($pluginname); + $pluginname = trim($pluginname ?? ''); if (!empty($pluginname)) { $categoryvar = $classname . '_' . $pluginname . '_category'; $purposevar = $classname . '_' . $pluginname . '_purpose'; diff --git a/admin/tool/dataprivacy/classes/external/purpose_exporter.php b/admin/tool/dataprivacy/classes/external/purpose_exporter.php index 8e2423553a0..bcbca407c4d 100644 --- a/admin/tool/dataprivacy/classes/external/purpose_exporter.php +++ b/admin/tool/dataprivacy/classes/external/purpose_exporter.php @@ -111,7 +111,7 @@ class purpose_exporter extends persistent_exporter { $values['formattedlawfulbases'] = $formattedbases; $formattedsensitivereasons = []; - $sensitivereasons = explode(',', $this->persistent->get('sensitivedatareasons')); + $sensitivereasons = explode(',', $this->persistent->get('sensitivedatareasons') ?? ''); if (!empty($sensitivereasons)) { foreach ($sensitivereasons as $reason) { if (empty(trim($reason))) { diff --git a/admin/tool/log/classes/helper/reader.php b/admin/tool/log/classes/helper/reader.php index 2e947748da0..ae0a376c816 100644 --- a/admin/tool/log/classes/helper/reader.php +++ b/admin/tool/log/classes/helper/reader.php @@ -74,10 +74,10 @@ trait reader { * @return mixed Decoded value */ public static function decode_other(?string $other) { - if ($other === 'N;' || preg_match('~^.:~', $other)) { + if ($other === 'N;' || preg_match('~^.:~', $other ?? '')) { return unserialize($other); } else { - return json_decode($other, true); + return json_decode($other ?? '', true); } } diff --git a/admin/tool/moodlenet/classes/local/remote_resource.php b/admin/tool/moodlenet/classes/local/remote_resource.php index e9526184b8c..a08a3bb7015 100644 --- a/admin/tool/moodlenet/classes/local/remote_resource.php +++ b/admin/tool/moodlenet/classes/local/remote_resource.php @@ -60,8 +60,8 @@ class remote_resource { public function __construct(\curl $curl, url $url, \stdClass $metadata) { $this->curl = $curl; $this->url = $url; - $this->filename = pathinfo($this->url->get_path(), PATHINFO_FILENAME); - $this->extension = pathinfo($this->url->get_path(), PATHINFO_EXTENSION); + $this->filename = pathinfo($this->url->get_path() ?? '', PATHINFO_FILENAME); + $this->extension = pathinfo($this->url->get_path() ?? '', PATHINFO_EXTENSION); $this->metadata = $metadata; } diff --git a/analytics/classes/model.php b/analytics/classes/model.php index 5fc63d4b166..155cb3d66c0 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -1828,7 +1828,7 @@ class model { */ public function get_name() { - if (trim($this->model->name) === '') { + if (trim($this->model->name ?? '') === '') { return $this->get_target()->get_name(); } else { diff --git a/backup/moodle2/backup_custom_fields.php b/backup/moodle2/backup_custom_fields.php index 9de5ce68067..2372003b170 100644 --- a/backup/moodle2/backup_custom_fields.php +++ b/backup/moodle2/backup_custom_fields.php @@ -197,7 +197,7 @@ class encrypted_final_element extends backup_final_element { $iv = self::generate_encryption_random_key(openssl_cipher_iv_length(backup::CIPHER)); // Everything is ready, let's encrypt and prepend the 1-shot iv. - $value = $iv . openssl_encrypt($value, backup::CIPHER, $this->key, OPENSSL_RAW_DATA, $iv); + $value = $iv . openssl_encrypt($value ?? '', backup::CIPHER, $this->key, OPENSSL_RAW_DATA, $iv); // Calculate the hmac of the value (iv + encrypted) and prepend it. $hmac = hash_hmac('sha256', $value, $this->key, true); diff --git a/backup/util/helper/restore_decode_processor.class.php b/backup/util/helper/restore_decode_processor.class.php index 0c7bf1e2cd7..80aa087b3f1 100644 --- a/backup/util/helper/restore_decode_processor.class.php +++ b/backup/util/helper/restore_decode_processor.class.php @@ -170,7 +170,7 @@ class restore_decode_processor { */ protected function precheck_content($content) { // Look for $@ in content (all interlinks contain that) - return (strpos($content, '$@') === false) ? false : $content; + return (strpos($content ?? '', '$@') === false) ? false : $content; } } diff --git a/backup/util/helper/restore_structure_parser_processor.class.php b/backup/util/helper/restore_structure_parser_processor.class.php index 9fc1f6e4d27..fdd9cb927bc 100644 --- a/backup/util/helper/restore_structure_parser_processor.class.php +++ b/backup/util/helper/restore_structure_parser_processor.class.php @@ -55,7 +55,7 @@ class restore_structure_parser_processor extends grouped_parser_processor { return ''; } else if (is_numeric($cdata)) { return $cdata; - } else if (strlen($cdata) < 32) { // Impossible to have one link in 32cc + } else if (strlen($cdata ?? '') < 32) { // Impossible to have one link in 32cc return $cdata; // (http://10.0.0.1/file.php/1/1.jpg, http://10.0.0.1/mod/url/view.php?id=) } diff --git a/backup/util/xml/output/xml_output.class.php b/backup/util/xml/output/xml_output.class.php index 159fbdb10b9..942c8ec1579 100644 --- a/backup/util/xml/output/xml_output.class.php +++ b/backup/util/xml/output/xml_output.class.php @@ -107,7 +107,7 @@ abstract class xml_output { if (!$this->running) { throw new xml_output_exception('xml_output_not_started'); } - $lenc = strlen($content); // Get length in bytes + $lenc = strlen($content ?? ''); // Get length in bytes if ($lenc == 0) { // 0 length contents, nothing to do return; } diff --git a/backup/util/xml/xml_writer.class.php b/backup/util/xml/xml_writer.class.php index cc1f3c26b5d..39ebeeaa73d 100644 --- a/backup/util/xml/xml_writer.class.php +++ b/backup/util/xml/xml_writer.class.php @@ -260,7 +260,7 @@ class xml_writer { * ignores the rest of characters. Also normalize linefeeds and return chars. */ protected function xml_safe_utf8($content) { - $content = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is','', $content); // clean CTRL chars + $content = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is','', $content ?? ''); // clean CTRL chars $content = preg_replace("/\r\n|\r/", "\n", $content); // Normalize line&return=>line return $content; } diff --git a/badges/classes/backpack_api_mapping.php b/badges/classes/backpack_api_mapping.php index aa9155c4b1d..4c10f672a77 100644 --- a/badges/classes/backpack_api_mapping.php +++ b/badges/classes/backpack_api_mapping.php @@ -170,8 +170,8 @@ class backpack_api_mapping { $url = str_replace('[SCHEME]', $urlscheme, $url); $url = str_replace('[HOST]', $urlhost, $url); $url = str_replace('[URL]', $apiurl, $url); - $url = str_replace('[PARAM1]', $param1, $url); - $url = str_replace('[PARAM2]', $param2, $url); + $url = str_replace('[PARAM1]', $param1 ?? '', $url); + $url = str_replace('[PARAM2]', $param2 ?? '', $url); return $url; } diff --git a/cache/classes/helper.php b/cache/classes/helper.php index cf9364e30ea..e2f8b2300df 100644 --- a/cache/classes/helper.php +++ b/cache/classes/helper.php @@ -634,7 +634,7 @@ class cache_helper { */ public static function hash_key($key, cache_definition $definition) { if ($definition->uses_simple_keys()) { - if (debugging() && preg_match('#[^a-zA-Z0-9_]#', $key)) { + if (debugging() && preg_match('#[^a-zA-Z0-9_]#', $key ?? '')) { throw new coding_exception('Cache definition '.$definition->get_id().' requires simple keys. Invalid key provided.', $key); } // We put the key first so that we can be sure the start of the key changes. diff --git a/course/classes/external/course_summary_exporter.php b/course/classes/external/course_summary_exporter.php index 0916abda094..e50f11bdf6b 100644 --- a/course/classes/external/course_summary_exporter.php +++ b/course/classes/external/course_summary_exporter.php @@ -64,7 +64,7 @@ class course_summary_exporter extends \core\external\exporter { if ($progress === 0 || $progress > 0) { $hasprogress = true; } - $progress = floor($progress); + $progress = floor($progress ?? 0); $coursecategory = \core_course_category::get($this->data->category, MUST_EXIST, true); return array( 'fullnamedisplay' => get_course_display_name_for_list($this->data), diff --git a/course/format/classes/base.php b/course/format/classes/base.php index 66b1e5f09f5..bc8a6d69ef7 100644 --- a/course/format/classes/base.php +++ b/course/format/classes/base.php @@ -608,7 +608,7 @@ abstract class base { $course = $this->get_course(); try { $sectionpreferences = (array) json_decode( - get_user_preferences('coursesectionspreferences_' . $course->id, null, $USER->id) + get_user_preferences('coursesectionspreferences_' . $course->id, null, $USER->id) ?? '' ); if (empty($sectionpreferences)) { $sectionpreferences = []; diff --git a/course/format/classes/output/local/content/frontpagesection.php b/course/format/classes/output/local/content/frontpagesection.php index 50a4b47e243..ac34c59ac6c 100644 --- a/course/format/classes/output/local/content/frontpagesection.php +++ b/course/format/classes/output/local/content/frontpagesection.php @@ -84,7 +84,7 @@ class frontpagesection implements named_templatable, renderable { $sectionoutput = new $this->sectionclass($format, $section); $sectionoutput->hide_controls(); - if (trim($section->name) == '') { + if (trim($section->name ?? '') == '') { $sectionoutput->hide_title(); } diff --git a/course/user.php b/course/user.php index c6abc6f5f20..05f61885341 100644 --- a/course/user.php +++ b/course/user.php @@ -148,7 +148,7 @@ switch ($mode) { $coursenode->collapse = true; $coursenode->make_inactive(); - if (!preg_match('/^user\d{0,}$/', $activenode->key)) { // No user name found. + if (!preg_match('/^user\d{0,}$/', $activenode->key ?? '')) { // No user name found. $userurl = new moodle_url('/user/view.php', array('id' => $user->id, 'course' => $course->id)); // Add the user name. $usernode = $activenode->add(fullname($user), $userurl, navigation_node::TYPE_SETTING); diff --git a/customfield/classes/field.php b/customfield/classes/field.php index 1f7b7e1ec2c..7bd440abce1 100644 --- a/customfield/classes/field.php +++ b/customfield/classes/field.php @@ -92,6 +92,6 @@ class field extends persistent { * @return array */ protected function get_configdata() : array { - return json_decode($this->raw_get('configdata'), true) ?? array(); + return json_decode($this->raw_get('configdata') ?? '', true) ?? array(); } } diff --git a/enrol/lti/classes/data_connector.php b/enrol/lti/classes/data_connector.php index b061da986ad..ff5a50cfb70 100644 --- a/enrol/lti/classes/data_connector.php +++ b/enrol/lti/classes/data_connector.php @@ -997,7 +997,7 @@ class data_connector extends DataConnector { $consumer->consumerName = $record->consumername; $consumer->consumerVersion = $record->consumerversion; $consumer->consumerGuid = $record->consumerguid; - $consumer->profile = json_decode($record->profile); + $consumer->profile = json_decode($record->profile ?? ''); $consumer->toolProxy = $record->toolproxy; $settings = unserialize($record->settings); if (!is_array($settings)) { diff --git a/enrol/lti/classes/local/ltiadvantage/lib/http_client.php b/enrol/lti/classes/local/ltiadvantage/lib/http_client.php index fe9fac862d8..1911ea2e1d0 100644 --- a/enrol/lti/classes/local/ltiadvantage/lib/http_client.php +++ b/enrol/lti/classes/local/ltiadvantage/lib/http_client.php @@ -78,8 +78,8 @@ class http_client implements IHttpClient { if (!$this->curlclient->get_errno() && !$this->curlclient->error) { // No errors, so format the response. $headersize = $info['header_size']; - $resheaders = substr($res, 0, $headersize); - $resbody = substr($res, $headersize); + $resheaders = substr($res ?? '', 0, $headersize); + $resbody = substr($res ?? '', $headersize); $headerlines = array_filter(explode("\r\n", $resheaders)); $parsedresponseheaders = [ 'httpstatus' => array_shift($headerlines) diff --git a/grade/report/singleview/classes/local/screen/user.php b/grade/report/singleview/classes/local/screen/user.php index 810bc5416a7..fb3a4b2a56a 100644 --- a/grade/report/singleview/classes/local/screen/user.php +++ b/grade/report/singleview/classes/local/screen/user.php @@ -410,7 +410,7 @@ class user extends tablelike implements selectable_items { $isscale = ($gradeitem->gradetype == GRADE_TYPE_SCALE); - $empties = (trim($value) === '' or ($isscale and $value == -1)); + $empties = (trim($value ?? '') === '' or ($isscale and $value == -1)); if ($filter == 'all' or $empties) { $data->$varname = ($isscale and empty($insertvalue)) ? diff --git a/h5p/ajax.php b/h5p/ajax.php index 04a3ae1d9b1..29cbba05fb0 100644 --- a/h5p/ajax.php +++ b/h5p/ajax.php @@ -59,7 +59,7 @@ switch ($action) { // Normalise Moodle language using underscore, as opposed to H5P which uses dash. $language = optional_param('default-language', null, PARAM_RAW); - $language = clean_param(str_replace('-', '_', $language), PARAM_LANG); + $language = clean_param(str_replace('-', '_', $language ?? ''), PARAM_LANG); if (!empty($name)) { $editor->ajax->action(H5PEditorEndpoints::SINGLE_LIBRARY, $name, diff --git a/h5p/classes/api.php b/h5p/classes/api.php index eda8766641a..d2e9974dad2 100644 --- a/h5p/classes/api.php +++ b/h5p/classes/api.php @@ -163,7 +163,7 @@ class api { unset($library->major_version); $library->minorVersion = (int) $library->minorversion; unset($library->minorversion); - $library->metadataSettings = json_decode($library->metadatasettings); + $library->metadataSettings = json_decode($library->metadatasettings ?? ''); // If we already add this library means that it is an old version,as the previous query was sorted by version. if (isset($added[$library->name])) { diff --git a/h5p/classes/editor_framework.php b/h5p/classes/editor_framework.php index 0ab8b956338..45296c840fb 100644 --- a/h5p/classes/editor_framework.php +++ b/h5p/classes/editor_framework.php @@ -274,7 +274,7 @@ class editor_framework implements H5peditorStorage { if ($details) { $library->title = $details->title; $library->runnable = $details->runnable; - $library->metadataSettings = json_decode($details->metadatasettings); + $library->metadataSettings = json_decode($details->metadatasettings ?? ''); $library->example = $details->example; $library->tutorial = $details->tutorial; $librariesin[] = $library; diff --git a/lib/accesslib.php b/lib/accesslib.php index f5b72000891..e403839ef25 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -4519,7 +4519,7 @@ function role_get_name(stdClass $role, $context = null, $rolenamedisplay = ROLEN } if ($rolenamedisplay == ROLENAME_ALIAS) { - if ($coursecontext and trim($role->coursealias) !== '') { + if ($coursecontext and trim($role->coursealias ?? '') !== '') { return format_string($role->coursealias, true, array('context'=>$coursecontext)); } else { return $original; @@ -4527,7 +4527,7 @@ function role_get_name(stdClass $role, $context = null, $rolenamedisplay = ROLEN } if ($rolenamedisplay == ROLENAME_BOTH) { - if ($coursecontext and trim($role->coursealias) !== '') { + if ($coursecontext and trim($role->coursealias ?? '') !== '') { return format_string($role->coursealias, true, array('context'=>$coursecontext)) . " ($original)"; } else { return $original; diff --git a/lib/behat/extension/Moodle/BehatExtension/Locator/FilesystemSkipPassedListLocator.php b/lib/behat/extension/Moodle/BehatExtension/Locator/FilesystemSkipPassedListLocator.php index 1a68338b77c..933d8a25703 100644 --- a/lib/behat/extension/Moodle/BehatExtension/Locator/FilesystemSkipPassedListLocator.php +++ b/lib/behat/extension/Moodle/BehatExtension/Locator/FilesystemSkipPassedListLocator.php @@ -64,7 +64,7 @@ final class FilesystemSkipPassedListLocator implements SpecificationLocator { * @return SpecificationIterator */ public function locateSpecifications(Suite $suite, $locator) { - if (!is_file($locator) || 'passed' !== pathinfo($locator, PATHINFO_EXTENSION)) { + if (!$locator || !is_file($locator) || 'passed' !== pathinfo($locator, PATHINFO_EXTENSION)) { return new NoSpecificationsIterator($suite); } diff --git a/lib/classes/component.php b/lib/classes/component.php index a7e8a6ce9a9..b818096fd87 100644 --- a/lib/classes/component.php +++ b/lib/classes/component.php @@ -973,7 +973,7 @@ $cache = '.var_export($cache, true).'; return (bool)preg_match('/^[a-z][a-z0-9]*$/', $pluginname); } else { - return (bool)preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]+$/', $pluginname); + return (bool)preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]+$/', $pluginname ?? ''); } } diff --git a/lib/classes/date.php b/lib/classes/date.php index 14e178ddddc..747a098faaa 100644 --- a/lib/classes/date.php +++ b/lib/classes/date.php @@ -319,7 +319,7 @@ class core_date { if (!defined('PHPUNIT_TEST')) { throw new coding_exception('core_date::phpunit_override_default_php_timezone() must be used only from unit tests'); } - $result = timezone_open($tz); // This triggers error if $tz invalid. + $result = timezone_open($tz ?? ''); // This triggers error if $tz invalid. if ($result !== false) { self::$defaultphptimezone = $tz; } else { diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index a61b4f89dec..247f371949c 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -594,7 +594,7 @@ class pgsql_native_moodle_database extends moodle_database { } else if (preg_match('/int(\d)/i', $rawcolumn->type, $matches)) { $info->type = 'int'; - if (strpos($rawcolumn->adsrc, 'nextval') === 0) { + if (strpos($rawcolumn->adsrc ?? '', 'nextval') === 0) { $info->primary_key = true; $info->meta_type = 'R'; $info->unique = true; diff --git a/lib/evalmath/evalmath.class.php b/lib/evalmath/evalmath.class.php index 26e204bb73d..7e1a8790b30 100644 --- a/lib/evalmath/evalmath.class.php +++ b/lib/evalmath/evalmath.class.php @@ -260,7 +260,7 @@ class EvalMath { if (is_null($o2)) return $this->trigger(get_string('unexpectedclosingbracket', 'mathslib')); else $output[] = $o2; } - if (preg_match('/^('.self::$namepat.')\($/', $stack->last(2), $matches)) { // did we just close a function? + if (preg_match('/^('.self::$namepat.')\($/', $stack->last(2) ?? '', $matches)) { // did we just close a function? $fnn = $matches[1]; // get the function name $arg_count = $stack->pop(); // see how many arguments there were (cleverly stored on the stack, thank you) $fn = $stack->pop(); diff --git a/lib/evalmath/readme_moodle.txt b/lib/evalmath/readme_moodle.txt index c34b31d4241..b40f83b79f0 100644 --- a/lib/evalmath/readme_moodle.txt +++ b/lib/evalmath/readme_moodle.txt @@ -13,6 +13,7 @@ Our changes: * added round, ceil and floor functions. * EvalMath::EvalMath() changed to EvalMath::__construct() and there is a new EvalMath::EvalMath function to maintain backwards compatibility +* Ensure a string is passed to preg_match in EvalMath::nfx. To see all changes diff against version 1.1, available from: http://www.phpclasses.org/browse/package/2695.html @@ -27,4 +28,4 @@ Changes by Stefan Erlachner, Thomas Niedermaier (MDL-64414): * add function or: e.g. if (or(condition_1, condition_2, ... condition_n)) * add function and: -e.g. if (and(condition_1, condition_2, ... condition_n)) \ No newline at end of file +e.g. if (and(condition_1, condition_2, ... condition_n)) diff --git a/lib/filebrowser/file_info.php b/lib/filebrowser/file_info.php index b7dcc46b47c..cba268f39b7 100644 --- a/lib/filebrowser/file_info.php +++ b/lib/filebrowser/file_info.php @@ -100,7 +100,7 @@ abstract class file_info { */ protected function build_search_files_sql($extensions, $prefix = null) { global $DB; - if (strlen($prefix)) { + if (strlen($prefix ?? '')) { $prefix = $prefix.'.'; } else { $prefix = ''; diff --git a/lib/filelib.php b/lib/filelib.php index 2fda3478737..514fb8aa3a0 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -518,9 +518,9 @@ function file_rewrite_pluginfile_urls($text, $file, $contextid, $component, $fil } if (!empty($options['reverse'])) { - return str_replace($baseurl, '@@PLUGINFILE@@/', $text); + return str_replace($baseurl, '@@PLUGINFILE@@/', $text ?? ''); } else { - return str_replace('@@PLUGINFILE@@/', $baseurl, $text); + return str_replace('@@PLUGINFILE@@/', $baseurl, $text ?? ''); } } @@ -785,7 +785,7 @@ function file_get_drafarea_files($draftitemid, $filepath = '/') { } // find the file this draft file was created from and count all references in local // system pointing to that file - $source = @unserialize($file->get_source()); + $source = @unserialize($file->get_source() ?? ''); if (isset($source->original)) { $item->refcount = $fs->search_references_count($source->original); } @@ -905,7 +905,7 @@ function file_get_submitted_draft_itemid($elname) { * @return stored_file */ function file_restore_source_field_from_draft_file($storedfile) { - $source = @unserialize($storedfile->get_source()); + $source = @unserialize($storedfile->get_source() ?? ''); if (!empty($source)) { if (is_object($source)) { $restoredsource = $source->source; @@ -1199,7 +1199,7 @@ function file_save_draft_area_files($draftitemid, $contextid, $component, $filea // Let's check if we can update this file or we need to delete and create. if ($newfile->is_directory()) { // Directories are always ok to just update. - } else if (($source = @unserialize($newfile->get_source())) && isset($source->original)) { + } else if (($source = @unserialize($newfile->get_source() ?? '')) && isset($source->original)) { // File has the 'original' - we need to update the file (it may even have not been changed at all). $original = file_storage::unpack_reference($source->original); if ($original['filename'] !== $oldfile->get_filename() || $original['filepath'] !== $oldfile->get_filepath()) { @@ -1235,7 +1235,7 @@ function file_save_draft_area_files($draftitemid, $contextid, $component, $filea // Field files.source for draftarea files contains serialised object with source and original information. // We only store the source part of it for non-draft file area. $newsource = $newfile->get_source(); - if ($source = @unserialize($newfile->get_source())) { + if ($source = @unserialize($newfile->get_source() ?? '')) { $newsource = $source->source; } if ($oldfile->get_source() !== $newsource) { @@ -1269,7 +1269,7 @@ function file_save_draft_area_files($draftitemid, $contextid, $component, $filea // the size and subdirectory tests are extra safety only, the UI should prevent it foreach ($newhashes as $file) { $file_record = array('contextid'=>$contextid, 'component'=>$component, 'filearea'=>$filearea, 'itemid'=>$itemid, 'timemodified'=>time()); - if ($source = @unserialize($file->get_source())) { + if ($source = @unserialize($file->get_source() ?? '')) { // Field files.source for draftarea files contains serialised object with source and original information. // We only store the source part of it for non-draft file area. $file_record['source'] = $source->source; @@ -1478,7 +1478,7 @@ function format_postdata_for_curlcall($postdata) { $currentdata = urlencode($k); format_array_postdata_for_curlcall($v, $currentdata, $data); } else { - $data[] = urlencode($k).'='.urlencode($v); + $data[] = urlencode($k).'='.urlencode($v ?? ''); } } $convertedpostdata = implode('&', $data); @@ -1771,7 +1771,7 @@ function mimeinfo($element, $filename) { $mimeinfo = & get_mimetypes_array(); static $iconpostfixes = array(256=>'-256', 128=>'-128', 96=>'-96', 80=>'-80', 72=>'-72', 64=>'-64', 48=>'-48', 32=>'-32', 24=>'-24', 16=>''); - $filetype = strtolower(pathinfo($filename, PATHINFO_EXTENSION)); + $filetype = strtolower(pathinfo($filename ?? '', PATHINFO_EXTENSION)); if (empty($filetype)) { $filetype = 'xxx'; // file without extension } @@ -2028,8 +2028,8 @@ function get_mimetype_description($obj, $capitalise=false) { } // MIME types may include + symbol but this is not permitted in string ids. - $safemimetype = str_replace('+', '_', $mimetype); - $safemimetypestr = str_replace('+', '_', $mimetypestr); + $safemimetype = str_replace('+', '_', $mimetype ?? ''); + $safemimetypestr = str_replace('+', '_', $mimetypestr ?? ''); $customdescription = mimeinfo('customdescription', $filename); if ($customdescription) { // Call format_string on the custom description so that multilang @@ -2919,7 +2919,7 @@ function file_overwrite_existing_draftfile(stored_file $newfile, stored_file $ex $fs = get_file_storage(); // Remember original file source field. - $source = @unserialize($existingfile->get_source()); + $source = @unserialize($existingfile->get_source() ?? ''); // Remember the original sortorder. $sortorder = $existingfile->get_sortorder(); if ($newfile->is_external_file()) { @@ -2943,7 +2943,7 @@ function file_overwrite_existing_draftfile(stored_file $newfile, stored_file $ex $newfile = $fs->create_file_from_storedfile($newfilerecord, $newfile); // Preserve original file location (stored in source field) for handling references. if (isset($source->original)) { - if (!($newfilesource = @unserialize($newfile->get_source()))) { + if (!($newfilesource = @unserialize($newfile->get_source() ?? ''))) { $newfilesource = new stdClass(); } $newfilesource->original = $source->original; diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index 71361d665d3..8fe268de6ca 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -1800,7 +1800,7 @@ class file_storage { // the latter of which can go to 100, we need to make sure that quality here is // in a safe range or PHP WILL CRASH AND DIE. You have been warned. $quality = $quality > 9 ? (int)(max(1.0, (float)$quality / 100.0) * 9.0) : $quality; - imagepng($img, NULL, $quality, NULL); + imagepng($img, NULL, $quality, PNG_NO_FILTER); break; default: @@ -2462,6 +2462,6 @@ class file_storage { * @return string The file's content hash */ public static function hash_from_string($content) { - return sha1($content); + return sha1($content ?? ''); } } diff --git a/lib/filestorage/file_system_filedir.php b/lib/filestorage/file_system_filedir.php index 8f9eb56e089..19418514937 100644 --- a/lib/filestorage/file_system_filedir.php +++ b/lib/filestorage/file_system_filedir.php @@ -426,7 +426,7 @@ class file_system_filedir extends file_system { $contenthash = file_storage::hash_from_string($content); // Binary length. - $filesize = strlen($content); + $filesize = strlen($content ?? ''); $hashpath = $this->get_fulldir_from_hash($contenthash); $hashfile = $this->get_local_path_from_hash($contenthash, false); diff --git a/lib/filestorage/tgz_extractor.php b/lib/filestorage/tgz_extractor.php index 0fb40ba38a0..4d1bc370fb2 100644 --- a/lib/filestorage/tgz_extractor.php +++ b/lib/filestorage/tgz_extractor.php @@ -493,7 +493,7 @@ class tgz_extractor { unlink($this->currentfile); } else { // For index file, get number of files and delete temp file. - $contents = file_get_contents($this->currentfile, null, null, null, 128); + $contents = file_get_contents($this->currentfile, false, null, 0, 128); $matches = array(); if (preg_match('~^' . preg_quote(tgz_packer::ARCHIVE_INDEX_COUNT_PREFIX) . '([0-9]+)~', $contents, $matches)) { diff --git a/lib/form/classes/filetypes_util.php b/lib/form/classes/filetypes_util.php index 6dc4c74ea8d..f11791bd1b7 100644 --- a/lib/form/classes/filetypes_util.php +++ b/lib/form/classes/filetypes_util.php @@ -62,7 +62,7 @@ class filetypes_util { */ public function normalize_file_types($types) { - if ($types === '') { + if ($types === '' || $types === null) { return []; } diff --git a/lib/form/passwordunmask.php b/lib/form/passwordunmask.php index e90ee4df89f..f82d12f4131 100644 --- a/lib/form/passwordunmask.php +++ b/lib/form/passwordunmask.php @@ -86,7 +86,7 @@ class MoodleQuickForm_passwordunmask extends MoodleQuickForm_password { */ public function export_for_template(renderer_base $output) { $context = parent::export_for_template($output); - $context['valuechars'] = array_fill(0, strlen($context['value']), 'x'); + $context['valuechars'] = array_fill(0, strlen($context['value'] ?? ''), 'x'); return $context; } diff --git a/lib/formslib.php b/lib/formslib.php index 7995f206d6d..4bb23d26df1 100644 --- a/lib/formslib.php +++ b/lib/formslib.php @@ -3261,7 +3261,7 @@ class MoodleQuickForm_Renderer extends HTML_QuickForm_Renderer_Tableless{ $html = str_replace('{type}', $element->getType(), $html); $html = str_replace('{name}', $element->getName(), $html); $html = str_replace('{groupname}', '', $html); - $html = str_replace('{class}', $element->getAttribute('class'), $html); + $html = str_replace('{class}', $element->getAttribute('class') ?? '', $html); $emptylabel = ''; if ($element->getLabel() == '') { $emptylabel = 'femptylabel'; diff --git a/lib/html2text/lib.php b/lib/html2text/lib.php index 18972d4c700..6c4588e22a6 100644 --- a/lib/html2text/lib.php +++ b/lib/html2text/lib.php @@ -52,7 +52,7 @@ class core_html2text extends \Html2Text\Html2Text { */ function __construct($html = '', $options = array()) { // Call the parent constructor. - parent::__construct($html, $options); + parent::__construct($html ?? '', $options); // MDL-27736: Trailing spaces before newline or tab. $this->entSearch[] = '/[ ]+([\n\t])/'; diff --git a/lib/mathslib.php b/lib/mathslib.php index 8753695cc16..0b33da3b9d7 100644 --- a/lib/mathslib.php +++ b/lib/mathslib.php @@ -123,7 +123,7 @@ class calc_formula { * @return string localised formula */ public static function localize($formula) { - $formula = str_replace('.', '$', $formula); // temp placeholder + $formula = str_replace('.', '$', $formula ?? ''); // temp placeholder $formula = str_replace(',', get_string('listsep', 'langconfig'), $formula); $formula = str_replace('$', get_string('decsep', 'langconfig'), $formula); return $formula; diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 2209a3efaf6..eb5f5dcd730 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -5836,7 +5836,7 @@ class navigation_json { } if ($child->forcetitle || $child->title !== $child->text) { - $attributes['title'] = htmlentities($child->title, ENT_QUOTES, 'UTF-8'); + $attributes['title'] = htmlentities($child->title ?? '', ENT_QUOTES, 'UTF-8'); } if (array_key_exists($child->key.':'.$child->type, $this->expandable)) { $attributes['expandable'] = $child->key; diff --git a/lib/outputcomponents.php b/lib/outputcomponents.php index 850d4b2bb75..7ad7e852f2b 100644 --- a/lib/outputcomponents.php +++ b/lib/outputcomponents.php @@ -2352,7 +2352,7 @@ class html_writer { if (!is_null($for)) { $attributes = array_merge($attributes, array('for' => $for)); } - $text = trim($text); + $text = trim($text ?? ''); $label = self::tag('label', $text, $attributes); // TODO MDL-12192 $colonize disabled for now yet diff --git a/lib/outputlib.php b/lib/outputlib.php index 6dc24b30054..1bbd202dfd2 100644 --- a/lib/outputlib.php +++ b/lib/outputlib.php @@ -1854,7 +1854,7 @@ class theme_config { // Now resolve all theme settings or do any other postprocessing. // This needs to be done before calling core parser, since the parser strips [[settings]] tags. $csspostprocess = $this->csspostprocess; - if (function_exists($csspostprocess)) { + if ($csspostprocess && function_exists($csspostprocess)) { $css = $csspostprocess($css, $this); } diff --git a/lib/setuplib.php b/lib/setuplib.php index 10e002a89bb..6e7c6f0f366 100644 --- a/lib/setuplib.php +++ b/lib/setuplib.php @@ -634,6 +634,7 @@ function get_docs_url($path = null) { $path = ''; } + $path = $path ?? ''; // Absolute URLs are used unmodified. if (substr($path, 0, 7) === 'http://' || substr($path, 0, 8) === 'https://') { return $path; diff --git a/lib/tablelib.php b/lib/tablelib.php index f81bb10717d..bf099cd9066 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -1463,7 +1463,7 @@ class flexible_table { // Load any existing user preferences. if ($this->persistent) { - $this->prefs = json_decode(get_user_preferences('flextable_' . $this->uniqueid), true); + $this->prefs = json_decode(get_user_preferences('flextable_' . $this->uniqueid) ?? '', true); $oldprefs = $this->prefs; } else if (isset($SESSION->flextable[$this->uniqueid])) { $this->prefs = $SESSION->flextable[$this->uniqueid]; diff --git a/lib/tests/behat/behat_accessibility.php b/lib/tests/behat/behat_accessibility.php index 7aa8d426f3f..022a7aed7f1 100644 --- a/lib/tests/behat/behat_accessibility.php +++ b/lib/tests/behat/behat_accessibility.php @@ -131,7 +131,7 @@ return (() => { EOF; for ($i = 0; $i < self::get_extended_timeout() * 10; $i++) { - $results = json_decode($this->evaluate_script($getresults)); + $results = json_decode($this->evaluate_script($getresults) ?? ''); if ($results) { break; } diff --git a/lib/weblib.php b/lib/weblib.php index 488c717ee6e..27ac693e402 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -97,7 +97,7 @@ function s($var) { } return preg_replace('/&#(\d+|x[0-9a-f]+);/i', '&#$1;', - htmlspecialchars($var, ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE)); + htmlspecialchars($var ?? '', ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE)); } /** @@ -149,7 +149,7 @@ function addslashes_js($var) { */ function strip_querystring($url) { - if ($commapos = strpos($url, '?')) { + if ($commapos = strpos($url ?? '', '?')) { return substr($url, 0, $commapos); } else { return $url; @@ -336,6 +336,7 @@ class moodle_url { $this->anchor = $url->anchor; } else { + $url = $url ?? ''; // Detect if anchor used. $apos = strpos($url, '#'); if ($apos !== false) { @@ -1115,7 +1116,7 @@ function validate_email($address) { require_once("{$CFG->libdir}/phpmailer/moodle_phpmailer.php"); - return moodle_phpmailer::validateAddress($address) && !preg_match('/[<>]/', $address); + return moodle_phpmailer::validateAddress($address ?? '') && !preg_match('/[<>]/', $address); } /** @@ -1528,7 +1529,7 @@ function format_string($string, $striplinks = true, $options = null) { * @return string */ function replace_ampersands_not_followed_by_entity($string) { - return preg_replace("/\&(?![a-zA-Z0-9#]{1,8};)/", "&", $string); + return preg_replace("/\&(?![a-zA-Z0-9#]{1,8};)/", "&", $string ?? ''); } /** diff --git a/media/player/youtube/classes/plugin.php b/media/player/youtube/classes/plugin.php index dd1478981e5..9537ae94138 100644 --- a/media/player/youtube/classes/plugin.php +++ b/media/player/youtube/classes/plugin.php @@ -63,7 +63,7 @@ class media_youtube_plugin extends core_media_player_external { protected function embed_external(moodle_url $url, $name, $width, $height, $options) { - $info = trim($name); + $info = trim($name ?? ''); if (empty($info) or strpos($info, 'http') === 0) { $info = get_string('pluginname', 'media_youtube'); } @@ -125,7 +125,7 @@ OET; if (is_numeric($rawtime)) { // Start time already specified as a number of seconds; ensure it's an integer. $seconds = $rawtime; - } else if (preg_match('/(\d+?h)?(\d+?m)?(\d+?s)?/i', $rawtime, $matches)) { + } else if (preg_match('/(\d+?h)?(\d+?m)?(\d+?s)?/i', $rawtime ?? '', $matches)) { // Convert into a raw number of seconds, as that's all embedded players accept. for ($i = 1; $i < count($matches); $i++) { if (empty($matches[$i])) { diff --git a/mod/data/classes/preset.php b/mod/data/classes/preset.php index fdeef6d8182..8f2c5ec1935 100644 --- a/mod/data/classes/preset.php +++ b/mod/data/classes/preset.php @@ -310,7 +310,7 @@ class preset { $instance = $this->manager->get_instance(); foreach (manager::TEMPLATES_LIST as $templatename => $templatefilename) { $templatefile = fopen("$exportdir/$templatefilename", 'w'); - fwrite($templatefile, $instance->{$templatename}); + fwrite($templatefile, $instance->{$templatename} ?? ''); fclose($templatefile); } @@ -710,7 +710,7 @@ class preset { $presetxmldata = "\n\n"; // Add description. - $presetxmldata .= '' . htmlspecialchars($this->description, ENT_COMPAT) . "\n\n"; + $presetxmldata .= '' . htmlspecialchars($this->description ?? '', ENT_COMPAT) . "\n\n"; // Add settings. // Raw settings are not preprocessed during saving of presets. diff --git a/mod/data/classes/search/entry.php b/mod/data/classes/search/entry.php index 95ee8a0a95c..7cf20be4d71 100644 --- a/mod/data/classes/search/entry.php +++ b/mod/data/classes/search/entry.php @@ -304,7 +304,7 @@ class entry extends \core_search\base_mod { continue; } $content->priority = $classname::get_priority(); - $content->addtemplateposition = strpos($template, '[['.$content->fldname.']]'); + $content->addtemplateposition = strpos($template ?? '', '[['.$content->fldname.']]'); } $orderqueue = new \SPLPriorityQueue(); diff --git a/mod/data/field/latlong/field.class.php b/mod/data/field/latlong/field.class.php index 19d094ee2fd..4d25d261ecc 100644 --- a/mod/data/field/latlong/field.class.php +++ b/mod/data/field/latlong/field.class.php @@ -170,11 +170,11 @@ class data_field_latlong extends data_field_base { } $lat = $content->content; - if (strlen($lat) < 1) { + if (strlen($lat ?? '') < 1) { return ''; } $long = $content->content1; - if (strlen($long) < 1) { + if (strlen($long ?? '') < 1) { return ''; } // We use format_float to display in the regional format. @@ -254,7 +254,7 @@ class data_field_latlong extends data_field_base { // When updating these values (which might be region formatted) we should format // the float to allow for a consistent float format in the database. $value = unformat_float($value); - $value = trim($value); + $value = trim($value ?? ''); if (strlen($value) > 0) { $value = floatval($value); } else { diff --git a/mod/data/field/number/field.class.php b/mod/data/field/number/field.class.php index df75243ad8f..e3053c8ba5b 100644 --- a/mod/data/field/number/field.class.php +++ b/mod/data/field/number/field.class.php @@ -68,7 +68,7 @@ class data_field_number extends data_field_base { return ''; } $number = $content->content; - $decimals = trim($this->field->param1); + $decimals = trim($this->field->param1 ?? ''); // Only apply number formatting if param1 contains an integer number >= 0. if (preg_match("/^\d+$/", $decimals)) { $decimals = $decimals * 1; diff --git a/mod/data/lib.php b/mod/data/lib.php index 355483a30f7..9ab3f3c45a8 100644 --- a/mod/data/lib.php +++ b/mod/data/lib.php @@ -840,19 +840,19 @@ function data_replace_field_in_templates($data, $searchfieldname, $newfieldname) $newdata = new stdClass(); $newdata->id = $data->id; $newdata->singletemplate = str_ireplace('[['.$searchfieldname.']]', - $prestring.$newfieldname.$poststring, $data->singletemplate); + $prestring.$newfieldname.$poststring, $data->singletemplate ?? ''); $newdata->listtemplate = str_ireplace('[['.$searchfieldname.']]', - $prestring.$newfieldname.$poststring, $data->listtemplate); + $prestring.$newfieldname.$poststring, $data->listtemplate ?? ''); $newdata->addtemplate = str_ireplace('[['.$searchfieldname.']]', - $prestring.$newfieldname.$poststring, $data->addtemplate); + $prestring.$newfieldname.$poststring, $data->addtemplate ?? ''); $newdata->addtemplate = str_ireplace('[['.$searchfieldname.'#id]]', - $prestring.$newfieldname.$idpart.$poststring, $data->addtemplate); + $prestring.$newfieldname.$idpart.$poststring, $data->addtemplate ?? ''); $newdata->rsstemplate = str_ireplace('[['.$searchfieldname.']]', - $prestring.$newfieldname.$poststring, $data->rsstemplate); + $prestring.$newfieldname.$poststring, $data->rsstemplate ?? ''); return $DB->update_record('data', $newdata); } diff --git a/mod/h5pactivity/classes/xapi/handler.php b/mod/h5pactivity/classes/xapi/handler.php index b05a9102377..358f6a52eea 100644 --- a/mod/h5pactivity/classes/xapi/handler.php +++ b/mod/h5pactivity/classes/xapi/handler.php @@ -83,7 +83,7 @@ class handler extends handler_base { // H5P add some extra params to ID to define subcontents. $parts = explode('?', $xapiobject, 2); $contextid = array_shift($parts); - $subcontent = str_replace('subContentId=', '', array_shift($parts)); + $subcontent = str_replace('subContentId=', '', array_shift($parts) ?? ''); if (empty($contextid) || !is_numeric($contextid)) { return null; } diff --git a/mod/lti/lib.php b/mod/lti/lib.php index bfe7cd71e28..c2a74c11e99 100644 --- a/mod/lti/lib.php +++ b/mod/lti/lib.php @@ -318,7 +318,7 @@ function mod_lti_get_all_content_items(\core_course\local\entity\content_item $d $type->name = 'lti_type_' . $ltitype->id; // Clean the name. We don't want tags here. $type->title = clean_param($ltitype->name, PARAM_NOTAGS); - $trimmeddescription = trim($ltitype->description); + $trimmeddescription = trim($ltitype->description ?? ''); $type->help = ''; if ($trimmeddescription != '') { // Clean the description. We don't want tags here. diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index 7bd5073ebd4..0a1ef823d57 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -2437,7 +2437,7 @@ function lti_get_configured_types($courseid, $sectionreturn = 0) { $type->name = 'lti_type_' . $ltitype->id; // Clean the name. We don't want tags here. $type->title = clean_param($ltitype->name, PARAM_NOTAGS); - $trimmeddescription = trim($ltitype->description); + $trimmeddescription = trim($ltitype->description ?? ''); if ($trimmeddescription != '') { // Clean the description. We don't want tags here. $type->help = clean_param($trimmeddescription, PARAM_NOTAGS); @@ -2454,7 +2454,7 @@ function lti_get_configured_types($courseid, $sectionreturn = 0) { function lti_get_domain_from_url($url) { $matches = array(); - if (preg_match(LTI_URL_DOMAIN_REGEX, $url, $matches)) { + if (preg_match(LTI_URL_DOMAIN_REGEX, $url ?? '', $matches)) { return $matches[1]; } } diff --git a/mod/quiz/accessrule/seb/classes/quiz_settings.php b/mod/quiz/accessrule/seb/classes/quiz_settings.php index 923fdb33d6d..a1512ad832b 100644 --- a/mod/quiz/accessrule/seb/classes/quiz_settings.php +++ b/mod/quiz/accessrule/seb/classes/quiz_settings.php @@ -662,7 +662,7 @@ class quiz_settings extends persistent { * @return array of string, the separate keys. */ private function split_keys($keys) : array { - $keys = preg_split('~[ \t\n\r,;]+~', $keys, -1, PREG_SPLIT_NO_EMPTY); + $keys = preg_split('~[ \t\n\r,;]+~', $keys ?? '', -1, PREG_SPLIT_NO_EMPTY); foreach ($keys as $i => $key) { $keys[$i] = strtolower($key); } diff --git a/mod/survey/classes/privacy/provider.php b/mod/survey/classes/privacy/provider.php index b545e4b7883..d31ffb99c1b 100644 --- a/mod/survey/classes/privacy/provider.php +++ b/mod/survey/classes/privacy/provider.php @@ -227,7 +227,7 @@ class provider implements 'options' => $record->qoptions ]); $qtype = $record->qtype; - $options = explode(',', $q->options); + $options = explode(',', $q->options ?? ''); $carry[] = [ 'question' => array_merge((array) $q, [ diff --git a/mod/workshop/classes/portfolio_caller.php b/mod/workshop/classes/portfolio_caller.php index 740a041822e..eb618fb7192 100644 --- a/mod/workshop/classes/portfolio_caller.php +++ b/mod/workshop/classes/portfolio_caller.php @@ -342,7 +342,7 @@ class mod_workshop_portfolio_caller extends portfolio_module_caller_base { } if ($this->workshop->overallfeedbackmode) { - if ($assessment->feedbackauthorattachment or trim($assessment->feedbackauthor) !== '') { + if ($assessment->feedbackauthorattachment or trim($assessment->feedbackauthor ?? '') !== '') { $output .= html_writer::tag('h3', get_string('overallfeedback', 'mod_workshop')); $content = $this->format_exported_text($assessment->feedbackauthor, $assessment->feedbackauthorformat); $content = portfolio_rewrite_pluginfile_urls($content, $this->workshop->context->id, 'mod_workshop', diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index 88c95c434bc..c50720a6b45 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -179,7 +179,7 @@ class moodle_content_writer implements content_writer { $returnstring = $path . DIRECTORY_SEPARATOR . $this->get_files_target_url($component, $filearea, $itemid) . '/'; $returnstring = clean_param($returnstring, PARAM_PATH); - return str_replace('@@PLUGINFILE@@/', $returnstring, $text); + return str_replace('@@PLUGINFILE@@/', $returnstring, $text ?? ''); } /** diff --git a/privacy/classes/tests/request/content_writer.php b/privacy/classes/tests/request/content_writer.php index 7b2b9b85847..c6e9d3d080f 100644 --- a/privacy/classes/tests/request/content_writer.php +++ b/privacy/classes/tests/request/content_writer.php @@ -382,7 +382,7 @@ class content_writer implements \core_privacy\local\request\content_writer { * @return string The processed string */ public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { - return str_replace('@@PLUGINFILE@@/', 'files/', $text); + return str_replace('@@PLUGINFILE@@/', 'files/', $text ?? ''); } /** diff --git a/question/engine/renderer.php b/question/engine/renderer.php index 4ce0e7c193c..b2f77b7e5f9 100644 --- a/question/engine/renderer.php +++ b/question/engine/renderer.php @@ -149,7 +149,7 @@ class core_question_renderer extends plugin_renderer_base { * @return HTML fragment. */ protected function number($number) { - if (trim($number) === '') { + if (trim($number ?? '') === '') { return ''; } $numbertext = ''; diff --git a/question/type/calculated/db/upgradelib.php b/question/type/calculated/db/upgradelib.php index 09803be12d7..9c8b2d7bbab 100644 --- a/question/type/calculated/db/upgradelib.php +++ b/question/type/calculated/db/upgradelib.php @@ -270,7 +270,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update * corresponding value. */ protected function substitute_values_for_eval($expression) { - return str_replace($this->search, $this->safevalue, $expression); + return str_replace($this->search ?? '', $this->safevalue ?? '', $expression ?? ''); } /** @@ -282,7 +282,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update * corresponding value. */ protected function substitute_values_pretty($text) { - return str_replace($this->search, $this->prettyvalue, $text); + return str_replace($this->search ?? '', $this->prettyvalue ?? '', $text ?? ''); } /** @@ -296,7 +296,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update $text = preg_replace_callback('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', function ($matches) use ($vs, $format, $length) { return $vs->format_float($vs->calculate($matches[1]), $length, $format); - }, $text); + }, $text ?? ''); return $this->substitute_values_pretty($text); } } diff --git a/question/type/calculatedmulti/db/upgradelib.php b/question/type/calculatedmulti/db/upgradelib.php index d17d43c1aa6..543fbbf8b65 100644 --- a/question/type/calculatedmulti/db/upgradelib.php +++ b/question/type/calculatedmulti/db/upgradelib.php @@ -294,7 +294,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u * corresponding value. */ protected function substitute_values_for_eval($expression) { - return str_replace($this->search, $this->safevalue, $expression); + return str_replace($this->search ?? '', $this->safevalue ?? '', $expression ?? ''); } /** @@ -306,7 +306,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u * corresponding value. */ protected function substitute_values_pretty($text) { - return str_replace($this->search, $this->prettyvalue, $text); + return str_replace($this->search ?? '', $this->prettyvalue ?? '', $text ?? ''); } /** @@ -320,7 +320,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u $text = preg_replace_callback(qtype_calculated::FORMULAS_IN_TEXT_REGEX, function ($matches) use ($vs, $format, $length) { return $vs->format_float($vs->calculate($matches[1]), $length, $format); - }, $text); + }, $text ?? ''); return $this->substitute_values_pretty($text); } } diff --git a/question/type/multianswer/renderer.php b/question/type/multianswer/renderer.php index aa4a444bec3..a935ed9bd5a 100644 --- a/question/type/multianswer/renderer.php +++ b/question/type/multianswer/renderer.php @@ -235,7 +235,7 @@ class qtype_multianswer_textfield_renderer extends qtype_multianswer_subq_render } // Work out a good input field size. - $size = max(1, core_text::strlen(trim($response)) + 1); + $size = max(1, core_text::strlen(trim($response ?? '')) + 1); foreach ($subq->answers as $ans) { $size = max($size, core_text::strlen(trim($ans->answer))); } diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index ccf62b1bea6..3aa8a2e8f9e 100644 --- a/question/type/numerical/questiontype.php +++ b/question/type/numerical/questiontype.php @@ -653,7 +653,7 @@ class qtype_numerical_answer_processor { public function apply_units($response, $separateunit = null) { // Strip spaces (which may be thousands separators) and change other forms // of writing e to e. - $response = str_replace(' ', '', $response); + $response = str_replace(' ', '', $response ?? ''); $response = preg_replace('~(?:e|E|(?:x|\*|×)10(?:\^|\*\*))([+-]?\d+)~', 'e$1', $response); // If a . is present or there are multiple , (i.e. 2,456,789 ) assume , From 5fbbb51882e3a5ee4fe2cd454efd61dbd76711b0 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 21:10:51 +0800 Subject: [PATCH 03/22] MDL-76362 core: Use empty default string when getting prefs The json_decode function does not accept a null, which is the traditional default for get_user_preferences. By passing a default of am empty string we avoid issues in PHP 8.1. --- course/format/classes/base.php | 2 +- lib/tablelib.php | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/course/format/classes/base.php b/course/format/classes/base.php index bc8a6d69ef7..26000882a46 100644 --- a/course/format/classes/base.php +++ b/course/format/classes/base.php @@ -608,7 +608,7 @@ abstract class base { $course = $this->get_course(); try { $sectionpreferences = (array) json_decode( - get_user_preferences('coursesectionspreferences_' . $course->id, null, $USER->id) ?? '' + get_user_preferences("coursesectionspreferences_{$course->id}", '', $USER->id) ); if (empty($sectionpreferences)) { $sectionpreferences = []; diff --git a/lib/tablelib.php b/lib/tablelib.php index bf099cd9066..b9b593a7ffa 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -573,7 +573,7 @@ class flexible_table { global $SESSION; if (isset($SESSION->flextable[$uniqueid])) { $prefs = $SESSION->flextable[$uniqueid]; - } else if (!$prefs = json_decode(get_user_preferences('flextable_' . $uniqueid), true)) { + } else if (!$prefs = json_decode(get_user_preferences("flextable_{$uniqueid}", ''), true)) { return ''; } @@ -1463,7 +1463,7 @@ class flexible_table { // Load any existing user preferences. if ($this->persistent) { - $this->prefs = json_decode(get_user_preferences('flextable_' . $this->uniqueid) ?? '', true); + $this->prefs = json_decode(get_user_preferences("flextable_{$this->uniqueid}", ''), true); $oldprefs = $this->prefs; } else if (isset($SESSION->flextable[$this->uniqueid])) { $this->prefs = $SESSION->flextable[$this->uniqueid]; @@ -2346,4 +2346,3 @@ class table_dataformat_export_format extends table_default_export_format_parent exit(); } } - From 745080671b5a22bc991fb09e58cc24dc9a084197 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 21:20:20 +0800 Subject: [PATCH 04/22] MDL-76362 question: Refactor question number unit tests --- question/engine/tests/coverage.php | 37 ++ question/engine/tests/helpers.php | 19 - .../engine/tests/question_engine_test.php | 359 +++++++++++++----- question/tests/coverage.php | 40 ++ 4 files changed, 345 insertions(+), 110 deletions(-) create mode 100644 question/engine/tests/coverage.php create mode 100644 question/tests/coverage.php diff --git a/question/engine/tests/coverage.php b/question/engine/tests/coverage.php new file mode 100644 index 00000000000..2589273d3dc --- /dev/null +++ b/question/engine/tests/coverage.php @@ -0,0 +1,37 @@ +. + +defined('MOODLE_INTERNAL') || die(); + +/** + * Coverage information for the question_engine. + * + * @copyright 2022 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +return new class extends phpunit_coverage_info { + /** @var array The list of files relative to the plugin root to include in coverage generation. */ + protected $includelistfiles = [ + 'bank.php', + 'datalib.php', + 'lib.php', + 'questionattempt.php', + 'questionattemptstep.php', + 'questionusage.php', + 'renderer.php', + 'states.php', + ]; +}; diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 33d9894c01a..c7da45ae584 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -1391,22 +1391,3 @@ class question_test_recordset extends moodle_recordset { $this->records = null; } } - -/** - * Helper class for tests that help to test core_question_renderer. - * - * @copyright 2018 Huong Nguyen - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ -class testable_core_question_renderer extends core_question_renderer { - - /** - * Test the private number function. - * - * @param null|string $number - * @return HTML - */ - public function number($number) { - return parent::number($number); - } -} diff --git a/question/engine/tests/question_engine_test.php b/question/engine/tests/question_engine_test.php index a46be4b1fc7..3f6d433bf76 100644 --- a/question/engine/tests/question_engine_test.php +++ b/question/engine/tests/question_engine_test.php @@ -28,18 +28,18 @@ namespace core_question; use advanced_testcase; use moodle_exception; use question_engine; -use testable_core_question_renderer; /** * Unit tests for the question_engine class. * * @copyright 2009 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \question_engine */ class question_engine_test extends advanced_testcase { /** - * Load required libraries + * Load required libraries. */ public static function setUpBeforeClass(): void { global $CFG; @@ -47,113 +47,290 @@ class question_engine_test extends advanced_testcase { require_once("{$CFG->dirroot}/question/engine/lib.php"); } - public function test_load_behaviour_class() { - // Exercise SUT + /** + * Tests for load_behaviour_class. + * + * @covers \question_engine::load_behaviour_class + */ + public function test_load_behaviour_class(): void { + // Exercise SUT. question_engine::load_behaviour_class('deferredfeedback'); - // Verify + + // Verify. $this->assertTrue(class_exists('qbehaviour_deferredfeedback')); } - public function test_load_behaviour_class_missing() { - // Exercise SUT + /** + * Tests for load_behaviour_class when a class is missing. + * + * @covers \question_engine::load_behaviour_class + */ + public function test_load_behaviour_class_missing(): void { + // Exercise SUT. $this->expectException(moodle_exception::class); question_engine::load_behaviour_class('nonexistantbehaviour'); } - public function test_get_behaviour_unused_display_options() { - $this->assertEquals(array(), question_engine::get_behaviour_unused_display_options('interactive')); - $this->assertEquals(array('correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'), - question_engine::get_behaviour_unused_display_options('deferredfeedback')); - $this->assertEquals(array('correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'), - question_engine::get_behaviour_unused_display_options('deferredcbm')); - $this->assertEquals(array('correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'), - question_engine::get_behaviour_unused_display_options('manualgraded')); + /** + * Test the get_behaviour_unused_display_options with various options. + * + * @covers \question_engine::get_behaviour_unused_display_options + * @dataProvider get_behaviour_unused_display_options_provider + * @param string $behaviour + * @param array $expected + */ + public function test_get_behaviour_unused_display_options(string $behaviour, array $expected): void { + $this->assertEquals($expected, question_engine::get_behaviour_unused_display_options($behaviour)); } - public function test_can_questions_finish_during_the_attempt() { - $this->assertFalse(question_engine::can_questions_finish_during_the_attempt('deferredfeedback')); - $this->assertTrue(question_engine::can_questions_finish_during_the_attempt('interactive')); + /** + * Data provider for get_behaviour_unused_display_options. + * + * @return array + */ + public function get_behaviour_unused_display_options_provider(): array { + return [ + 'interactive' => [ + 'interactive', + [], + ], + 'deferredfeedback' => [ + 'deferredfeedback', + ['correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'], + ], + 'deferredcbm' => [ + 'deferredcbm', + ['correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'], + ], + 'manualgraded' => [ + 'manualgraded', + ['correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'], + ], + ]; } - public function test_sort_behaviours() { - $in = array('b1' => 'Behave 1', 'b2' => 'Behave 2', 'b3' => 'Behave 3', 'b4' => 'Behave 4', 'b5' => 'Behave 5', 'b6' => 'Behave 6'); - - $out = array('b1' => 'Behave 1', 'b2' => 'Behave 2', 'b3' => 'Behave 3', 'b4' => 'Behave 4', 'b5' => 'Behave 5', 'b6' => 'Behave 6'); - $this->assertSame($out, question_engine::sort_behaviours($in, '', '', '')); - - $this->assertSame($out, question_engine::sort_behaviours($in, '', 'b4', 'b4')); - - $out = array('b4' => 'Behave 4', 'b5' => 'Behave 5', 'b6' => 'Behave 6'); - $this->assertSame($out, question_engine::sort_behaviours($in, '', 'b1,b2,b3,b4', 'b4')); - - $out = array('b6' => 'Behave 6', 'b1' => 'Behave 1', 'b4' => 'Behave 4'); - $this->assertSame($out, question_engine::sort_behaviours($in, 'b6,b1,b4', 'b2,b3,b4,b5', 'b4')); - - $out = array('b6' => 'Behave 6', 'b5' => 'Behave 5', 'b4' => 'Behave 4'); - $this->assertSame($out, question_engine::sort_behaviours($in, 'b6,b5,b4', 'b1,b2,b3', 'b4')); - - $out = array('b6' => 'Behave 6', 'b5' => 'Behave 5', 'b4' => 'Behave 4'); - $this->assertSame($out, question_engine::sort_behaviours($in, 'b1,b6,b5', 'b1,b2,b3,b4', 'b4')); - - $out = array('b2' => 'Behave 2', 'b4' => 'Behave 4', 'b6' => 'Behave 6'); - $this->assertSame($out, question_engine::sort_behaviours($in, 'b2,b4,b6', 'b1,b3,b5', 'b2')); - - // Ignore unknown input in the order argument. - $this->assertSame($in, question_engine::sort_behaviours($in, 'unknown', '', '')); - - // Ignore unknown input in the disabled argument. - $this->assertSame($in, question_engine::sort_behaviours($in, '', 'unknown', '')); + /** + * Tests for can_questions_finish_during_the_attempt. + * + * @covers \question_engine::can_questions_finish_during_the_attempt + * @dataProvider can_questions_finish_during_the_attempt_provider + * @param string $behaviour + * @param bool $expected + */ + public function test_can_questions_finish_during_the_attempt(string $behaviour, bool $expected): void { + $this->assertEquals($expected, question_engine::can_questions_finish_during_the_attempt($behaviour)); } - public function test_is_manual_grade_in_range() { - $_POST[] = array('q1:2_-mark' => 0.5, 'q1:2_-maxmark' => 1.0, - 'q1:2_:minfraction' => 0, 'q1:2_:maxfraction' => 1); + /** + * Data provider for can_questions_finish_during_the_attempt_provider. + * + * @return array + */ + public function can_questions_finish_during_the_attempt_provider(): array { + return [ + ['deferredfeedback', false], + ['interactive', true], + ]; + } + + /** + * Tests for sort_behaviours + * + * @covers \question_engine::sort_behaviours + * @dataProvider sort_behaviours_provider + * @param array $input The params passed to sort_behaviours + * @param array $expected + */ + public function test_sort_behaviours(array $input, array $expected): void { + $this->assertSame($expected, question_engine::sort_behaviours(...$input)); + } + + /** + * Data provider for sort_behaviours. + * + * @return array + */ + public function sort_behaviours_provider(): array { + $in = [ + 'b1' => 'Behave 1', + 'b2' => 'Behave 2', + 'b3' => 'Behave 3', + 'b4' => 'Behave 4', + 'b5' => 'Behave 5', + 'b6' => 'Behave 6', + ]; + + return [ + [ + [$in, '', '', ''], + $in, + ], + [ + [$in, '', 'b4', 'b4'], + $in, + ], + [ + [$in, '', 'b1,b2,b3,b4', 'b4'], + ['b4' => 'Behave 4', 'b5' => 'Behave 5', 'b6' => 'Behave 6'], + ], + [ + [$in, 'b6,b1,b4', 'b2,b3,b4,b5', 'b4'], + ['b6' => 'Behave 6', 'b1' => 'Behave 1', 'b4' => 'Behave 4'], + ], + [ + [$in, 'b6,b5,b4', 'b1,b2,b3', 'b4'], + ['b6' => 'Behave 6', 'b5' => 'Behave 5', 'b4' => 'Behave 4'], + ], + [ + [$in, 'b1,b6,b5', 'b1,b2,b3,b4', 'b4'], + ['b6' => 'Behave 6', 'b5' => 'Behave 5', 'b4' => 'Behave 4'], + ], + [ + [$in, 'b2,b4,b6', 'b1,b3,b5', 'b2'], + ['b2' => 'Behave 2', 'b4' => 'Behave 4', 'b6' => 'Behave 6'], + ], + // Ignore unknown input in the order argument. + [ + [$in, 'unknown', '', ''], + $in, + ], + // Ignore unknown input in the disabled argument. + [ + [$in, '', 'unknown', ''], + $in, + ], + ]; + } + + /** + * Tests for is_manual_grade_in_range. + * + * @dataProvider is_manual_grade_in_range_provider + * @covers \question_engine::is_manual_grade_in_range + * @param array $post The values to add to $_POST + * @param array $params The params to pass to is_manual_grade_in_range + * @param bool $expected + */ + public function test_is_manual_grade_in_range(array $post, array $params, bool $expected): void { + $_POST[] = $post; + $this->assertEquals($expected, question_engine::is_manual_grade_in_range(...$params)); + } + + /** + * Data provider for is_manual_grade_in_range tests. + * + * @return array + */ + public function is_manual_grade_in_range_provider(): array { + return [ + 'In range' => [ + 'post' => [ + 'q1:2_-mark' => 0.5, + 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => 0, + 'q1:2_:maxfraction' => 1, + ], + 'range' => [1, 2], + 'expected' => true, + ], + 'Bottom end' => [ + 'post' => [ + 'q1:2_-mark' => -1.0, + 'q1:2_-maxmark' => 2.0, + 'q1:2_:minfraction' => -0.5, + 'q1:2_:maxfraction' => 1, + ], + 'range' => [1, 2], + 'expected' => true, + ], + 'Too low' => [ + 'post' => [ + 'q1:2_-mark' => -1.1, + 'q1:2_-maxmark' => 2.0, + 'q1:2_:minfraction' => -0.5, + 'q1:2_:maxfraction' => 1, + ], + 'range' => [1, 2], + 'expected' => true, + ], + 'Top end' => [ + 'post' => [ + 'q1:2_-mark' => 3.0, + 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => -6.0, + 'q1:2_:maxfraction' => 3.0, + ], + 'range' => [1, 2], + 'expected' => true, + ], + 'Too high' => [ + 'post' => [ + 'q1:2_-mark' => 3.1, + 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => -6.0, + 'q1:2_:maxfraction' => 3.0, + ], + 'range' => [1, 2], + 'expected' => true, + ], + ]; + } + + /** + * Tests for is_manual_grade_in_range. + * + * @covers \question_engine::is_manual_grade_in_range + */ + public function test_is_manual_grade_in_range_ungraded(): void { $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); } - public function test_is_manual_grade_in_range_bottom_end() { - $_POST[] = array('q1:2_-mark' => -1.0, 'q1:2_-maxmark' => 2.0, - 'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1); - $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + /** + * Ensure that the number renderer performs as expected. + * + * @covers \core_question_renderer::number + * @dataProvider render_question_number_provider + * @param mixed $value + * @param string $expected + */ + public function test_render_question_number($value, string $expected): void { + global $PAGE; + + $renderer = new \core_question_renderer($PAGE, 'core_question'); + $rc = new \ReflectionClass($renderer); + $rcm = $rc->getMethod('number'); + $rcm->setAccessible(true); + + $this->assertEquals($expected, $rcm->invoke($renderer, $value)); } - public function test_is_manual_grade_in_range_too_low() { - $_POST[] = array('q1:2_-mark' => -1.1, 'q1:2_-maxmark' => 2.0, - 'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1); - $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); - } - - public function test_is_manual_grade_in_range_top_end() { - $_POST[] = array('q1:2_-mark' => 3.0, 'q1:2_-maxmark' => 1.0, - 'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0); - $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); - } - - public function test_is_manual_grade_in_range_too_high() { - $_POST[] = array('q1:2_-mark' => 3.1, 'q1:2_-maxmark' => 1.0, - 'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0); - $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); - } - - public function test_is_manual_grade_in_range_ungraded() { - $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); - } - - public function test_render_question_number() { - global $CFG, $PAGE; - - require_once("{$CFG->dirroot}/question/engine/tests/helpers.php"); - $renderer = new testable_core_question_renderer($PAGE, 'core_question'); - - // Test with number is i character. - $this->assertEquals('

Information

', $renderer->number('i')); - // Test with number is empty string. - $this->assertEquals('', $renderer->number('')); - // Test with number is 0. - $this->assertEquals('

Question 0

', $renderer->number(0)); - // Test with number is numeric. - $this->assertEquals('

Question 1

', $renderer->number(1)); - // Test with number is string. - $this->assertEquals('

Question 1 of 2

', $renderer->number('1 of 2')); + /** + * Data provider for test_render_question_number. + * + * @return array + */ + public function render_question_number_provider(): array { + return [ + 'Test with number is i character' => [ + 'i', + '

Information

', + ], + 'Test with number is empty string' => [ + '', + '', + ], + 'Test with number is 0' => [ + 0, + '

Question 0

', + ], + 'Test with number is numeric' => [ + 1, + '

Question 1

', + ], + 'Test with number is string' => [ + '1 of 2', + '

Question 1 of 2

', + ], + ]; } } diff --git a/question/tests/coverage.php b/question/tests/coverage.php new file mode 100644 index 00000000000..4bcdaa33a5e --- /dev/null +++ b/question/tests/coverage.php @@ -0,0 +1,40 @@ +. + +defined('MOODLE_INTERNAL') || die(); + +/** + * Coverage information for the core_question. + * + * @copyright 2022 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +return new class extends phpunit_coverage_info { + /** @var array The list of files relative to the plugin root to include in coverage generation. */ + protected $includelistfiles = [ + 'category_class.php', + 'category_form.php', + 'editlib.php', + 'export_form.php', + 'format.php', + 'import_form.php', + 'lib.php', + 'move_form.php', + 'previewlib.php', + 'renderer.php', + 'upgrade.php', + ]; +}; From 4ff3447c8c00887fe4f34f246c390542baa18c2a Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 21:21:09 +0800 Subject: [PATCH 05/22] MDL-76362 question: Add test for number(null) --- question/engine/tests/question_engine_test.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/question/engine/tests/question_engine_test.php b/question/engine/tests/question_engine_test.php index 3f6d433bf76..edb2aa9fcf8 100644 --- a/question/engine/tests/question_engine_test.php +++ b/question/engine/tests/question_engine_test.php @@ -319,6 +319,10 @@ class question_engine_test extends advanced_testcase { '', '', ], + 'Test with null' => [ + null, + '', + ], 'Test with number is 0' => [ 0, '

Question 0

', From 5f412b33019189be3ece29625b3b124cbf3feabc Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:09:56 +0800 Subject: [PATCH 06/22] MDL-76362 qtype_numeric: Refactor answer tests --- .../numerical/tests/answerprocessor_test.php | 372 +++++++++++++----- question/type/numerical/tests/coverage.php | 31 ++ 2 files changed, 305 insertions(+), 98 deletions(-) create mode 100644 question/type/numerical/tests/coverage.php diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php index e50bfc56829..db80069a563 100644 --- a/question/type/numerical/tests/answerprocessor_test.php +++ b/question/type/numerical/tests/answerprocessor_test.php @@ -27,17 +27,6 @@ namespace qtype_numerical; use qtype_numerical_answer_processor; -defined('MOODLE_INTERNAL') || die(); - -global $CFG; -require_once($CFG->dirroot . '/question/type/numerical/questiontype.php'); - -class testable_qtype_numerical_answer_processor extends qtype_numerical_answer_processor { - public function parse_response($response) { - return parent::parse_response($response); - } -} - /** * Unit test for the numerical questions answers processor. * @@ -45,60 +34,109 @@ class testable_qtype_numerical_answer_processor extends qtype_numerical_answer_p * @category test * @copyright 2008 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \qtype_numerical_answer_processor */ class answerprocessor_test extends \advanced_testcase { - public function test_parse_response() { - $ap = new testable_qtype_numerical_answer_processor( - array('m' => 1, 'cm' => 100), false, '.', ','); + /** + * Test setup. + */ + public function setUp(): void { + global $CFG; - $this->assertEquals(array('3', '142', '', ''), $ap->parse_response('3.142')); - $this->assertEquals(array('', '2', '', ''), $ap->parse_response('.2')); - $this->assertEquals(array('1', '', '', ''), $ap->parse_response('1.')); - $this->assertEquals(array('1', '0', '', ''), $ap->parse_response('1.0')); - $this->assertEquals(array('-1', '', '', ''), $ap->parse_response('-1.')); - $this->assertEquals(array('+1', '0', '', ''), $ap->parse_response('+1.0')); - - $this->assertEquals(array('1', '', '4', ''), $ap->parse_response('1e4')); - $this->assertEquals(array('3', '142', '-4', ''), $ap->parse_response('3.142E-4')); - $this->assertEquals(array('', '2', '+2', ''), $ap->parse_response('.2e+2')); - $this->assertEquals(array('1', '', '-1', ''), $ap->parse_response('1.e-1')); - $this->assertEquals(array('1', '0', '0', ''), $ap->parse_response('1.0e0')); - - $this->assertEquals(array('3', '', '8', ''), $ap->parse_response('3x10^8')); - $this->assertEquals(array('3', '', '8', ''), $ap->parse_response('3×10^8')); - $this->assertEquals(array('3', '0', '8', ''), $ap->parse_response('3.0*10^8')); - $this->assertEquals(array('3', '00', '-8', ''), $ap->parse_response('3.00x10**-8')); - $this->assertEquals(array('0', '001', '7', ''), $ap->parse_response('0.001×10**7')); - - $this->assertEquals(array('1', '', '', 'm'), $ap->parse_response('1m')); - $this->assertEquals(array('3', '142', '', 'm'), $ap->parse_response('3.142 m')); - $this->assertEquals(array('', '2', '', 'm'), $ap->parse_response('.2m')); - $this->assertEquals(array('1', '', '', 'cm'), $ap->parse_response('1.cm')); - $this->assertEquals(array('1', '0', '', 'cm'), $ap->parse_response('1.0 cm')); - $this->assertEquals(array('-1', '', '', 'm'), $ap->parse_response('-1.m')); - $this->assertEquals(array('+1', '0', '', 'cm'), $ap->parse_response('+1.0cm')); - - $this->assertEquals(array('1', '', '4', 'm'), $ap->parse_response('1e4 m')); - $this->assertEquals(array('3', '142', '-4', 'cm'), $ap->parse_response('3.142E-4 cm')); - $this->assertEquals(array('', '2', '+2', 'm'), $ap->parse_response('.2e+2m')); - $this->assertEquals(array('1', '', '-1', 'm'), $ap->parse_response('1.e-1 m')); - $this->assertEquals(array('1', '0', '0', 'cm'), $ap->parse_response('1.0e0cm')); - - $this->assertEquals(array('1000000', '', '', ''), - $ap->parse_response('1,000,000')); - $this->assertEquals(array('1000', '00', '', 'm'), - $ap->parse_response('1,000.00 m')); - - $this->assertEquals(array(null, null, null, null), $ap->parse_response('frog')); - $this->assertEquals(array('3', '', '', 'frogs'), $ap->parse_response('3 frogs')); - $this->assertEquals(array(null, null, null, null), $ap->parse_response('. m')); - $this->assertEquals(array(null, null, null, null), $ap->parse_response('.e8 m')); - $this->assertEquals(array(null, null, null, null), $ap->parse_response(',')); + require_once("{$CFG->dirroot}/question/type/numerical/questiontype.php"); } - protected function verify_value_and_unit($exectedval, $expectedunit, $expectedmultiplier, - qtype_numerical_answer_processor $ap, $input, $separateunit = null) { - list($val, $unit, $multiplier) = $ap->apply_units($input, $separateunit); + /** + * Test the parse_response function. + * + * @covers ::parse_response + * @dataProvider parse_response_provider + * @param array $expected + * @param mixed $args + */ + public function test_parse_response(array $expected, $args): void { + $ap = new qtype_numerical_answer_processor([ + 'm' => 1, + 'cm' => 100, + ], false, '.', ','); + + $rc = new \ReflectionClass($ap); + $rcm = $rc->getMethod('parse_response'); + $rcm->setAccessible(True); + + $this->assertEquals($expected, $rcm->invoke($ap, $args)); + } + + /** + * Data provider for the parse_response function. + * + * @return array + */ + public function parse_response_provider(): array { + return [ + [['3', '142', '', ''], '3.142'], + [['', '2', '', ''], '.2'], + [['1', '', '', ''], '1.'], + [['1', '0', '', ''], '1.0'], + [['-1', '', '', ''], '-1.'], + [['+1', '0', '', ''], '+1.0'], + + [['1', '', '4', ''], '1e4'], + [['3', '142', '-4', ''], '3.142E-4'], + [['', '2', '+2', ''], '.2e+2'], + [['1', '', '-1', ''], '1.e-1'], + [['1', '0', '0', ''], '1.0e0'], + + [['3', '', '8', ''], '3x10^8'], + [['3', '', '8', ''], '3×10^8'], + [['3', '0', '8', ''], '3.0*10^8'], + [['3', '00', '-8', ''], '3.00x10**-8'], + [['0', '001', '7', ''], '0.001×10**7'], + + [['1', '', '', 'm'], '1m'], + [['3', '142', '', 'm'], '3.142 m'], + [['', '2', '', 'm'], '.2m'], + [['1', '', '', 'cm'], '1.cm'], + [['1', '0', '', 'cm'], '1.0 cm'], + [['-1', '', '', 'm'], '-1.m'], + [['+1', '0', '', 'cm'], '+1.0cm'], + + [['1', '', '4', 'm'], '1e4 m'], + [['3', '142', '-4', 'cm'], '3.142E-4 cm'], + [['', '2', '+2', 'm'], '.2e+2m'], + [['1', '', '-1', 'm'], '1.e-1 m'], + [['1', '0', '0', 'cm'], '1.0e0cm'], + + [['1000000', '', '', ''], '1,000,000'], + [['1000', '00', '', 'm'], '1,000.00 m'], + + [[null, null, null, null], 'frog'], + [['3', '', '', 'frogs'], '3 frogs'], + [[null, null, null, null], '. m'], + [[null, null, null, null], '.e8 m'], + [[null, null, null, null], ','], + ]; + } + + /** + * Call apply_units and verify the value and units returned. + * + * @param int|float $exectedval + * @param null|string $expectedunit + * @param int|float $expectedmultiplier + * @param qtype_numerical_answer_processor $ap + * @param null|int|float $input + * @param null|string $separateunit + */ + protected function verify_value_and_unit( + $exectedval, + $expectedunit, + $expectedmultiplier, + qtype_numerical_answer_processor $ap, + $input, + $separateunit = null + ): void { + [$val, $unit, $multiplier] = $ap->apply_units($input, $separateunit); if (is_null($exectedval)) { $this->assertNull($val); } else { @@ -112,57 +150,195 @@ class answerprocessor_test extends \advanced_testcase { } } - public function test_apply_units() { + /** + * Test the apply_units function with various parameters. + * + * @covers \qtype_numerical_answer_processor::apply_units + * @dataProvider apply_units_provider + * @param mixed $expectedvalue + * @param string|null $expectedunit + * @param float|int|null $expectedmultiplier + * @param string|null $input + */ + public function test_apply_units( + $expectedvalue, + $expectedunit, + $expectedmultiplier, + $input + ): void { $ap = new qtype_numerical_answer_processor( - array('m/s' => 1, 'c' => 3.3356409519815E-9, - 'mph' => 2.2369362920544), false, '.', ','); + [ + 'm/s' => 1, + 'c' => 3.3356409519815E-9, + 'mph' => 2.2369362920544 + ], + false, + '.', + ',' + ); - $this->verify_value_and_unit(3e8, 'm/s', 1, $ap, '3x10^8 m/s'); - $this->verify_value_and_unit(3e8, '', null, $ap, '3x10^8'); - $this->verify_value_and_unit(1, 'c', 299792458, $ap, '1c'); - $this->verify_value_and_unit(1, 'mph', 0.44704, $ap, '0001.000 mph'); - - $this->verify_value_and_unit(1, 'frogs', null, $ap, '1 frogs'); - $this->verify_value_and_unit(null, null, null, $ap, '. m/s'); + $this->verify_value_and_unit( + $expectedvalue, + $expectedunit, + $expectedmultiplier, + $ap, + $input + ); } - public function test_apply_units_separate_unit() { + /** + * Data provider for apply_units tests. + * + * @return array + */ + public function apply_units_provider(): array { + return [ + [3e8, 'm/s', 1, '3x10^8 m/s'], + [3e8, '', null, '3x10^8'], + [1, 'c', 299792458, '1c'], + [1, 'mph', 0.44704, '0001.000 mph'], + + [1, 'frogs', null, '1 frogs'], + [null, null, null, '. m/s'], + ]; + } + + /** + * Test the apply_units function with various parameters and different units. + * + * @covers \qtype_numerical_answer_processor::apply_units + * @dataProvider apply_units_provider_with_units + * @param mixed $expectedvalue + * @param string|null $expectedunit + * @param float|int|null $expectedmultiplier + * @param string|null $input + * @param string $units + */ + public function test_apply_units_with_unit( + $expectedvalue, + $expectedunit, + $expectedmultiplier, + $input, + $units + ): void { $ap = new qtype_numerical_answer_processor( - array('m/s' => 1, 'c' => 3.3356409519815E-9, - 'mph' => 2.2369362920544), false, '.', ','); + [ + 'm/s' => 1, + 'c' => 3.3356409519815E-9, + 'mph' => 2.2369362920544 + ], + false, + '.', + ',' + ); - $this->verify_value_and_unit(3e8, 'm/s', 1, $ap, '3x10^8', 'm/s'); - $this->verify_value_and_unit(3e8, '', null, $ap, '3x10^8', ''); - $this->verify_value_and_unit(1, 'c', 299792458, $ap, '1', 'c'); - $this->verify_value_and_unit(1, 'mph', 0.44704, $ap, '0001.000', 'mph'); - - $this->verify_value_and_unit(1, 'frogs', null, $ap, '1', 'frogs'); - $this->verify_value_and_unit(null, null, null, $ap, '.', 'm/s'); + $this->verify_value_and_unit( + $expectedvalue, + $expectedunit, + $expectedmultiplier, + $ap, + $input, + $units + ); } - public function test_euro_style() { - $ap = new qtype_numerical_answer_processor(array(), false, ',', ' '); + /** + * Data provider for apply_units with different units. + * + * @return array + */ + public function apply_units_provider_with_units(): array { + return [ + [3e8, 'm/s', 1, '3x10^8', 'm/s'], + [3e8, '', null, '3x10^8', ''], + [1, 'c', 299792458, '1', 'c'], + [1, 'mph', 0.44704, '0001.000', 'mph'], - $this->assertEquals(array(-1000, '', null), $ap->apply_units('-1 000')); - $this->assertEquals(array(3.14159, '', null), $ap->apply_units('3,14159')); + [1, 'frogs', null, '1', 'frogs'], + [null, null, null, '.', 'm/s'], + ]; } - public function test_percent() { - $ap = new qtype_numerical_answer_processor(array('%' => 100), false, '.', ','); - - $this->assertEquals(array('3', '%', 0.01), $ap->apply_units('3%')); - $this->assertEquals(array('1e-6', '%', 0.01), $ap->apply_units('1e-6 %')); - $this->assertEquals(array('100', '', null), $ap->apply_units('100')); + /** + * Test apply_units with a comma float unit. + * + * @covers \qtype_numerical_answer_processor::apply_units + * @dataProvider euro_provider + * @param array $expected + * @param string $params + */ + public function test_euro_style(array $expected, string $params): void { + $ap = new qtype_numerical_answer_processor([], false, ',', ' '); + $this->assertEquals($expected, $ap->apply_units($params)); } - public function test_currency() { - $ap = new qtype_numerical_answer_processor(array('$' => 1, '£' => 1), true, '.', ','); + /** + * Data provider for apply_units with euro float separators. + * + * return array + */ + public function euro_provider(): array { + return [ + [[-1000, '', null], '-1 000'], + [[3.14159, '', null], '3,14159'], + ]; + } - $this->assertEquals(array('1234.56', '£', 1), $ap->apply_units('£1,234.56')); - $this->assertEquals(array('100', '$', 1), $ap->apply_units('$100')); - $this->assertEquals(array('100', '$', 1), $ap->apply_units('$100.')); - $this->assertEquals(array('100.00', '$', 1), $ap->apply_units('$100.00')); - $this->assertEquals(array('100', '', null), $ap->apply_units('100')); - $this->assertEquals(array('100', 'frog', null), $ap->apply_units('frog 100')); + /** + * Test apply_units with percentage values. + * + * @covers \qtype_numerical_answer_processor::apply_units + * @dataProvider percent_provider + * @param array $expected + * @param string $params + */ + public function test_percent(array $expected, string $params): void { + $ap = new qtype_numerical_answer_processor(['%' => 100], false, '.', ','); + $this->assertEquals($expected, $ap->apply_units($params)); + } + + /** + * Data provider for apply_units with percentages. + * + * @return array + */ + public function percent_provider(): array { + return [ + [['3', '%', 0.01], '3%'], + [['1e-6', '%', 0.01], '1e-6 %'], + [['100', '', null], '100'], + ]; + } + + /** + * Test apply_units with currency values. + * + * @covers \qtype_numerical_answer_processor::apply_units + * @dataProvider currency_provider + * @param array $expected + * @param string $params + */ + public function test_currency(array $expected, string $params): void { + $ap = new qtype_numerical_answer_processor([ + '$' => 1, + '£' => 1, + ], true, '.', ','); + $this->assertEquals($expected, $ap->apply_units($params)); + } + + /** + * Data provider for apply_units with currency values. + * + * @return array + */ + public function currency_provider(): array { + return [ + [['1234.56', '£', 1], '£1,234.56'], + [['100', '$', 1], '$100'], + [['100', '$', 1], '$100.'], + [['100.00', '$', 1], '$100.00'], + [['100', '', null], '100'], + [['100', 'frog', null], 'frog 100'], + ]; } } diff --git a/question/type/numerical/tests/coverage.php b/question/type/numerical/tests/coverage.php new file mode 100644 index 00000000000..8d8e9355908 --- /dev/null +++ b/question/type/numerical/tests/coverage.php @@ -0,0 +1,31 @@ +. + +defined('MOODLE_INTERNAL') || die(); + +/** + * Coverage information for the qtype_numerical. + * + * @copyright 2022 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +return new class extends phpunit_coverage_info { + /** @var array The list of files relative to the plugin root to include in coverage generation. */ + protected $includelistfiles = [ + 'question.php', + 'questiontype.php', + ]; +}; From 12b36d2a32a2fc732ffb919b0d53fe5173958f54 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:11:22 +0800 Subject: [PATCH 07/22] MDL-76362 qtype_numerical: Support empty units for apply_units --- question/type/numerical/questiontype.php | 6 +++++- question/type/numerical/tests/answerprocessor_test.php | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index 3aa8a2e8f9e..5ba8b3a8681 100644 --- a/question/type/numerical/questiontype.php +++ b/question/type/numerical/questiontype.php @@ -651,9 +651,13 @@ class qtype_numerical_answer_processor { * by the unit multiplier, if any, and the unit string, for reference. */ public function apply_units($response, $separateunit = null) { + if ($response === null || trim($response) === '') { + return [null, null, null]; + } + // Strip spaces (which may be thousands separators) and change other forms // of writing e to e. - $response = str_replace(' ', '', $response ?? ''); + $response = str_replace(' ', '', $response); $response = preg_replace('~(?:e|E|(?:x|\*|×)10(?:\^|\*\*))([+-]?\d+)~', 'e$1', $response); // If a . is present or there are multiple , (i.e. 2,456,789 ) assume , diff --git a/question/type/numerical/tests/answerprocessor_test.php b/question/type/numerical/tests/answerprocessor_test.php index db80069a563..dd764665b5d 100644 --- a/question/type/numerical/tests/answerprocessor_test.php +++ b/question/type/numerical/tests/answerprocessor_test.php @@ -62,7 +62,7 @@ class answerprocessor_test extends \advanced_testcase { $rc = new \ReflectionClass($ap); $rcm = $rc->getMethod('parse_response'); - $rcm->setAccessible(True); + $rcm->setAccessible(true); $this->assertEquals($expected, $rcm->invoke($ap, $args)); } @@ -200,6 +200,9 @@ class answerprocessor_test extends \advanced_testcase { [1, 'frogs', null, '1 frogs'], [null, null, null, '. m/s'], + [null, null, null, null], + [null, null, null, ''], + [null, null, null, ' '], ]; } From 5fbd2eac329ced8da050857fbb8b10143eefd8ac Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:17:22 +0800 Subject: [PATCH 08/22] MDL-76362 qtype: Update formatting of qtype_calculated* upgradelib --- question/type/calculated/db/upgradelib.php | 49 +++++++-------- .../type/calculatedmulti/db/upgradelib.php | 59 ++++++++----------- 2 files changed, 47 insertions(+), 61 deletions(-) diff --git a/question/type/calculated/db/upgradelib.php b/question/type/calculated/db/upgradelib.php index 9c8b2d7bbab..c7695d89b9a 100644 --- a/question/type/calculated/db/upgradelib.php +++ b/question/type/calculated/db/upgradelib.php @@ -14,25 +14,13 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * Upgrade library code for the calculated question type. - * - * @package qtype - * @subpackage calculated - * @copyright 2011 The Open University - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - - -defined('MOODLE_INTERNAL') || die(); - - /** * Class for converting attempt data for calculated questions when upgrading * attempts to the new question engine. * * This class is used by the code in question/engine/upgrade/upgradelib.php. * + * @package qtype_calculated * @copyright 2011 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -86,7 +74,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update throw new coding_exception("Brokes state {$state->id} for calculated question {$state->question}. (It did not specify a dataset."); } - list($datasetbit, $realanswer) = explode('-', $state->answer, 2); + [$datasetbit, $realanswer] = explode('-', $state->answer, 2); $selecteditem = substr($datasetbit, 7); if (is_null($this->selecteditem)) { @@ -98,21 +86,21 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update } if (!$realanswer) { - return array('', ''); + return ['', '']; } if (strpos($realanswer, '|||||') === false) { $answer = $realanswer; $unit = ''; } else { - list($answer, $unit) = explode('|||||', $realanswer, 2); + [$answer, $unit] = explode('|||||', $realanswer, 2); } - return array($answer, $unit); + return [$answer, $unit]; } public function response_summary($state) { - list($answer, $unit) = $this->parse_response($state); + [$answer, $unit] = $this->parse_response($state); if (empty($answer) && empty($unit)) { $resp = null; @@ -157,9 +145,11 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update return; } - list($answer, $unit) = $this->parse_response($state); - if (!empty($this->question->options->showunits) && - $this->question->options->showunits == 1) { + [$answer, $unit] = $this->parse_response($state); + if ( + !empty($this->question->options->showunits) && + $this->question->options->showunits == 1 + ) { // Multichoice units. $data['answer'] = $answer; $data['unit'] = $unit; @@ -185,9 +175,9 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update $this->question->id, $selecteditem); // Prepare an array for {@link substitute_values()}. - $this->search = array(); - $this->safevalue = array(); - $this->prettyvalue = array(); + $this->search = []; + $this->safevalue = []; + $this->prettyvalue = []; foreach ($this->values as $name => $value) { if (!is_numeric($value)) { $a = new stdClass(); @@ -293,10 +283,13 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update */ public function replace_expressions_in_text($text, $length = null, $format = null) { $vs = $this; // Can't see to use $this in a PHP closure. - $text = preg_replace_callback('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', - function ($matches) use ($vs, $format, $length) { - return $vs->format_float($vs->calculate($matches[1]), $length, $format); - }, $text ?? ''); + $text = preg_replace_callback( + '~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', + function ($matches) use ($vs, $format, $length) { + return $vs->format_float($vs->calculate($matches[1]), $length, $format); + }, + $text + ); return $this->substitute_values_pretty($text); } } diff --git a/question/type/calculatedmulti/db/upgradelib.php b/question/type/calculatedmulti/db/upgradelib.php index 543fbbf8b65..ca795687ed1 100644 --- a/question/type/calculatedmulti/db/upgradelib.php +++ b/question/type/calculatedmulti/db/upgradelib.php @@ -14,25 +14,13 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * Upgrade library code for the calculated multiple-choice question type. - * - * @package qtype - * @subpackage calculatedmulti - * @copyright 2011 The Open University - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - - -defined('MOODLE_INTERNAL') || die(); - - /** * Class for converting attempt data for calculated multiple-choice questions * when upgrading attempts to the new question engine. * * This class is used by the code in question/engine/upgrade/upgradelib.php. * + * @package qtype_calculatedmulti * @copyright 2011 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -69,9 +57,8 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u return $this->to_text($this->replace_expressions_in_text($ans->answer)); } } - } else { - $rightbits = array(); + $rightbits = []; foreach ($this->question->options->answers as $ans) { if ($ans->fraction >= 0.000001) { $rightbits[] = $this->to_text($this->replace_expressions_in_text($ans->answer)); @@ -87,7 +74,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u throw new coding_exception("Brokes state {$state->id} for calcluatedmulti question {$state->question}. (It did not specify a dataset."); } - list($datasetbit, $answer) = explode('-', $state->answer, 2); + [$datasetbit, $answer] = explode('-', $state->answer, 2); $selecteditem = substr($datasetbit, 7); if (is_null($this->selecteditem)) { @@ -99,7 +86,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u } if (strpos($answer, ':') !== false) { - list($order, $responses) = explode(':', $answer); + [$order, $responses] = explode(':', $answer); return $responses; } else { // Sometimes, a bug means that a state is missing the : bit, @@ -116,7 +103,8 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u if (is_numeric($responses)) { if (array_key_exists($responses, $this->question->options->answers)) { return $this->to_text($this->replace_expressions_in_text( - $this->question->options->answers[$responses]->answer)); + $this->question->options->answers[$responses]->answer + )); } else { $this->logger->log_assumption("Dealing with a place where the student selected a choice that was later deleted for @@ -126,15 +114,15 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u } else { return null; } - } else { if (!empty($responses)) { $responses = explode(',', $responses); - $bits = array(); + $bits = []; foreach ($responses as $response) { if (array_key_exists($response, $this->question->options->answers)) { $bits[] = $this->to_text($this->replace_expressions_in_text( - $this->question->options->answers[$response]->answer)); + $this->question->options->answers[$response]->answer + )); } else { $this->logger->log_assumption("Dealing with a place where the student selected a choice that was later deleted for @@ -161,15 +149,16 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u public function set_first_step_data_elements($state, &$data) { $this->explode_answer($state); $this->updater->qa->questionsummary = $this->to_text( - $this->replace_expressions_in_text($this->question->questiontext)); + $this->replace_expressions_in_text($this->question->questiontext) + ); $this->updater->qa->rightanswer = $this->right_answer($this->question); foreach ($this->values as $name => $value) { $data['_var_' . $name] = $value; } - list($datasetbit, $answer) = explode('-', $state->answer, 2); - list($order, $responses) = explode(':', $answer); + [$datasetbit, $answer] = explode('-', $state->answer, 2); + [$order, $responses] = explode(':', $answer); $data['_order'] = $order; $this->order = explode(',', $order); } @@ -189,7 +178,6 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u $data['answer'] = '-1'; } } - } else { $responses = explode(',', $responses); foreach ($this->order as $key => $ansid) { @@ -206,12 +194,14 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u $this->selecteditem = $selecteditem; $this->updater->qa->variant = $selecteditem; $this->values = $this->qeupdater->load_dataset( - $this->question->id, $selecteditem); + $this->question->id, + $selecteditem + ); // Prepare an array for {@link substitute_values()}. - $this->search = array(); - $this->safevalue = array(); - $this->prettyvalue = array(); + $this->search = []; + $this->safevalue = []; + $this->prettyvalue = []; foreach ($this->values as $name => $value) { if (!is_numeric($value)) { $a = new stdClass(); @@ -317,10 +307,13 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u */ public function replace_expressions_in_text($text, $length = null, $format = null) { $vs = $this; // Can't see to use $this in a PHP closure. - $text = preg_replace_callback(qtype_calculated::FORMULAS_IN_TEXT_REGEX, - function ($matches) use ($vs, $format, $length) { - return $vs->format_float($vs->calculate($matches[1]), $length, $format); - }, $text ?? ''); + $text = preg_replace_callback( + qtype_calculated::FORMULAS_IN_TEXT_REGEX, + function ($matches) use ($vs, $format, $length) { + return $vs->format_float($vs->calculate($matches[1]), $length, $format); + }, + $text + ); return $this->substitute_values_pretty($text); } } From 71c1fa0d8ea42fadbca4259b8b6df12454d40a21 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:26:45 +0800 Subject: [PATCH 09/22] MDL-76362 qtype_calculated*: Address issues with null strings --- question/type/calculated/db/upgradelib.php | 15 +++++++++------ question/type/calculatedmulti/db/upgradelib.php | 16 ++++++++++------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/question/type/calculated/db/upgradelib.php b/question/type/calculated/db/upgradelib.php index c7695d89b9a..c01b8ae6344 100644 --- a/question/type/calculated/db/upgradelib.php +++ b/question/type/calculated/db/upgradelib.php @@ -27,22 +27,22 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_updater { protected $selecteditem = null; /** @var array variable name => value */ - protected $values; + protected $values = []; /** @var array variable names wrapped in {...}. Used by {@link substitute_values()}. */ - protected $search; + protected $search = []; /** * @var array variable values, with negative numbers wrapped in (...). * Used by {@link substitute_values()}. */ - protected $safevalue; + protected $safevalue = []; /** * @var array variable values, with negative numbers wrapped in (...). * Used by {@link substitute_values()}. */ - protected $prettyvalue; + protected $prettyvalue = []; public function question_summary() { return ''; // Done later, after we know which dataset is used. @@ -260,7 +260,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update * corresponding value. */ protected function substitute_values_for_eval($expression) { - return str_replace($this->search ?? '', $this->safevalue ?? '', $expression ?? ''); + return str_replace($this->search, $this->safevalue, $expression ?? ''); } /** @@ -272,7 +272,7 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update * corresponding value. */ protected function substitute_values_pretty($text) { - return str_replace($this->search ?? '', $this->prettyvalue ?? '', $text ?? ''); + return str_replace($this->search, $this->prettyvalue, $text ?? ''); } /** @@ -282,6 +282,9 @@ class qtype_calculated_qe2_attempt_updater extends question_qtype_attempt_update * @return string the text with values substituted. */ public function replace_expressions_in_text($text, $length = null, $format = null) { + if ($text === null || $text === '') { + return $text; + } $vs = $this; // Can't see to use $this in a PHP closure. $text = preg_replace_callback( '~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~', diff --git a/question/type/calculatedmulti/db/upgradelib.php b/question/type/calculatedmulti/db/upgradelib.php index ca795687ed1..9beb0c82a0d 100644 --- a/question/type/calculatedmulti/db/upgradelib.php +++ b/question/type/calculatedmulti/db/upgradelib.php @@ -27,22 +27,22 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_updater { protected $selecteditem = null; /** @var array variable name => value */ - protected $values; + protected $values = []; /** @var array variable names wrapped in {...}. Used by {@link substitute_values()}. */ - protected $search; + protected $search = []; /** * @var array variable values, with negative numbers wrapped in (...). * Used by {@link substitute_values()}. */ - protected $safevalue; + protected $safevalue = []; /** * @var array variable values, with negative numbers wrapped in (...). * Used by {@link substitute_values()}. */ - protected $prettyvalue; + protected $prettyvalue = []; protected $order; @@ -284,7 +284,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u * corresponding value. */ protected function substitute_values_for_eval($expression) { - return str_replace($this->search ?? '', $this->safevalue ?? '', $expression ?? ''); + return str_replace($this->search, $this->safevalue, $expression ?? ''); } /** @@ -296,7 +296,7 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u * corresponding value. */ protected function substitute_values_pretty($text) { - return str_replace($this->search ?? '', $this->prettyvalue ?? '', $text ?? ''); + return str_replace($this->search, $this->prettyvalue, $text ?? ''); } /** @@ -306,6 +306,10 @@ class qtype_calculatedmulti_qe2_attempt_updater extends question_qtype_attempt_u * @return string the text with values substituted. */ public function replace_expressions_in_text($text, $length = null, $format = null) { + if ($text === null || $text === '') { + return $text; + } + $vs = $this; // Can't see to use $this in a PHP closure. $text = preg_replace_callback( qtype_calculated::FORMULAS_IN_TEXT_REGEX, From 2250ab07e623a997fb40b6a39692aa4c35a5f2b2 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:29:02 +0800 Subject: [PATCH 10/22] MDL-76362 qtype_numerical: Fix bug in phpdoc --- question/type/numerical/questiontype.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index 5ba8b3a8681..37d29dafbb7 100644 --- a/question/type/numerical/questiontype.php +++ b/question/type/numerical/questiontype.php @@ -647,10 +647,10 @@ class qtype_numerical_answer_processor { * default unit, by using the given unit multiplier. * * @param string $response a value, optionally with a unit. - * @return array(numeric, sting) the value with the unit stripped, and normalised + * @return array(numeric, string, multiplier) the value with the unit stripped, and normalised * by the unit multiplier, if any, and the unit string, for reference. */ - public function apply_units($response, $separateunit = null) { + public function apply_units($response, $separateunit = null): array { if ($response === null || trim($response) === '') { return [null, null, null]; } From ddf0f08cc9615c52d954f6d2aa2097224d9b2ee9 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:29:25 +0800 Subject: [PATCH 11/22] MDL-76362 h5p: Use string as default lang param --- h5p/ajax.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/h5p/ajax.php b/h5p/ajax.php index 29cbba05fb0..f02f098e5e1 100644 --- a/h5p/ajax.php +++ b/h5p/ajax.php @@ -58,8 +58,8 @@ switch ($action) { $minor = optional_param('minorVersion', 0, PARAM_INT); // Normalise Moodle language using underscore, as opposed to H5P which uses dash. - $language = optional_param('default-language', null, PARAM_RAW); - $language = clean_param(str_replace('-', '_', $language ?? ''), PARAM_LANG); + $language = optional_param('default-language', '', PARAM_RAW); + $language = clean_param(str_replace('-', '_', $language), PARAM_LANG); if (!empty($name)) { $editor->ajax->action(H5PEditorEndpoints::SINGLE_LIBRARY, $name, From de4de9cec5c52efddae7c1a97b75b25a875e38be Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:29:49 +0800 Subject: [PATCH 12/22] MDL-76362 core: Test coursealias before using --- lib/accesslib.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/accesslib.php b/lib/accesslib.php index e403839ef25..f340b1f8641 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -4519,7 +4519,7 @@ function role_get_name(stdClass $role, $context = null, $rolenamedisplay = ROLEN } if ($rolenamedisplay == ROLENAME_ALIAS) { - if ($coursecontext and trim($role->coursealias ?? '') !== '') { + if ($coursecontext && $role->coursealias && trim($role->coursealias) !== '') { return format_string($role->coursealias, true, array('context'=>$coursecontext)); } else { return $original; @@ -4527,7 +4527,7 @@ function role_get_name(stdClass $role, $context = null, $rolenamedisplay = ROLEN } if ($rolenamedisplay == ROLENAME_BOTH) { - if ($coursecontext and trim($role->coursealias ?? '') !== '') { + if ($coursecontext && $role->coursealias && trim($role->coursealias) !== '') { return format_string($role->coursealias, true, array('context'=>$coursecontext)) . " ($original)"; } else { return $original; From d08319ddf79f86faf9b043d1da7427a1c6eb2871 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:34:26 +0800 Subject: [PATCH 13/22] MDL-76362 core: Update core_component plugin name tests --- lib/tests/component_test.php | 78 +++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/lib/tests/component_test.php b/lib/tests/component_test.php index 631fd27643b..c40ed846dcc 100644 --- a/lib/tests/component_test.php +++ b/lib/tests/component_test.php @@ -22,13 +22,6 @@ * @copyright 2013 Petr Skoda {@link http://skodak.org} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ - -defined('MOODLE_INTERNAL') || die(); - - -/** - * Class core_component_testcase. - */ class component_test extends advanced_testcase { /** @@ -217,33 +210,52 @@ class component_test extends advanced_testcase { } } - public function test_is_valid_plugin_name() { - $this->assertTrue(core_component::is_valid_plugin_name('mod', 'example1')); - $this->assertTrue(core_component::is_valid_plugin_name('mod', 'feedback360')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'feedback_360')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', '2feedback')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', '1example')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'example.xx')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', '.example')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', '_example')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'example_')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'example_x1')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'example-x1')); - $this->assertFalse(core_component::is_valid_plugin_name('mod', 'role')); + /** + * Test that the get_plugin_list_with_file() function returns the correct list of plugins. + * + * @covers \core_component::is_valid_plugin_name + * @dataProvider is_valid_plugin_name_provider + * @param array $arguments + * @param bool $expected + */ + public function test_is_valid_plugin_name(array $arguments, bool $expected): void { + $this->assertEquals($expected, core_component::is_valid_plugin_name(...$arguments)); + } - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'example1')); - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'example_x1')); - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'example_x1_xxx')); - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'feedback360')); - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'feed_back360')); - $this->assertTrue(core_component::is_valid_plugin_name('tool', 'role')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', '1example')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', 'example.xx')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', 'example-xx')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', '.example')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', '_example')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', 'example_')); - $this->assertFalse(core_component::is_valid_plugin_name('tool', 'example__x1')); + /** + * Data provider for the is_valid_plugin_name function. + * + * @return array + */ + public function is_valid_plugin_name_provider(): array { + return [ + [['mod', 'example1'], true], + [['mod', 'feedback360'], true], + [['mod', 'feedback_360'], false], + [['mod', '2feedback'], false], + [['mod', '1example'], false], + [['mod', 'example.xx'], false], + [['mod', '.example'], false], + [['mod', '_example'], false], + [['mod', 'example_'], false], + [['mod', 'example_x1'], false], + [['mod', 'example-x1'], false], + [['mod', 'role'], false], + + [['tool', 'example1'], true], + [['tool', 'example_x1'], true], + [['tool', 'example_x1_xxx'], true], + [['tool', 'feedback360'], true], + [['tool', 'feed_back360'], true], + [['tool', 'role'], true], + [['tool', '1example'], false], + [['tool', 'example.xx'], false], + [['tool', 'example-xx'], false], + [['tool', '.example'], false], + [['tool', '_example'], false], + [['tool', 'example_'], false], + [['tool', 'example__x1'], false], + ]; } public function test_normalize_componentname() { From a1f4f7bac564ab6f5faa1f76e8834066a7a23539 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:30:16 +0800 Subject: [PATCH 14/22] MDL-76362 core: plugin names must be strings to be valid --- lib/classes/component.php | 5 ++--- lib/tests/component_test.php | 6 ++++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/classes/component.php b/lib/classes/component.php index b818096fd87..fb6fc9012b0 100644 --- a/lib/classes/component.php +++ b/lib/classes/component.php @@ -970,10 +970,9 @@ $cache = '.var_export($cache, true).'; } // Modules MUST NOT have any underscores, // component normalisation would break very badly otherwise! - return (bool)preg_match('/^[a-z][a-z0-9]*$/', $pluginname); - + return !is_null($pluginname) && (bool) preg_match('/^[a-z][a-z0-9]*$/', $pluginname); } else { - return (bool)preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]+$/', $pluginname ?? ''); + return !is_null($pluginname) && (bool) preg_match('/^[a-z](?:[a-z0-9_](?!__))*[a-z0-9]+$/', $pluginname); } } diff --git a/lib/tests/component_test.php b/lib/tests/component_test.php index c40ed846dcc..2d0cc23632c 100644 --- a/lib/tests/component_test.php +++ b/lib/tests/component_test.php @@ -255,6 +255,12 @@ class component_test extends advanced_testcase { [['tool', '_example'], false], [['tool', 'example_'], false], [['tool', 'example__x1'], false], + + // Some invalid cases. + [['mod', null], false], + [['mod', ''], false], + [['tool', null], false], + [['tool', ''], false], ]; } From 788d86d7a36c1667776f0cb9a273cb4f73c872f1 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:31:42 +0800 Subject: [PATCH 15/22] MDL-76362 core_file: Check prefix exists before checking length --- lib/filebrowser/file_info.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/filebrowser/file_info.php b/lib/filebrowser/file_info.php index cba268f39b7..2127d1f4f60 100644 --- a/lib/filebrowser/file_info.php +++ b/lib/filebrowser/file_info.php @@ -100,8 +100,8 @@ abstract class file_info { */ protected function build_search_files_sql($extensions, $prefix = null) { global $DB; - if (strlen($prefix ?? '')) { - $prefix = $prefix.'.'; + if ($prefix && strlen($prefix)) { + $prefix = $prefix . '.'; } else { $prefix = ''; } From dbfb5eaa45f251ab92daa0d9013dd5b620b2f3b6 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:36:54 +0800 Subject: [PATCH 16/22] MDL-76362 core: Short-circuit strip_querystring on empty values --- lib/tests/weblib_test.php | 30 ++++++++++++++++++++++++++++-- lib/weblib.php | 5 ++++- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/tests/weblib_test.php b/lib/tests/weblib_test.php index 6b93afa3f1b..8e4f5606764 100644 --- a/lib/tests/weblib_test.php +++ b/lib/tests/weblib_test.php @@ -23,9 +23,7 @@ * @author T.J.Hunt@open.ac.uk * @license http://www.gnu.org/copyleft/gpl.html GNU Public License */ - class weblib_test extends advanced_testcase { - /** * @covers ::format_string */ @@ -985,4 +983,32 @@ EXPECTED; public function test_get_html_lang_attribute_value(string $langcode, string $expected): void { $this->assertEquals($expected, get_html_lang_attribute_value($langcode)); } + + /** + * Data provider for strip_querystring tests. + * + * @return array + */ + public function strip_querystring_provider(): array { + return [ + 'Null' => [null, ''], + 'Empty string' => ['', ''], + 'No querystring' => ['https://example.com', 'https://example.com'], + 'Querystring' => ['https://example.com?foo=bar', 'https://example.com'], + 'Querystring with fragment' => ['https://example.com?foo=bar#baz', 'https://example.com'], + 'Querystring with fragment and path' => ['https://example.com/foo/bar?foo=bar#baz', 'https://example.com/foo/bar'], + ]; + } + + /** + * Test the strip_querystring function with various exampels. + * + * @dataProvider strip_querystring_provider + * @param mixed $value + * @param mixed $expected + * @covers ::strip_querystring + */ + public function test_strip_querystring($value, $expected): void { + $this->assertEquals($expected, strip_querystring($value)); + } } diff --git a/lib/weblib.php b/lib/weblib.php index 27ac693e402..10b27d82cda 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -148,8 +148,11 @@ function addslashes_js($var) { * @return string The remaining URL. */ function strip_querystring($url) { + if ($url === null || $url === '') { + return ''; + } - if ($commapos = strpos($url ?? '', '?')) { + if ($commapos = strpos($url, '?')) { return substr($url, 0, $commapos); } else { return $url; From a4ea607c24d22d9ca8aa129e3052a02adc427efe Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:37:07 +0800 Subject: [PATCH 17/22] MDL-76362 core: Short circuit s() on empty values --- lib/weblib.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/weblib.php b/lib/weblib.php index 10b27d82cda..45b00ef4e69 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -91,13 +91,18 @@ define('URL_MATCH_EXACT', 2); * @return string */ function s($var) { - if ($var === false) { return '0'; } - return preg_replace('/&#(\d+|x[0-9a-f]+);/i', '&#$1;', - htmlspecialchars($var ?? '', ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE)); + if ($var === null || $var === '') { + return ''; + } + + return preg_replace( + '/&#(\d+|x[0-9a-f]+);/i', '&#$1;', + htmlspecialchars($var, ENT_QUOTES | ENT_HTML401 | ENT_SUBSTITUTE) + ); } /** From 77a0a535b3668975a53feb477b3e911ff98065a6 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 6 Jan 2023 22:37:59 +0800 Subject: [PATCH 18/22] MDL-76362 core_privacy: Shortcircuit URL rewriting on empty content --- .../local/request/moodle_content_writer.php | 17 ++++++----------- .../classes/tests/request/content_writer.php | 2 +- privacy/tests/moodle_content_writer_test.php | 12 ++++++++++++ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php index c50720a6b45..a8d893b8776 100644 --- a/privacy/classes/local/request/moodle_content_writer.php +++ b/privacy/classes/local/request/moodle_content_writer.php @@ -14,17 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * This file contains the moodle format implementation of the content writer. - * - * @package core_privacy - * @copyright 2018 Andrew Nicols - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ namespace core_privacy\local\request; -defined('MOODLE_INTERNAL') || die(); - /** * The moodle_content_writer is the default Moodle implementation of a content writer. * @@ -33,6 +24,7 @@ defined('MOODLE_INTERNAL') || die(); * * Objects of data are stored as JSON. * + * @package core_privacy * @copyright 2018 Andrew Nicols * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ @@ -168,7 +160,10 @@ class moodle_content_writer implements content_writer { * @param string $text The text to be processed * @return string The processed string */ - public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { + public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text): string { + if ($text === null || $text === '') { + return ''; + } // Need to take into consideration the subcontext to provide the full path to this file. $subcontextpath = ''; if (!empty($subcontext)) { @@ -179,7 +174,7 @@ class moodle_content_writer implements content_writer { $returnstring = $path . DIRECTORY_SEPARATOR . $this->get_files_target_url($component, $filearea, $itemid) . '/'; $returnstring = clean_param($returnstring, PARAM_PATH); - return str_replace('@@PLUGINFILE@@/', $returnstring, $text ?? ''); + return str_replace('@@PLUGINFILE@@/', $returnstring, $text); } /** diff --git a/privacy/classes/tests/request/content_writer.php b/privacy/classes/tests/request/content_writer.php index c6e9d3d080f..3a8c64e7e30 100644 --- a/privacy/classes/tests/request/content_writer.php +++ b/privacy/classes/tests/request/content_writer.php @@ -381,7 +381,7 @@ class content_writer implements \core_privacy\local\request\content_writer { * @param string $text The text to be processed * @return string The processed string */ - public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text) : string { + public function rewrite_pluginfile_urls(array $subcontext, $component, $filearea, $itemid, $text): string { return str_replace('@@PLUGINFILE@@/', 'files/', $text ?? ''); } diff --git a/privacy/tests/moodle_content_writer_test.php b/privacy/tests/moodle_content_writer_test.php index d40736d1d4f..e2d8de216fe 100644 --- a/privacy/tests/moodle_content_writer_test.php +++ b/privacy/tests/moodle_content_writer_test.php @@ -1292,6 +1292,18 @@ class moodle_content_writer_test extends advanced_testcase { */ public function rewrite_pluginfile_urls_provider() { return [ + 'nullcontent' => [ + 'intro', + 0, + null, + '', + ], + 'emptycontent' => [ + 'intro', + 0, + '', + '', + ], 'zeroitemid' => [ 'intro', 0, From 8f4bd4ce1178d847a461ffc525e44893755ded62 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 9 Jan 2023 11:43:30 +0800 Subject: [PATCH 19/22] MDL-76362 core: Coding style fixes --- .../helper/restore_structure_parser_processor.class.php | 6 ++++-- backup/util/xml/output/xml_output.class.php | 2 +- backup/util/xml/xml_writer.class.php | 2 +- grade/report/singleview/classes/local/screen/user.php | 9 ++++----- lib/filestorage/file_storage.php | 2 +- lib/mathslib.php | 2 +- mod/workshop/classes/portfolio_caller.php | 2 +- 7 files changed, 13 insertions(+), 12 deletions(-) diff --git a/backup/util/helper/restore_structure_parser_processor.class.php b/backup/util/helper/restore_structure_parser_processor.class.php index fdd9cb927bc..67a7a2d9213 100644 --- a/backup/util/helper/restore_structure_parser_processor.class.php +++ b/backup/util/helper/restore_structure_parser_processor.class.php @@ -55,8 +55,10 @@ class restore_structure_parser_processor extends grouped_parser_processor { return ''; } else if (is_numeric($cdata)) { return $cdata; - } else if (strlen($cdata ?? '') < 32) { // Impossible to have one link in 32cc - return $cdata; // (http://10.0.0.1/file.php/1/1.jpg, http://10.0.0.1/mod/url/view.php?id=) + } else if (strlen($cdata ?? '') < 32) { + // Impossible to have one link in 32cc. + // (http://10.0.0.1/file.php/1/1.jpg, http://10.0.0.1/mod/url/view.php?id=). + return $cdata; } if (strpos($cdata, '$@FILEPHP@$') !== false) { diff --git a/backup/util/xml/output/xml_output.class.php b/backup/util/xml/output/xml_output.class.php index 942c8ec1579..059c2f95a08 100644 --- a/backup/util/xml/output/xml_output.class.php +++ b/backup/util/xml/output/xml_output.class.php @@ -107,7 +107,7 @@ abstract class xml_output { if (!$this->running) { throw new xml_output_exception('xml_output_not_started'); } - $lenc = strlen($content ?? ''); // Get length in bytes + $lenc = strlen($content ?? ''); // Get length in bytes. if ($lenc == 0) { // 0 length contents, nothing to do return; } diff --git a/backup/util/xml/xml_writer.class.php b/backup/util/xml/xml_writer.class.php index 39ebeeaa73d..4673e4da3d4 100644 --- a/backup/util/xml/xml_writer.class.php +++ b/backup/util/xml/xml_writer.class.php @@ -260,7 +260,7 @@ class xml_writer { * ignores the rest of characters. Also normalize linefeeds and return chars. */ protected function xml_safe_utf8($content) { - $content = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is','', $content ?? ''); // clean CTRL chars + $content = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is', '', $content ?? ''); // clean CTRL chars. $content = preg_replace("/\r\n|\r/", "\n", $content); // Normalize line&return=>line return $content; } diff --git a/grade/report/singleview/classes/local/screen/user.php b/grade/report/singleview/classes/local/screen/user.php index fb3a4b2a56a..efeffe92bb1 100644 --- a/grade/report/singleview/classes/local/screen/user.php +++ b/grade/report/singleview/classes/local/screen/user.php @@ -405,16 +405,15 @@ class user extends tablelike implements selectable_items { $gradeitem = grade_item::fetch([ 'courseid' => $this->courseid, - 'id' => $matches[1] + 'id' => $matches[1], ]); $isscale = ($gradeitem->gradetype == GRADE_TYPE_SCALE); - $empties = (trim($value ?? '') === '' or ($isscale and $value == -1)); + $empties = (trim($value ?? '') === '' || ($isscale && $value == -1)); - if ($filter == 'all' or $empties) { - $data->$varname = ($isscale and empty($insertvalue)) ? - -1 : $insertvalue; + if ($filter == 'all' || $empties) { + $data->$varname = ($isscale && empty($insertvalue)) ? -1 : $insertvalue; } } } diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index 8fe268de6ca..e3a852271ff 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -1800,7 +1800,7 @@ class file_storage { // the latter of which can go to 100, we need to make sure that quality here is // in a safe range or PHP WILL CRASH AND DIE. You have been warned. $quality = $quality > 9 ? (int)(max(1.0, (float)$quality / 100.0) * 9.0) : $quality; - imagepng($img, NULL, $quality, PNG_NO_FILTER); + imagepng($img, null, $quality, PNG_NO_FILTER); break; default: diff --git a/lib/mathslib.php b/lib/mathslib.php index 0b33da3b9d7..6cee99a3e32 100644 --- a/lib/mathslib.php +++ b/lib/mathslib.php @@ -123,7 +123,7 @@ class calc_formula { * @return string localised formula */ public static function localize($formula) { - $formula = str_replace('.', '$', $formula ?? ''); // temp placeholder + $formula = str_replace('.', '$', $formula ?? ''); // Temp placeholder. $formula = str_replace(',', get_string('listsep', 'langconfig'), $formula); $formula = str_replace('$', get_string('decsep', 'langconfig'), $formula); return $formula; diff --git a/mod/workshop/classes/portfolio_caller.php b/mod/workshop/classes/portfolio_caller.php index eb618fb7192..aaaad77f630 100644 --- a/mod/workshop/classes/portfolio_caller.php +++ b/mod/workshop/classes/portfolio_caller.php @@ -342,7 +342,7 @@ class mod_workshop_portfolio_caller extends portfolio_module_caller_base { } if ($this->workshop->overallfeedbackmode) { - if ($assessment->feedbackauthorattachment or trim($assessment->feedbackauthor ?? '') !== '') { + if ($assessment->feedbackauthorattachment || trim($assessment->feedbackauthor ?? '') !== '') { $output .= html_writer::tag('h3', get_string('overallfeedback', 'mod_workshop')); $content = $this->format_exported_text($assessment->feedbackauthor, $assessment->feedbackauthorformat); $content = portfolio_rewrite_pluginfile_urls($content, $this->workshop->context->id, 'mod_workshop', From e6dfa9ff3c1a265df6c85f010117096f2de0a7f7 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sat, 21 Jan 2023 13:24:00 +0100 Subject: [PATCH 20/22] MDL-76362 output: Preserve original behaviour for PHP 8.1 Whenever the page_requirements_manager::js_fix_url() is called with null url, it must throw an exception and emit 0 warnings. It's covered by an explicit test: test_js_fix_url_coding_exception with data set "Provide a null argument" --- lib/outputrequirementslib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/outputrequirementslib.php b/lib/outputrequirementslib.php index 07428ae29b8..109814a3f7e 100644 --- a/lib/outputrequirementslib.php +++ b/lib/outputrequirementslib.php @@ -720,7 +720,7 @@ class page_requirements_manager { if ($url instanceof moodle_url) { return $url; - } else if (strpos($url, '/') === 0) { + } else if (null !== $url && strpos($url, '/') === 0) { // Fix the admin links if needed. if ($CFG->admin !== 'admin') { if (strpos($url, "/admin/") === 0) { From 03bc275093b04a1e41682b246211aa270678e877 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sat, 21 Jan 2023 16:01:33 +0100 Subject: [PATCH 21/22] MDL-76362 enrol_lti: Prevent calling to DataConnector with null keys While it could have been fixed in DataConnector (3rd part lib), better prevent in our code to call to it with null keys. Covered by unit tests. --- enrol/lti/classes/data_connector.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/enrol/lti/classes/data_connector.php b/enrol/lti/classes/data_connector.php index ff5a50cfb70..430e14dd3ea 100644 --- a/enrol/lti/classes/data_connector.php +++ b/enrol/lti/classes/data_connector.php @@ -88,16 +88,18 @@ class data_connector extends DataConnector { global $DB; $id = $consumer->getRecordId(); + $key = $consumer->getKey(); + $result = false; if (!empty($id)) { $result = $DB->get_record($this->consumertable, ['id' => $id]); - } else { - $key256 = DataConnector::getConsumerKey($consumer->getKey()); + } else if (!empty($key)) { + $key256 = DataConnector::getConsumerKey($key); $result = $DB->get_record($this->consumertable, ['consumerkey256' => $key256]); } if ($result) { - if (empty($key256) || empty($result->consumerkey) || ($consumer->getKey() === $result->consumerkey)) { + if (empty($key256) || empty($result->consumerkey) || ($key === $result->consumerkey)) { $this->build_tool_consumer_object($result, $consumer); return true; } From 36b4f13e9b9057372c7097c77efcd4e446275a92 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sat, 21 Jan 2023 18:18:21 +0100 Subject: [PATCH 22/22] MDL-76362 workshop: check for empty (null included) feedbackreviewer As far as feedbackreviewer can be null, we cannot, since PHP 8.1 apply any string operation (trim, strlen...) on it, hence, checking before applying. --- mod/workshop/view.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/workshop/view.php b/mod/workshop/view.php index 67b8bec38de..c8957a23108 100644 --- a/mod/workshop/view.php +++ b/mod/workshop/view.php @@ -695,7 +695,7 @@ switch ($workshop->phase) { echo $output->render($workshop->prepare_submission_summary($submission, $shownames)); echo $output->box_end(); - if (strlen(trim($assessment->feedbackreviewer)) > 0) { + if (!empty($assessment->feedbackreviewer) && strlen(trim($assessment->feedbackreviewer)) > 0) { echo $output->render(new workshop_feedback_reviewer($assessment)); } }