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
    • Rank:
      1181

      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?

        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 Škoda added a comment -

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

          Show
          Petr Škoda 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 Škoda added a comment -

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

          Show
          Petr Škoda 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 Škoda 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 Škoda 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: