MDL-46739 user preferences: change value column to TEXT.

Other similar columns config.value and config_plugins.value are TEXT, so it is surprising that this column is different, and that has lead to bugs in the past, so we should make it consistent.
marxjohnson · Nov 11, 2024 · 8f9b57d · 8f9b57d
1 parent ec7711b
commit 8f9b57d
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 22 deletions.
diff --git a/.upgradenotes/MDL-46739-2024091010555321.yml b/.upgradenotes/MDL-46739-2024091010555321.yml
@@ -0,0 +1,10 @@
+issueNumber: MDL-46739
+notes:
+  core:
+    - message: >-
+        The {user_preferences}.value database field is now TEXT instead of CHAR.
+        This means that any queries with a condition on this field in a WHERE or
+        JOIN statement will need updating to use `$DB->sql_compare_text()`. See
+        the `$newusers` query in
+        `\core\task\send_new_users_password_task::execute` for an example.
+      type: changed
diff --git a/blocks/online_users/classes/fetcher.php b/blocks/online_users/classes/fetcher.php
@@ -80,7 +80,7 @@ protected function set_sql($currentgroup, $now, $timetoshowusers, $context, $sit
         $uservisibilityselect = "";
         if ($CFG->block_online_users_onlinestatushiding) {
             $uservisibility = ", up.value AS uservisibility";
-            $uservisibilityselect = "AND (" . $DB->sql_cast_char2int('up.value') . " = 1
+            $uservisibilityselect = "AND (" . $DB->sql_cast_char2int('up.value', true) . " = 1
                                     OR up.value IS NULL
                                     OR u.id = :userid)";
         }
@@ -97,7 +97,7 @@ protected function set_sql($currentgroup, $now, $timetoshowusers, $context, $sit
             $lastaccess = ", MAX(u.lastaccess) AS lastaccess";
             $timeaccess = ", MAX(ul.timeaccess) AS lastaccess";
             if ($CFG->block_online_users_onlinestatushiding) {
-                $uservisibility = ", MAX(up.value) AS uservisibility";
+                $uservisibility = ", MAX(" . $DB->sql_cast_char2int('up.value', true) . ") AS uservisibility";
             }
             $params['currentgroup'] = $currentgroup;
         }

diff --git a/lib/classes/task/send_new_user_passwords_task.php b/lib/classes/task/send_new_user_passwords_task.php
@@ -45,7 +45,13 @@ public function execute() {
         global $DB;
 
         // Generate new password emails for users - ppl expect these generated asap.
-        if ($DB->count_records('user_preferences', array('name' => 'create_password', 'value' => '1'))) {
+        if (
+            $DB->record_exists_select(
+                'user_preferences',
+                'name = ? AND ' . $DB->sql_compare_text('value', 2) . ' = ?',
+                ['create_password', '1']
+            )
+        ) {
             mtrace('Creating passwords for new users...');
             $userfieldsapi = \core_user\fields::for_name();
             $usernamefields = $userfieldsapi->get_sql('u', false, '', '', false)->selects;
@@ -54,10 +60,13 @@ public function execute() {
                                                      $usernamefields, u.username, u.lang,
                                                      p.id as prefid
                                                 FROM {user} u
-                                                JOIN {user_preferences} p ON u.id=p.userid
-                                               WHERE p.name='create_password' AND p.value='1' AND
-                                                     u.email !='' AND u.suspended = 0 AND
-                                                     u.auth != 'nologin' AND u.deleted = 0");
+                                                JOIN {user_preferences} p ON u.id = p.userid
+                                               WHERE p.name = 'create_password'
+                                                 AND " . $DB->sql_compare_text('p.value', 2) . " = '1'
+                                                 AND u.email <> ''
+                                                 AND u.suspended = 0
+                                                 AND u.auth <> 'nologin'
+                                                 AND u.deleted = 0");
 
             // Note: we can not send emails to suspended accounts.
             foreach ($newusers as $newuser) {

diff --git a/lib/db/install.xml b/lib/db/install.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
-<XMLDB PATH="lib/db" VERSION="20240923" COMMENT="XMLDB file for core Moodle tables"
+<XMLDB PATH="lib/db" VERSION="20241001" COMMENT="XMLDB file for core Moodle tables"
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:noNamespaceSchemaLocation="../../lib/xmldb/xmldb.xsd"
 >
@@ -939,7 +939,7 @@
         <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
         <FIELD NAME="userid" TYPE="int" LENGTH="10" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/>
         <FIELD NAME="name" TYPE="char" LENGTH="255" NOTNULL="true" SEQUENCE="false"/>
-        <FIELD NAME="value" TYPE="char" LENGTH="1333" NOTNULL="true" SEQUENCE="false"/>
+        <FIELD NAME="value" TYPE="text" NOTNULL="true" SEQUENCE="false"/>
       </FIELDS>
       <KEYS>
         <KEY NAME="primary" TYPE="primary" FIELDS="id"/>

diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php
@@ -1447,5 +1447,17 @@ function xmldb_main_upgrade($oldversion) {
     // Automatically generated Moodle v4.5.0 release upgrade line.
     // Put any upgrade step following this.
 
+    if ($oldversion < 2024102500.01) {
+        // Changing type of field value on table user_preferences to text.
+        $table = new xmldb_table('user_preferences');
+        $field = new xmldb_field('value', XMLDB_TYPE_TEXT, null, null, XMLDB_NOTNULL, null, null, 'name');
+
+        // Launch change of type for field value.
+        $dbman->change_field_type($table, $field);
+
+        // Main savepoint reached.
+        upgrade_main_savepoint(true, 2024102500.01);
+    }
+
     return true;
 }
diff --git a/lib/moodlelib.php b/lib/moodlelib.php
@@ -1490,11 +1490,7 @@ function set_user_preference($name, $value, $user = null) {
     } else if (is_array($value)) {
         throw new coding_exception('Invalid value in set_user_preference() call, arrays are not allowed');
     }
-    // Value column maximum length is 1333 characters.
     $value = (string)$value;
-    if (core_text::strlen($value) > 1333) {
-        throw new coding_exception('Invalid value in set_user_preference() call, value is is too long for the value column');
-    }
 
     if (is_null($user)) {
         $user = $USER;

diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php
@@ -1495,14 +1495,14 @@ public function test_set_user_preference(): void {
         $this->assertEquals($longvalue,
             $DB->get_field('user_preferences', 'value', array('userid' => $USER->id, 'name' => '_test_long_user_preference')));
 
-        // Test > 1333 char values, coding_exception expected.
-        $longvalue = str_repeat('a', 1334);
-        try {
-            set_user_preference('_test_long_user_preference', $longvalue);
-            $this->fail('Exception expected - longer than 1333 chars not allowed as preference value');
-        } catch (\moodle_exception $ex) {
-            $this->assertInstanceOf('coding_exception', $ex);
-        }
+        // Larger preference values are allowed as of MDL-46739.
+        $longervalue = str_repeat('a', 1334);
+        set_user_preference('_test_long_user_preference', $longervalue);
+        $this->assertEquals($longervalue, get_user_preferences('_test_long_user_preference'));
+        $this->assertEquals(
+            $longervalue,
+            $DB->get_field('user_preferences', 'value', ['userid' => $USER->id, 'name' => '_test_long_user_preference'])
+        );
 
         // Test invalid params.
         try {

diff --git a/version.php b/version.php
@@ -29,7 +29,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$version  = 2024102500.00;              // YYYYMMDD      = weekly release date of this DEV branch.
+$version  = 2024102500.01;              // YYYYMMDD      = weekly release date of this DEV branch.
                                         //         RR    = release increments - 00 in DEV branches.
                                         //           .XX = incremental changes.
 $release  = '5.0dev (Build: 20241025)';    // Human-friendly version name