Moodle
  1. Moodle
  2. MDL-20245

Allow bigger values (up to 1333 chars) in user preferences, alleviating some problems with grader report

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      1) Go into the grader report for a course containing several categories. Collapse and re-expand several of them by clicking the +, o, - symbol next to the category name. Check that they can be collapsed and re-expanded.

      2) Run the lib/simpletest/testmoodlelib.php unit tests. Not failure related with "test_set_user_preference" should happen. That ensures that storage up to 1333 chars is working ok.

      Show
      1) Go into the grader report for a course containing several categories. Collapse and re-expand several of them by clicking the +, o, - symbol next to the category name. Check that they can be collapsed and re-expanded. 2) Run the lib/simpletest/testmoodlelib.php unit tests. Not failure related with "test_set_user_preference" should happen. That ensures that storage up to 1333 chars is working ok.
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-20245_user_preferences2

      Description

      Noticed in my Postgres log that inserts into the user_preference table have been failing. Here is the log entry:

      ERROR: value too long for type character varying(255)
      STATEMENT: UPDATE mdl_user_preferences SET value = 'a:2:{s:14:\"aggregatesonly\";a:11:

      {i:41;s:4:\"1621\";i:46;s:4:\"1466\";i:47;s:4:\"1498\";i:48;s:4:\"1470 \";i:49;s:4:\"1622\";i:50;s:4:\"1623\";i:51;s:4:\"1465\";i:52;s:4:\"1668\";i:53;s:4:\"1669\";i:54;s:4:\"1670\";i:55;s:4:\"1671\";}

      s:10:\"gradesonly\";a:2:{i:1;s:4:\"1620\";i:2;s:4:\"3024\";}}' WHERE id =
      '1619';

      A quick look at the DB for the record in question:

      select * from mdl_user_preferences where id = '1619';
      id | userid | name | value
      ---------------------------------------------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      1619 | 12405 | grade_report_grader_collapsed_categories | a:2:{s:14:"aggregatesonly";a:11:

      {i:41;s:4:"1621";i:46;s:4:"1466";i:47;s:4:"1498";i:48;s:4:"1470";i:49;s:4:"1622";i:50;s:4:"1623";i:51;s:4:"1465";i:52;s:4:"1668";i:53;s:4:"1669";i:54;s:4:"1670";i:55;s:4:"1671";}

      s:10:"gradesonly";a:1:{i:1;s:4:"1620";}}

      Unfortunately, I cannot determine how exactly this was triggered. I
      cannot decipher exactly what was being clicked on by the user at the
      time these things were logged. Any tips on recreating the problem?

      If I were to guess, I would say that there is a problem where a
      person may belong to too many courses with too many categories and
      have PHP pref arrays that don't serialize to a small enough value.
      Perhaps the size of the serialized value should be checked or else a
      note presented to the user to that the settings were not saved?

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Eloy Lafuente (stronk7) added a comment -

            Uhm... that field for user preferences is supposed to be used to store simple values and not serialised info, that can easily grow over 255cc.

            Asigning to Nico (and addressing to 1.9.6) as seems that Gradebook is the responsible for this... couldn't we split that info into smaller (ideally individual) parts or so?

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Uhm... that field for user preferences is supposed to be used to store simple values and not serialised info, that can easily grow over 255cc. Asigning to Nico (and addressing to 1.9.6) as seems that Gradebook is the responsible for this... couldn't we split that info into smaller (ideally individual) parts or so? Ciao
            Hide
            Petr Skoda added a comment -

            hmm, I can imagine putting a lot more than 255 there, maybe we could change it in 2.0?

            Show
            Petr Skoda added a comment - hmm, I can imagine putting a lot more than 255 there, maybe we could change it in 2.0?
            Hide
            Tim Hunt added a comment -

            Hey, I just got a report of this at the OU, and luckily I was a good boy and searched before creating a duplicate.

            I think that value should be TEXT, like in config and config_plugins.

            I also thing that using serialise PHP for this particular pref sucks badly. I think that the gradebook user prefs should be stored in human-readable key-value pairs.

            I suppose that means that this bug should probably be split into two. One for changing the column type, which I think should happen in 2.0, and another for fixing gradebook user prefs later.

            Show
            Tim Hunt added a comment - Hey, I just got a report of this at the OU, and luckily I was a good boy and searched before creating a duplicate. I think that value should be TEXT, like in config and config_plugins. I also thing that using serialise PHP for this particular pref sucks badly. I think that the gradebook user prefs should be stored in human-readable key-value pairs. I suppose that means that this bug should probably be split into two. One for changing the column type, which I think should happen in 2.0, and another for fixing gradebook user prefs later.
            Hide
            Matthew Davidson added a comment -

            this issue needs to be fixed, it causes failed sql. We are running the latest 1.9, but we also see that Moodle 2.0 has this issue as well.

            Show
            Matthew Davidson added a comment - this issue needs to be fixed, it causes failed sql. We are running the latest 1.9, but we also see that Moodle 2.0 has this issue as well.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Raising this to critical, setting affected versions to 1.9.x and 2.0.x and sending to stable backlog. Needs fix in both branches IMO.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Raising this to critical, setting affected versions to 1.9.x and 2.0.x and sending to stable backlog. Needs fix in both branches IMO. Ciao
            Hide
            Charles Fulton added a comment -

            If I'm reading the code right there's just the two subfields, so splitting them out shouldn't be a big deal. https://github.com/mackensen/moodle/commit/b7a9176d8ef4374cfb0ccdce8078ec9120c50807 I think does this in 2.0.2+ and applies fairly cleanly against 1.9 as well. Tested on MySQL only but should be db-neutral.

            Show
            Charles Fulton added a comment - If I'm reading the code right there's just the two subfields, so splitting them out shouldn't be a big deal. https://github.com/mackensen/moodle/commit/b7a9176d8ef4374cfb0ccdce8078ec9120c50807 I think does this in 2.0.2+ and applies fairly cleanly against 1.9 as well. Tested on MySQL only but should be db-neutral.
            Hide
            Charles Fulton added a comment -

            Note, grepping the repository didn't reveal any other obvious cases of serialized preferences.

            Show
            Charles Fulton added a comment - Note, grepping the repository didn't reveal any other obvious cases of serialized preferences.
            Hide
            Ben Beachy added a comment -

            A triggering event is a click on the category aggregates only icon (box with minus sign) in the grader report (Course > Settings > Grades). When the field is full (has 255 characters) the user receives a 'Error writing to database'.

            Presumably the field fills as users switch grade categories to aggregate only. In our system only two faculty out of approximately five hundred have encountered this problem, though. It's possible that another bug is contributing to the problem. Or it's possible that most faculty don't hide grade categories. Or maybe I don't understand how this field is populated.

            I've worked around this problem by setting the user_preferences.value field to type 'text'. This sets the field limit to 65k characters.

            Show
            Ben Beachy added a comment - A triggering event is a click on the category aggregates only icon (box with minus sign) in the grader report (Course > Settings > Grades). When the field is full (has 255 characters) the user receives a 'Error writing to database'. Presumably the field fills as users switch grade categories to aggregate only. In our system only two faculty out of approximately five hundred have encountered this problem, though. It's possible that another bug is contributing to the problem. Or it's possible that most faculty don't hide grade categories. Or maybe I don't understand how this field is populated. I've worked around this problem by setting the user_preferences.value field to type 'text'. This sets the field limit to 65k characters.
            Hide
            Iñigo Zendegi added a comment -

            Hello,

            I´ve just realized that this issue also affects 2.1.* versions so I´ve voted for it,

            Meanwhile, what can I do to solve it? Can I just extend the column size or this can make some other things fail?

            Thanks in advance.

            Show
            Iñigo Zendegi added a comment - Hello, I´ve just realized that this issue also affects 2.1.* versions so I´ve voted for it, Meanwhile, what can I do to solve it? Can I just extend the column size or this can make some other things fail? Thanks in advance.
            Hide
            Petr Skoda added a comment -

            master branch (2.2dev) now contains support for varchar fields up to 1333 chars long

            Show
            Petr Skoda added a comment - master branch (2.2dev) now contains support for varchar fields up to 1333 chars long
            Hide
            Iñigo Zendegi added a comment -

            Thanks Petr!

            Until 2.2 version comes out could I resize this column manually in the database (up to 1000 chars long, for example) or should I wait?

            Show
            Iñigo Zendegi added a comment - Thanks Petr! Until 2.2 version comes out could I resize this column manually in the database (up to 1000 chars long, for example) or should I wait?
            Hide
            Iñigo Zendegi added a comment -

            Hello again,

            I'm being pressed to solve this issue ASAP, so I've tried to resize this column manually (up to 500) and realized that this change fixes the issue, but I'm afraid of this manual change causing problems in further moodle upgrades.

            Do you suggest me to apply this change on the real moodle site or should I wait?

            Thanks.

            Show
            Iñigo Zendegi added a comment - Hello again, I'm being pressed to solve this issue ASAP, so I've tried to resize this column manually (up to 500) and realized that this change fixes the issue, but I'm afraid of this manual change causing problems in further moodle upgrades. Do you suggest me to apply this change on the real moodle site or should I wait? Thanks.
            Hide
            Andrew Davis added a comment -

            Ive closed the duplicate bug reports for this issue.

            Show
            Andrew Davis added a comment - Ive closed the duplicate bug reports for this issue.
            Hide
            Daniel M. Zimmerman added a comment -

            For what it's worth, this happens on MySQL-based systems too (at least on mine, with Moodle 2.1.1+ (20110817) running; I'll be upgrading soon). Is there a safe way to fix the problem without doing an upgrade?

            Show
            Daniel M. Zimmerman added a comment - For what it's worth, this happens on MySQL-based systems too (at least on mine, with Moodle 2.1.1+ (20110817) running; I'll be upgrading soon). Is there a safe way to fix the problem without doing an upgrade?
            Hide
            Wes Matchett added a comment -

            I am having this same problem in several courses. The Moodle version is 2.1.2+ (build 20111012) using MySQL.

            Since the problem returns an error to the user and prevents the gradebook from collapsing I have adjusted the mdl_user_preferences.value size to varchar(512). This prevents the error for now. I am also voting for this to be solved.

            Show
            Wes Matchett added a comment - I am having this same problem in several courses. The Moodle version is 2.1.2+ (build 20111012) using MySQL. Since the problem returns an error to the user and prevents the gradebook from collapsing I have adjusted the mdl_user_preferences.value size to varchar(512). This prevents the error for now. I am also voting for this to be solved.
            Hide
            Andrew Davis added a comment -

            I'm adding branch information that contains a potential fix for this. I've changed the user_preferences column "value" from varchar to text as is the case in the config table.

            If you have manually increased the size of the value column that should be fine. Assuming the fix I have proposed gets accepted you can safely increase the size of the column.

            Do not apply the fix I have provided on production servers until after it has been through our review and testing process.

            I will raise a separate issue to change the grader report to not store serialized data in the user_preferences table.

            Show
            Andrew Davis added a comment - I'm adding branch information that contains a potential fix for this. I've changed the user_preferences column "value" from varchar to text as is the case in the config table. If you have manually increased the size of the value column that should be fine. Assuming the fix I have proposed gets accepted you can safely increase the size of the column. Do not apply the fix I have provided on production servers until after it has been through our review and testing process. I will raise a separate issue to change the grader report to not store serialized data in the user_preferences table.
            Hide
            Andrew Davis added a comment -

            I've raised MDL-30668 to look at not storing serialized data in the user_preferences table.

            Show
            Andrew Davis added a comment - I've raised MDL-30668 to look at not storing serialized data in the user_preferences table.
            Hide
            Petr Skoda added a comment -

            First of all small text is deprecated, new fields should use only big text. Also we should not change varchar to text like this because sql that joins into preference values would break badly (such as our cron and contrib)!

            Anyway why do we still use 255 in 2.2 stable?

            Show
            Petr Skoda added a comment - First of all small text is deprecated, new fields should use only big text. Also we should not change varchar to text like this because sql that joins into preference values would break badly (such as our cron and contrib)! Anyway why do we still use 255 in 2.2 stable?
            Hide
            Andrew Davis added a comment -

            Ok, so in your opinion am I better off simply increasing the size of the column?

            Theres no 2.2 branch yet because it hasnt been through peer review

            Show
            Andrew Davis added a comment - Ok, so in your opinion am I better off simply increasing the size of the column? Theres no 2.2 branch yet because it hasnt been through peer review
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            You asked me to take a look at this yesterday, in fact I've done the lazy thing and had a discussion with Eloy about it (thank you Eloy).
            We walked through the different options that we could see and we've ended up settling on a completely different idea to anything purposed here.
            There were three options we considered:

            1. Changing to text, we didn't get very far with that, it's not bc compatible, surrounding code would need review + upgrade (as Petr mentioned cron for instance), and we'd need to ensure all code used the user pref API. Easiest solution certainly, good for future code, but not great for performance and bc kill it.
            2. A two part solution, review the gradebook usage and shrink it's storage in some way. At the same time increase the field size to 1333 for 2.2 and master. < 2.2 max size is 255 still so really the gradebook review + changes are the only leg this stands on. At the same time it doesn't actually solve the problem, that developers will still in the future store large values and even at 1333 chars it'd only be a matter of time before we hit the next issue.
            3. Third solution (credit to Eloy here) introduce a new text field in the user pref table for storing large values (lets call it valueoversized) and then altering the uesr prefs API to deal with that. Essentially the getter would look for the first not null value, and the setter would decide where to store the value depending upon the size of it, e.g. value > 255 ? valueoversized : value. With this solution the API handles the madness and code can store anything it wants there (future proof). It's completely backwards compatible (nothing old requires changing) and its safe to join on the table with all fields except the new one. The downsides, there is a new field so performance, however all user prefs are cached in the session after first load so that won't be bad, and the other downside is that as part of the setter we will next textlib so that we can get the size of the value reliably. Of course in 2.2 and master we would still increase value to 1333 so that there was even less chance of using the text field.

            Anyway the conclusion that we came to was that introducing the new field and using that for large values as described is the least evil and most compatible solution.
            What are everyones thoughts on that?

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, You asked me to take a look at this yesterday, in fact I've done the lazy thing and had a discussion with Eloy about it (thank you Eloy). We walked through the different options that we could see and we've ended up settling on a completely different idea to anything purposed here. There were three options we considered: Changing to text, we didn't get very far with that, it's not bc compatible, surrounding code would need review + upgrade (as Petr mentioned cron for instance), and we'd need to ensure all code used the user pref API. Easiest solution certainly, good for future code, but not great for performance and bc kill it. A two part solution, review the gradebook usage and shrink it's storage in some way. At the same time increase the field size to 1333 for 2.2 and master. < 2.2 max size is 255 still so really the gradebook review + changes are the only leg this stands on. At the same time it doesn't actually solve the problem, that developers will still in the future store large values and even at 1333 chars it'd only be a matter of time before we hit the next issue. Third solution (credit to Eloy here) introduce a new text field in the user pref table for storing large values (lets call it valueoversized) and then altering the uesr prefs API to deal with that. Essentially the getter would look for the first not null value, and the setter would decide where to store the value depending upon the size of it, e.g. value > 255 ? valueoversized : value. With this solution the API handles the madness and code can store anything it wants there (future proof). It's completely backwards compatible (nothing old requires changing) and its safe to join on the table with all fields except the new one. The downsides, there is a new field so performance, however all user prefs are cached in the session after first load so that won't be bad, and the other downside is that as part of the setter we will next textlib so that we can get the size of the value reliably. Of course in 2.2 and master we would still increase value to 1333 so that there was even less chance of using the text field. Anyway the conclusion that we came to was that introducing the new field and using that for large values as described is the least evil and most compatible solution. What are everyones thoughts on that? Cheers Sam
            Hide
            Andrew Davis added a comment - - edited

            Ill have to have a think about this...

            Option 3 seems like a very invasive change Vs just increasing the size of the existing varchar field however you make an annoyingly good case Adding the new field certainly seems like it would resolve this once and for all.

            Show
            Andrew Davis added a comment - - edited Ill have to have a think about this... Option 3 seems like a very invasive change Vs just increasing the size of the existing varchar field however you make an annoyingly good case Adding the new field certainly seems like it would resolve this once and for all.
            Hide
            Andrew Davis added a comment - - edited

            Just recording some notes on how the valueoversized system could work to make sure I understand.

            We increase the length of user_preferences->value to 1333.

            check_user_preferences_loaded() currently uses $DB->get_records_menu() which can only handle 2 columns (name, value). It would switch to $DB->get_records() to get 3 columns (name, value, valueoversized). Then we would iterate over the returned array and compress it down to name and value by doing something like...

            $user->preference[$name] = !empty($value) ? $value : $valueoversized;
            

            Is there a way to do that that doesn't involve looping over every preference?

            set_user_preference() would need to check the length of the value using textlib::strlen(). If its above 1333 (define a new constant for this I guess) it goes into valueoversized and value is set to null. This section of code would need to be altered to deal with both value and valueoversized

            if ($preference = $DB->get_record('user_preferences', array('userid'=>$user->id, 'name'=>$name))) {
                    if ($preference->value === $value and isset($user->preference[$name]) and $user->preference[$name] === $value) {
                        // preference already set to this value
                        return true;
                    }
                    $DB->set_field('user_preferences', 'value', $value, array('id'=>$preference->id));
             
                } else {
                    $preference = new stdClass();
                    $preference->userid = $user->id;
                    $preference->name   = $name;
                    $preference->value  = $value;
                    $DB->insert_record('user_preferences', $preference);
                }
            

            Show
            Andrew Davis added a comment - - edited Just recording some notes on how the valueoversized system could work to make sure I understand. We increase the length of user_preferences->value to 1333. check_user_preferences_loaded() currently uses $DB->get_records_menu() which can only handle 2 columns (name, value). It would switch to $DB->get_records() to get 3 columns (name, value, valueoversized). Then we would iterate over the returned array and compress it down to name and value by doing something like... $user->preference[$name] = !empty($value) ? $value : $valueoversized; Is there a way to do that that doesn't involve looping over every preference? set_user_preference() would need to check the length of the value using textlib::strlen(). If its above 1333 (define a new constant for this I guess) it goes into valueoversized and value is set to null. This section of code would need to be altered to deal with both value and valueoversized if ($preference = $DB->get_record('user_preferences', array('userid'=>$user->id, 'name'=>$name))) { if ($preference->value === $value and isset($user->preference[$name]) and $user->preference[$name] === $value) { // preference already set to this value return true; } $DB->set_field('user_preferences', 'value', $value, array('id'=>$preference->id));   } else { $preference = new stdClass(); $preference->userid = $user->id; $preference->name = $name; $preference->value = $value; $DB->insert_record('user_preferences', $preference); }
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            That is about the gist of it.
            Just noting the field size can only be increased for 2.2+ and master.
            I wouldn't use a define for it, I'd just hard code it and comment. Should we ever increase it again we'd need to review use and hopefully we'd find the comment.
            As for the iteration in check_user_preferences, I cannot think of any smart way to avoid that, it'd just have to be done.

            Perhaps worth waiting for Petr to have a read of these changes and get his thoughts on it. There may be something we have overlooked.

            Also worth knowing I just dreamed up the name for the new field, it wasn't discussed, feel free to come up with something better

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, That is about the gist of it. Just noting the field size can only be increased for 2.2+ and master. I wouldn't use a define for it, I'd just hard code it and comment. Should we ever increase it again we'd need to review use and hopefully we'd find the comment. As for the iteration in check_user_preferences, I cannot think of any smart way to avoid that, it'd just have to be done. Perhaps worth waiting for Petr to have a read of these changes and get his thoughts on it. There may be something we have overlooked. Also worth knowing I just dreamed up the name for the new field, it wasn't discussed, feel free to come up with something better Cheers Sam
            Hide
            Tim Hunt added a comment -

            The valueoversized thing is horrific. That can't possibly be a good idea.

            Just changing value to TEXT seems so obvious to me.

            What backwards-compability issues are there? They must be less than your complex value + valueoversized change.

            And user_pref values are cached in session. I don't believe that there would be performance problems, unless you can show me measurements. Also, if there are performance problems, then there are probably worse performance problems with config_plugins.


            I would be happy to just make value CHAR(1333), and then add code to set_user_preferences to throw a clear coding_exception explaining the problem if users try to set a value that is too long. That is simple, and deals with any potential performance issues.

            We should probably consider doing the same for config and config_plugins, we can can convince ourselves that is sufficient.


            Anyway, -1000000 from my for the valueoversized suggestion. It is totally ridiculous. It has maintenance headache written all over it.

            Show
            Tim Hunt added a comment - The valueoversized thing is horrific. That can't possibly be a good idea. Just changing value to TEXT seems so obvious to me. What backwards-compability issues are there? They must be less than your complex value + valueoversized change. And user_pref values are cached in session. I don't believe that there would be performance problems, unless you can show me measurements. Also, if there are performance problems, then there are probably worse performance problems with config_plugins. I would be happy to just make value CHAR(1333), and then add code to set_user_preferences to throw a clear coding_exception explaining the problem if users try to set a value that is too long. That is simple, and deals with any potential performance issues. We should probably consider doing the same for config and config_plugins, we can can convince ourselves that is sufficient. Anyway, -1000000 from my for the valueoversized suggestion. It is totally ridiculous. It has maintenance headache written all over it.
            Hide
            Tim Hunt added a comment -

            David pointed out that the alleged BC concerns were the ones Petr raised in this comment: http://tracker.moodle.org/browse/MDL-20245?focusedCommentId=133543&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-133543

            Searching the code for

            {user_preferences}

            , the only match I find is https://github.com/moodle/moodle/blob/master/lib/cronlib.php#L207

            That query would, indeed, need one trivial change if we changed the value column to TEXT.

            Grepping for user_preferences (without the {}) finds a some more occurrences, but I don't think there are any more causes for concern.

            Show
            Tim Hunt added a comment - David pointed out that the alleged BC concerns were the ones Petr raised in this comment: http://tracker.moodle.org/browse/MDL-20245?focusedCommentId=133543&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-133543 Searching the code for {user_preferences} , the only match I find is https://github.com/moodle/moodle/blob/master/lib/cronlib.php#L207 That query would, indeed, need one trivial change if we changed the value column to TEXT. Grepping for user_preferences (without the {}) finds a some more occurrences, but I don't think there are any more causes for concern.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            (would be so cool to avoid adjectives like "ridiculous" and avoid to ridiculously abs(vote) > 1)

            Regarding the valueoversized (textvalue) idea, it was entirely my idea (but the name, lol), not Sam's or Andrew's one, just to clarify it. And mainly trying to avoid big (current and future) slowdowns in queries handling the "value" column in any where/join clause.

            Perhaps, after all, the solution for this should be to keep that serialized information out from user_preferences, and only accept simple/small values there.

            ridiculously yours, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - (would be so cool to avoid adjectives like "ridiculous" and avoid to ridiculously abs(vote) > 1) Regarding the valueoversized (textvalue) idea, it was entirely my idea (but the name, lol), not Sam's or Andrew's one, just to clarify it. And mainly trying to avoid big (current and future) slowdowns in queries handling the "value" column in any where/join clause. Perhaps, after all, the solution for this should be to keep that serialized information out from user_preferences, and only accept simple/small values there. ridiculously yours, ciao
            Hide
            Tim Hunt added a comment -

            Sorry, yes. Inappropriate language. And, since user_prefs are cached in session, probably a good idea to limit field size. I think CHAR(1333) + coding exception is the way to go.

            Show
            Tim Hunt added a comment - Sorry, yes. Inappropriate language. And, since user_prefs are cached in session, probably a good idea to limit field size. I think CHAR(1333) + coding exception is the way to go.
            Hide
            Andrew Davis added a comment - - edited

            I have implemented the value column max length increase and added a throw new coding_exception() to set_user_preference(). Diff url and branch updated. So is this the solution we're going with?

            Show
            Andrew Davis added a comment - - edited I have implemented the value column max length increase and added a throw new coding_exception() to set_user_preference(). Diff url and branch updated. So is this the solution we're going with?
            Hide
            Tim Hunt added a comment -

            Are we sure that textlib::strlen($value) > 1333 and 'value too big to fit in CHAR(1333)' mean exactly the same thing in all DBs? (due to the complexities of UTF-8)

            ... if they are not the same, does it really matter?

            Also, are we going to make any attempt to make the grader_report user preferences value more compact - rather than the current, slightly verbose, serialise PHP array format?

            Show
            Tim Hunt added a comment - Are we sure that textlib::strlen($value) > 1333 and 'value too big to fit in CHAR(1333)' mean exactly the same thing in all DBs? (due to the complexities of UTF-8) ... if they are not the same, does it really matter? Also, are we going to make any attempt to make the grader_report user preferences value more compact - rather than the current, slightly verbose, serialise PHP array format?
            Hide
            Andrew Davis added a comment -

            Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account.

            I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.

            Show
            Andrew Davis added a comment - Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account. I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.
            Hide
            Andrew Davis added a comment -

            Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account.

            I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.

            Show
            Andrew Davis added a comment - Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account. I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.
            Hide
            Tim Hunt added a comment -

            That sounds like a good plan. Thanks.

            Show
            Tim Hunt added a comment - That sounds like a good plan. Thanks.
            Hide
            Andrew Davis added a comment -

            MySql pre 4.1 (released 2004) apparently treats VARCHAR(N) as a number of bytes. All subsequent versions treat it as a number of characters and apparently it copes with UTF-8.

            Postgres has apparently treated VARCHAR(N) as number of characters and dealt with utf-8 since forever.

            In Oracle you can do either VARCHAR2(N BYTE) or VARCHAR2(N CHAR). Provided we're using CHAR all is well. If not Oracle users will potentially run into problems with a long utf-8 string passing the PHP check but being too long for the column.

            SQL Server is complicated and I didn't find a simple answer.

            All of the above goes out the window if the database simply isn't configured to use utf-8. You would always get into trouble if you did something like have PHP supply a UTF-8 string to a database configured to be ASCII. A long utf-8 string could pass the check in PHP, be interpreted as single byte characters by the DB and become too long to go into the value column. Not sure if we can do much about that.

            Show
            Andrew Davis added a comment - MySql pre 4.1 (released 2004) apparently treats VARCHAR(N) as a number of bytes. All subsequent versions treat it as a number of characters and apparently it copes with UTF-8. Postgres has apparently treated VARCHAR(N) as number of characters and dealt with utf-8 since forever. In Oracle you can do either VARCHAR2(N BYTE) or VARCHAR2(N CHAR). Provided we're using CHAR all is well. If not Oracle users will potentially run into problems with a long utf-8 string passing the PHP check but being too long for the column. SQL Server is complicated and I didn't find a simple answer. All of the above goes out the window if the database simply isn't configured to use utf-8. You would always get into trouble if you did something like have PHP supply a UTF-8 string to a database configured to be ASCII. A long utf-8 string could pass the check in PHP, be interpreted as single byte characters by the DB and become too long to go into the value column. Not sure if we can do much about that.
            Hide
            Andrew Davis added a comment -

            This involves a database change so cannot be included in the 2.2 and 2.1 branches. I considered back porting just the addition of the safety check in PHP but then anyone wanting to increase the size of the value column themselves would have both increase the column size and modify the code to match their increased column size.

            Show
            Andrew Davis added a comment - This involves a database change so cannot be included in the 2.2 and 2.1 branches. I considered back porting just the addition of the safety check in PHP but then anyone wanting to increase the size of the value column themselves would have both increase the column size and modify the code to match their increased column size.
            Hide
            Tim Hunt added a comment -

            This cannot be done in 2.1, because varchar(1333) support was only added in 2.2

            "And thirdly, the code is more what you'd call 'guidelines' than actual rules." - it is safe to make exactly the same DB change on the stable branch and master, especially when it is an idempotent change like increasing the column width to 1333, so I think we should bend our own rules here and fix this in 2.2 .

            Show
            Tim Hunt added a comment - This cannot be done in 2.1, because varchar(1333) support was only added in 2.2 "And thirdly, the code is more what you'd call 'guidelines' than actual rules." - it is safe to make exactly the same DB change on the stable branch and master, especially when it is an idempotent change like increasing the column width to 1333, so I think we should bend our own rules here and fix this in 2.2 .
            Hide
            Andrew Davis added a comment -

            I've produced a 2.2 stable version of the fix. Ultimately it's up to the integrators whether they want to integrate the 2.2 version as well as the version for master.

            Show
            Andrew Davis added a comment - I've produced a 2.2 stable version of the fix. Ultimately it's up to the integrators whether they want to integrate the 2.2 version as well as the version for master.
            Hide
            Andrew Davis added a comment -

            I'm submitting this for integration. Although it doesn't officially have a peer reviewer its certainly had plenty of eyes on it.

            Show
            Andrew Davis added a comment - I'm submitting this for integration. Although it doesn't officially have a peer reviewer its certainly had plenty of eyes on it.
            Hide
            Tim Hunt added a comment -

            Thanks Andrew.

            Show
            Tim Hunt added a comment - Thanks Andrew.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Oki so this is really one "workaround / alleviator" that, exclusively under 22_STABLE and master will allow bigger (1333) values there, leading to "more cases" working, but not all (I mean, if there are, say 200 categories, it will continue breaking, correct? Because 1333 will be exceeded too.

            So, the "real" fix will arrive once MDL-30668 is done (changing storage format and/or place). And that will be applied to all supported stable branches. Does that mean that we should "tidy" a bit the subject of this issue to something like "Allow bigger values in user preferences, alleviating some problems with grader preferences" or so.

            Finally, note the we are still in the initial "on-sync" weeks between 22_STABLE and master so we cannot apply the patches on their current form (they cause versions to diverge). To solve this, there are 2 possibilities, you pick the preferred one:

            1) Amend your patches so both are using the same 2011120500.02 + .01 (right now) version.
            2) Send this back to the "awaiting integration" status, so it gets fixed next week, when we are already allowing to diverge versions. Note that this *requires amending too" because the current versions will be outdated.

            I'll keep this halted until hearing from you...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Oki so this is really one "workaround / alleviator" that, exclusively under 22_STABLE and master will allow bigger (1333) values there, leading to "more cases" working, but not all (I mean, if there are, say 200 categories, it will continue breaking, correct? Because 1333 will be exceeded too. So, the "real" fix will arrive once MDL-30668 is done (changing storage format and/or place). And that will be applied to all supported stable branches. Does that mean that we should "tidy" a bit the subject of this issue to something like "Allow bigger values in user preferences, alleviating some problems with grader preferences" or so. Finally, note the we are still in the initial "on-sync" weeks between 22_STABLE and master so we cannot apply the patches on their current form (they cause versions to diverge). To solve this, there are 2 possibilities, you pick the preferred one: 1) Amend your patches so both are using the same 2011120500.02 + .01 (right now) version. 2) Send this back to the "awaiting integration" status, so it gets fixed next week, when we are already allowing to diverge versions. Note that this *requires amending too" because the current versions will be outdated. I'll keep this halted until hearing from you...ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Oki, I'm going to:

            • add extra commit putting versions on-sync (2011120500.03)
            • modify the title of the issue an point everybody to the "real" fix: MDL-30668
            • push it to integration, so testing can happen.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Oki, I'm going to: add extra commit putting versions on-sync (2011120500.03) modify the title of the issue an point everybody to the "real" fix: MDL-30668 push it to integration, so testing can happen. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Two new commits have been added about:

            • keep 22_STABLE and master versions on-sync (2011120500.03)
            • added 2 assertions to test_set_user_preference() so both the 1333 and 1334 cases are covered. They have been tested agains the 4 DBs
            Show
            Eloy Lafuente (stronk7) added a comment - Two new commits have been added about: keep 22_STABLE and master versions on-sync (2011120500.03) added 2 assertions to test_set_user_preference() so both the 1333 and 1334 cases are covered. They have been tested agains the 4 DBs
            Hide
            Sam Hemelryk added a comment -

            Tested and passed thanks all.

            Show
            Sam Hemelryk added a comment - Tested and passed thanks all.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

            Now... disconnect, relax and enjoy the next days, yay!

            Closing...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

              People

              • Votes:
                34 Vote for this issue
                Watchers:
                27 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: