From ef0a12bcfff4b443615d47f0614fc347c7d6b2a0 Mon Sep 17 00:00:00 2001 From: Stephan Robotta Date: Tue, 22 Oct 2024 17:48:40 +0200 Subject: [PATCH] Change column type from type to typeid in languages. Optimize string fetching for templates. --- classes/repository/instance_repository.php | 25 ++--- .../repository/model/db_localized_string.php | 6 +- .../model/localized_string_type.php | 92 +++++++++++-------- .../template_category_repository.php | 13 ++- .../template_criterion_repository.php | 48 +++++----- db/upgrade.php | 24 +++++ version.php | 2 +- 7 files changed, 129 insertions(+), 81 deletions(-) diff --git a/classes/repository/instance_repository.php b/classes/repository/instance_repository.php index 1f13ebf..8b9ae60 100644 --- a/classes/repository/instance_repository.php +++ b/classes/repository/instance_repository.php @@ -180,13 +180,16 @@ private static function get_strings(string $type, int $subratingid, bool $throwo if (!array_key_exists($cachekey, $sortedstrings) || PHPUNIT_TEST) { $sortedstrings[$cachekey] = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => $type, 'foreignkey' => $subratingid]); + $rs = $DB->get_recordset( + tables::LOCALIZED_STRING_TABLE, + ['typeid' => localized_string_type::str2id($type), 'foreignkey' => $subratingid] + ); foreach ($rs as $dboheader) { $dbobj = new db_localized_string; $dbobj->id = $dboheader->id; $dbobj->languageid = $dboheader->languageid; $dbobj->string = $dboheader->string; - $dbobj->type = $dboheader->type; + $dbobj->typeid = $dboheader->typeid; $dbobj->foreignkey = $dboheader->foreignkey; $sortedstrings[$cachekey][$dbobj->languageid] = $dbobj; @@ -277,7 +280,7 @@ public function save(instance $instance) { $id = $DB->insert_record(tables::INSTANCE_TABLE, $dboinstance); $instance->set_id($id); // Set the grade. - $DB->set_field('verbalfeedback', 'grade', $instance->grade, ['id' => $id]); + $DB->set_field(tables::INSTANCE_TABLE, 'grade', $instance->grade, ['id' => $id]); } else { $DB->update_record(tables::INSTANCE_TABLE, $dboinstance); } @@ -408,7 +411,7 @@ public function delete(int $instanceid) { foreach ($dbocategories as $dbocategory) { // Delete category headers. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_CATEGORY_HEADER, "foreignkey" => $dbocategory->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CATEGORY_HEADER), "foreignkey" => $dbocategory->id]); // Delete category criteria. $dbocriteria = $DB->get_records(tables::INSTANCE_CRITERION_TABLE, ["categoryid" => $dbocategory->id]); @@ -416,7 +419,7 @@ public function delete(int $instanceid) { // Delete criterion description. $dbodescriptions = $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_CRITERION, "foreignkey" => $dbocriterion->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_CRITERION), "foreignkey" => $dbocriterion->id]); // Delete criterion subratings. $dbosubratings = $DB->get_records(tables::INSTANCE_SUBRATING_TABLE, ["criterionid" => $dbocriterion->id]); @@ -424,27 +427,27 @@ public function delete(int $instanceid) { // Delete subrating titles. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_TITLE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_TITLE), "foreignkey" => $dbosubrating->id]); // Delete subrating descriptions. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_DESCRIPTION, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_DESCRIPTION), "foreignkey" => $dbosubrating->id]); // Delete subrating very negative texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_NEGATIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating negative texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_NEGATIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_NEGATIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating positive texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_POSITIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_POSITIVE), "foreignkey" => $dbosubrating->id]); // Delete subrating very positive texts. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, - ["type" => localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE, "foreignkey" => $dbosubrating->id]); + ["typeid" => localized_string_type::str2id(localized_string_type::INSTANCE_SUBRATING_VERY_POSITIVE), "foreignkey" => $dbosubrating->id]); } // Delete subratings. diff --git a/classes/repository/model/db_localized_string.php b/classes/repository/model/db_localized_string.php index 9ff237e..f3c14dc 100644 --- a/classes/repository/model/db_localized_string.php +++ b/classes/repository/model/db_localized_string.php @@ -34,9 +34,9 @@ class db_localized_string { */ public $id; /** - * @var string The type + * @var int The type */ - public $type; + public $typeid; /** * @var int The foreign key */ @@ -66,7 +66,7 @@ public static function from_localized_string(localized_string $localizedstring, } $dbo = new db_localized_string(); $dbo->id = $localizedstring->get_id(); - $dbo->type = $type; + $dbo->typeid = localized_string_type::str2id($type); $dbo->foreignkey = $foreignkey; $dbo->languageid = $localizedstring->get_language_id(); $dbo->string = $localizedstring->get_string(); diff --git a/classes/repository/model/localized_string_type.php b/classes/repository/model/localized_string_type.php index 4c028c2..531cac0 100644 --- a/classes/repository/model/localized_string_type.php +++ b/classes/repository/model/localized_string_type.php @@ -62,48 +62,68 @@ class localized_string_type { public const TEMPLATE_SUBRATING_VERY_POSITIVE = 'template_subrating_verypositive'; /** - * Return whether a type exists + * Return all existing types in a order, so that the numeric value + 1 can + * be used as a string. + * + * @return string[] + */ + public static function getStringTypes(): array { + return [ + self::INSTANCE_CRITERION, + self::INSTANCE_CATEGORY_HEADER, + self::INSTANCE_SUBRATING_TITLE, + self::INSTANCE_SUBRATING_DESCRIPTION, + self::INSTANCE_SUBRATING_VERY_NEGATIVE, + self::INSTANCE_SUBRATING_NEGATIVE, + self::INSTANCE_SUBRATING_POSITIVE, + self::INSTANCE_SUBRATING_VERY_POSITIVE, + self::TEMPLATE_CRITERION, + self::TEMPLATE_CATEGORY_HEADER, + self::TEMPLATE_SUBRATING_TITLE, + self::TEMPLATE_SUBRATING_DESCRIPTION, + self::TEMPLATE_SUBRATING_VERY_NEGATIVE, + self::TEMPLATE_SUBRATING_NEGATIVE, + self::TEMPLATE_SUBRATING_POSITIVE, + self::TEMPLATE_SUBRATING_VERY_POSITIVE, + ]; + } + /** + * Return whether a type string exists. + * * @param string $type A string type * @return bool If a string type exists */ public static function exists(string $type) { - switch($type) { - case self::INSTANCE_CRITERION: - return true; - case self::INSTANCE_CATEGORY_HEADER: - return true; - case self::INSTANCE_SUBRATING_TITLE: - return true; - case self::INSTANCE_SUBRATING_DESCRIPTION: - return true; - case self::INSTANCE_SUBRATING_VERY_NEGATIVE: - return true; - case self::INSTANCE_SUBRATING_NEGATIVE: - return true; - case self::INSTANCE_SUBRATING_POSITIVE: - return true; - case self::INSTANCE_SUBRATING_VERY_POSITIVE: - return true; + return in_array($type, self::getStringTypes()); + } - case self::TEMPLATE_CRITERION: - return true; - case self::TEMPLATE_CATEGORY_HEADER: - return true; - case self::TEMPLATE_SUBRATING_TITLE: - return true; - case self::TEMPLATE_SUBRATING_DESCRIPTION: - return true; - case self::TEMPLATE_SUBRATING_VERY_NEGATIVE: - return true; - case self::TEMPLATE_SUBRATING_NEGATIVE: - return true; - case self::TEMPLATE_SUBRATING_POSITIVE: - return true; - case self::TEMPLATE_SUBRATING_VERY_POSITIVE: - return true; + /** + * Convert string constant to id. + * + * @param string $type + * @return int + * @throws \InvalidArgumentException + */ + public static function str2id(string $type):int { + $key = array_search($type, self::getStringTypes()); + if ($key === false) { + throw new \InvalidArgumentException("Invalid str: $type"); + } + return $key + 1; + } - default: - return false; + /** + * Convert id to string contant. + * + * @param int $id + * @return string + * @throws \InvalidArgumentException + */ + public static function id2str(int $id):string { + $constants = self::getStringTypes(); + if ($id < 1 || $id > count($constants)) { + throw new \InvalidArgumentException("Invalid id: $id"); } + return $constants[$id - 1]; } } diff --git a/classes/repository/template_category_repository.php b/classes/repository/template_category_repository.php index f958821..c156b67 100644 --- a/classes/repository/template_category_repository.php +++ b/classes/repository/template_category_repository.php @@ -140,8 +140,10 @@ public function delete_by_id(int $id): bool { global $DB; try { $transaction = $DB->start_delegated_transaction(); - $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id, - 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER, ]); + $DB->delete_records(tables::LOCALIZED_STRING_TABLE, [ + 'foreignkey' => $id, + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER), + ]); $DB->delete_records(tables::PARAMETRIZED_TEMPLATE_CRITERION_TABLE, ['categoryid' => $id]); $DB->delete_records(tables::TEMPLATE_CATEGORY_TABLE, ['id' => $id]); @@ -163,7 +165,7 @@ private function get_headers($foreignkey): array { global $DB; $dboheaders = $DB->get_records(tables::LOCALIZED_STRING_TABLE, - ['foreignkey' => $foreignkey, 'type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); + ['foreignkey' => $foreignkey, 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER)]); $headers = []; foreach ($dboheaders as $dboheader) { $headers[] = db_localized_string::to_localized_string($dboheader); @@ -182,7 +184,10 @@ private function get_headers_by_category_ids(): array { global $DB; $headers = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => localized_string_type::TEMPLATE_CATEGORY_HEADER]); + $rs = $DB->get_recordset( + tables::LOCALIZED_STRING_TABLE, + ['typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CATEGORY_HEADER)] + ); foreach ($rs as $row) { if (!\array_key_exists($row->foreignkey, $headers)) { $headers[$row->foreignkey] = []; diff --git a/classes/repository/template_criterion_repository.php b/classes/repository/template_criterion_repository.php index 61c1caa..ac787a9 100644 --- a/classes/repository/template_criterion_repository.php +++ b/classes/repository/template_criterion_repository.php @@ -243,23 +243,23 @@ public function delete(int $id) { $dbosubratings = $DB->get_records(tables::TEMPLATE_SUBRATINGS_TABLE, ['criterionid' => $id]); foreach ($dbosubratings as $dbosubrating) { $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_TITLE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_TITLE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_DESCRIPTION), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_NEGATIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_NEGATIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_NEGATIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_POSITIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_POSITIVE), ], ); $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $dbosubrating->id, - 'type' => localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_SUBRATING_VERY_POSITIVE), ], ); } $DB->delete_records(tables::TEMPLATE_SUBRATINGS_TABLE, ['criterionid' => $id]); // Delete localized strings. $DB->delete_records(tables::LOCALIZED_STRING_TABLE, ['foreignkey' => $id, - 'type' => localized_string_type::TEMPLATE_CRITERION, ], ); + 'typeid' => localized_string_type::str2id(localized_string_type::TEMPLATE_CRITERION), ], ); // Delete criterion. $DB->delete_records(tables::TEMPLATE_CRITERION_TABLE, ['id' => $id]); @@ -286,36 +286,33 @@ public static function dbo_to_template_criterion($dbo) { /** * Get all localized strings for a type, hashed by foreignkey, cached in memory for speed. * - * // phpcs:ignore moodle.Commenting.ValidTags.Invalid - * @TODO - evaluate this for memory usage. * @param string $type + * @param int $foreignkey * @return array * @throws \dml_exception */ - private function get_all_localized_strings_for_type(string $type): array { + private function get_all_localized_strings_for_type(string $type, int $foreignkey): array { global $DB; static $strings = []; - if (isset($strings[$type]) && !PHPUNIT_TEST) { + $typeid = localized_string_type::str2id($type); + $cachekey = $typeid . '~~' . $foreignkey; + + if (array_key_exists($cachekey, $strings) && !PHPUNIT_TEST) { // We already have this cached, so return it. - return $strings[$type]; + return $strings[$cachekey]; } - $strings[$type] = []; + $strings[$cachekey] = []; - $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['type' => $type]); + $rs = $DB->get_recordset(tables::LOCALIZED_STRING_TABLE, ['typeid' => $typeid, 'foreignkey' => $foreignkey]); foreach ($rs as $row) { - $type = $row->type; - $key = $row->foreignkey; - if (!isset($strings[$type][$key])) { - $strings[$type][$key] = []; - } - $strings[$type][$key][$row->id] = db_localized_string::to_localized_string($row); + $strings[$cachekey][$row->id] = db_localized_string::to_localized_string($row); } $rs->close(); - return $strings[$type]; + return $strings[$cachekey]; } /** @@ -331,13 +328,12 @@ private function get_localized_strings(int $foreignkey, string $type): array { throw new \Exception("Unknown localized string type."); } - $allstrings = $this->get_all_localized_strings_for_type($type); - $localizedstrings = $allstrings[$foreignkey] ?? null; - if (!$localizedstrings) { - throw new coding_exception("Couldn't find localized strings by type '$type' and foreignkey '$foreignkey'"); + $allstrings = $this->get_all_localized_strings_for_type($type, $foreignkey); + if (empty($allstrings)) { + throw new \coding_exception("Couldn't find localized strings by type '$type' and foreignkey '$foreignkey'"); } - return $localizedstrings; + return $allstrings; } /** diff --git a/db/upgrade.php b/db/upgrade.php index 66358fe..284ba38 100644 --- a/db/upgrade.php +++ b/db/upgrade.php @@ -82,5 +82,29 @@ function xmldb_verbalfeedback_upgrade($oldversion) { upgrade_main_savepoint(true, 2021100103); } + if ($oldversion < 2024101700) { + require_once($CFG->dirroot . '/mod/verbalfeedback/classes/repository/model/localized_string_type.php'); + $table = new xmldb_table('verbalfeedback_local_string'); + $field = new xmldb_field('typeid', XMLDB_TYPE_INTEGER, '3', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, 0); + + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Convert all strings to ids. + $offset = 0; + while ($records = $DB->get_records('verbalfeedback_local_string', null, 'id', 'id, type, typeid', $offset, 100)) { + foreach ($records as $record) { + $record->typeid = \mod_verbalfeedback\repository\model\localized_string_type::str2id($record->type); + $DB->update_record('verbalfeedback_local_string', $record); + } + $offset += 100; + } + $dbman->drop_field($table, new xmldb_field('type')); + $table->add_index('subitemtype', XMLDB_INDEX_NOTUNIQUE, ['foreignkey', 'typeid']); + + upgrade_mod_savepoint(true, 2024101700, 'verbalfeedback'); + } + return true; } diff --git a/version.php b/version.php index e38601a..70f76df 100644 --- a/version.php +++ b/version.php @@ -25,7 +25,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_verbalfeedback'; -$plugin->version = 2024071300; +$plugin->version = 2024101700; $plugin->requires = 2022112800; $plugin->maturity = MATURITY_STABLE; $plugin->cron = 0;