Moodle
  1. Moodle
  2. MDL-38344

Manage subscription throw error, if length of event name is more than 255

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      Test 1

      1. Prerequisite: Moodle site without this patch
      2. Navigate to calendar (Course > Upcoming events block > Go to calendar...")
      3. Add course event, user event and site event.
      4. Apply this patch and upgrade site
      5. Check events which were created above and make sure there is no change.
      6. Check database and make sure name field in mdl_event table is of type text.

      Test 2

      1. Log in as any user
      2. Navigate to the calendar (Course > Upcoming events block > Go to calendar..."
      3. Click Manage subscriptions
      4. Add a calendar name and the URL (https://www.google.com/calendar/ical/gsummerofcode%40gmail.com/public/basic.ics)
      5. Make sure you don't see any error.

      Test 3

      1. Install new moodle site with this patch
      2. Check database and make sure name field in mdl_event table is of type text.
      Show
      Test 1 Prerequisite: Moodle site without this patch Navigate to calendar (Course > Upcoming events block > Go to calendar...") Add course event, user event and site event. Apply this patch and upgrade site Check events which were created above and make sure there is no change. Check database and make sure name field in mdl_event table is of type text. Test 2 Log in as any user Navigate to the calendar (Course > Upcoming events block > Go to calendar..." Click Manage subscriptions Add a calendar name and the URL ( https://www.google.com/calendar/ical/gsummerofcode%40gmail.com/public/basic.ics ) Make sure you don't see any error. Test 3 Install new moodle site with this patch Check database and make sure name field in mdl_event table is of type text.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull 2.4 Branch:
      wip-mdl-38344-m24
    • Pull Master Branch:
      wip-mdl-38344
    • Rank:
      48230

      Description

      I was testing the calendar subscription system using the following URL.

      https://www.google.com/calendar/ical/gsummerofcode%40gmail.com/public/basic.ics

      When the subscription form was submitted the following error was shown.

      Error writing to database
      
      More information about this error
      
      Debug info: 
      Error code: dmlwriteexception
      Stack trace:
      line 476 of \lib\setuplib.php: moodle_exception thrown
      line 83 of \calendar\managesubscriptions.php: call to print_error()
      

      Replication steps:

      1. Log in as any user
      2. Navigate to the calendar (Course > Upcoming events block > Go to calendar..."
      3. Click Manage subscriptions
      4. Add a calendar name and the URL above

      It may be that the @ in the URL is being filtered at some point, which is preventing the fetching of calendar events. It looks like the subscription is being added but no events are being added for it.

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          It's happening because name is too big in one of the events

          stdClass object {
            name => (string) [GSoC 2013] All mentors must be signed up and all student proposals matched with a mentor - 07:00 UTC Student acceptance choice deadline. IRC meeting to resolve any outstanding duplicate accepted students - 19:00 UTC #gsoc (organizations must send a delegate to represent them in this meeting regardless of if they are in a duplicate situation before the meeting.)
            description => (string)
            timestart => (int) 1369404000
            timeduration => (int) 1800
            uuid => (string) ianlk0qlrolpa26namaa9fkgh4@google.com
            timemodified => (int) 1362555672
            subscriptionid => (int) 4
            userid => (string) 2
            groupid => (string) 0
            courseid => (string) 0
            eventtype => (string) user
          }
          

          This can be checked manually creating an event and pasting object->name

          Show
          Rajesh Taneja added a comment - It's happening because name is too big in one of the events stdClass object { name => (string) [GSoC 2013] All mentors must be signed up and all student proposals matched with a mentor - 07:00 UTC Student acceptance choice deadline. IRC meeting to resolve any outstanding duplicate accepted students - 19:00 UTC #gsoc (organizations must send a delegate to represent them in this meeting regardless of if they are in a duplicate situation before the meeting.) description => (string) timestart => (int) 1369404000 timeduration => (int) 1800 uuid => (string) ianlk0qlrolpa26namaa9fkgh4@google.com timemodified => (int) 1362555672 subscriptionid => (int) 4 userid => (string) 2 groupid => (string) 0 courseid => (string) 0 eventtype => (string) user } This can be checked manually creating an event and pasting object->name
          Hide
          Rajesh Taneja added a comment -

          Either we can truncate event->name to 255 or make it large to avoid such problems.

          Show
          Rajesh Taneja added a comment - Either we can truncate event->name to 255 or make it large to avoid such problems.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          Given char fields support 1333 characters we could consider increasing the size event.name from 255 to 1333 in master.

          In looking up the iCalandar RFC I found the following section http://tools.ietf.org/html/rfc5545#section-5.
          Point 4 under recommended practices when dealing with iCalendar objects states -

          An implementation can truncate a "SUMMARY" property value to 255
          octets, but it MUST NOT truncate the value in the middle of a
          UTF-8 multi-octet sequence.

          From what I read the summary field is noted as being a text type and I could find no restriction on the length of a text field. Perhaps best to seek other advice or research there to be sure.

          Really truncating isn't that nice, but as it is a recommended practice for the spec I think the plan should be:

          1. Increase the field size from 255 to 1333 in master and then handle anything over that by truncating it (to the word).
          2. In stable branches truncate the summary to 255 (to the word).

          Jonathan Harker, I've added you as a watcher here in case you have any thoughts or preferences as to how this should be handled.

          Many thanks.
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, Given char fields support 1333 characters we could consider increasing the size event.name from 255 to 1333 in master. In looking up the iCalandar RFC I found the following section http://tools.ietf.org/html/rfc5545#section-5 . Point 4 under recommended practices when dealing with iCalendar objects states - An implementation can truncate a "SUMMARY" property value to 255 octets, but it MUST NOT truncate the value in the middle of a UTF-8 multi-octet sequence. From what I read the summary field is noted as being a text type and I could find no restriction on the length of a text field. Perhaps best to seek other advice or research there to be sure. Really truncating isn't that nice, but as it is a recommended practice for the spec I think the plan should be: Increase the field size from 255 to 1333 in master and then handle anything over that by truncating it (to the word). In stable branches truncate the summary to 255 (to the word). Jonathan Harker, I've added you as a watcher here in case you have any thoughts or preferences as to how this should be handled. Many thanks. Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          It make sense to increase size in master, but should we put another limit (1333) on it?
          As you mentioned there is no restriction on the length, putting another limit might break this again.

          Will wait for Jonathan's opinion on this as well.

          Show
          Rajesh Taneja added a comment - Thanks Sam, It make sense to increase size in master, but should we put another limit (1333) on it? As you mentioned there is no restriction on the length, putting another limit might break this again. Will wait for Jonathan's opinion on this as well.
          Hide
          Jonathan Harker added a comment -

          Is there a really good reason we can't just use a text field instead of continuing to use a varchar with an arbitrary length restriction on it?

          Show
          Jonathan Harker added a comment - Is there a really good reason we can't just use a text field instead of continuing to use a varchar with an arbitrary length restriction on it?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          If there is any (current or future) plan about doing it searchable/ordereable... then I'd move to 1333 & truncate at that length. Else text is ok.

          Show
          Eloy Lafuente (stronk7) added a comment - If there is any (current or future) plan about doing it searchable/ordereable... then I'd move to 1333 & truncate at that length. Else text is ok.
          Hide
          Rajesh Taneja added a comment -

          Hello Jonathan,

          I had a word with Sam and Eloy on this and we decided that it should be only size change.
          Reason: If anyone ever plan to provide event search feature/plugin then text will defeat the purpose. Also, 1333 should take care of 99.99% cases, with 1333 we can always use truncate to keep it within limits.

          In addition to this, Eloy is happy to have db change on stable branches, so will backport this to 2.4 as well (manage subscription was introduced in 2.4)

          Show
          Rajesh Taneja added a comment - Hello Jonathan, I had a word with Sam and Eloy on this and we decided that it should be only size change. Reason: If anyone ever plan to provide event search feature/plugin then text will defeat the purpose. Also, 1333 should take care of 99.99% cases, with 1333 we can always use truncate to keep it within limits. In addition to this, Eloy is happy to have db change on stable branches, so will backport this to 2.4 as well (manage subscription was introduced in 2.4)
          Hide
          Jonathan Harker added a comment - - edited

          Sorry but those reasons smell like MySQL limitations rather than anything inherent about database design. Searching and sorting varchar(1333) fields won't be any more efficient than searching and ordering text fields. Generally:

          • anything over varchar(100) should be a text field anyway,
          • length limits should only apply if they are expected by some standard,
          • most modern databases store char, varchar and text the same way anyway, and are slowed down by length-checking and padding whitespace for char( n), and
          • if there's no domain reason to limit the length (the RFC does not specify one), then the database field should be text.
          Show
          Jonathan Harker added a comment - - edited Sorry but those reasons smell like MySQL limitations rather than anything inherent about database design. Searching and sorting varchar(1333) fields won't be any more efficient than searching and ordering text fields. Generally: anything over varchar(100) should be a text field anyway, length limits should only apply if they are expected by some standard, most modern databases store char, varchar and text the same way anyway, and are slowed down by length-checking and padding whitespace for char( n), and if there's no domain reason to limit the length (the RFC does not specify one), then the database field should be text.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jonathan,

          I have converted name field to text. Also, IMHO it should be fine to just backport this to 2.4 only, as subscriptions were introduced in 2.4 and we don't expect user to enter long event name in 2.3 (Assumption made on tracker history.)

          Show
          Rajesh Taneja added a comment - Thanks Jonathan, I have converted name field to text. Also, IMHO it should be fine to just backport this to 2.4 only, as subscriptions were introduced in 2.4 and we don't expect user to enter long event name in 2.3 (Assumption made on tracker history.)
          Hide
          Jason Fowler added a comment - - edited

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [Y] Databases
          [Y] Testing
          [Y] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Jason Fowler added a comment - - edited [Y] Syntax [-] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Dan Poltawski added a comment -

          Hi Raj,

          Please can you add upgrade instructions testing the upgrade (i.e. making sure the data isn't corrupted by this change and that the field is changed properly)

          I've integrated this to master ONLY. I have not backported it because in normal circumstances we do not accept database schema changes on the stable branches. It can cause all sorts of problems as well as lots of work for moodle partners, downstream distributions etc.

          Please can you file a backport request for getting this into the stable branches. From a brief discussion between integrators we think that a truncate might be preferable here for 2.4.

          Show
          Dan Poltawski added a comment - Hi Raj, Please can you add upgrade instructions testing the upgrade (i.e. making sure the data isn't corrupted by this change and that the field is changed properly) I've integrated this to master ONLY. I have not backported it because in normal circumstances we do not accept database schema changes on the stable branches. It can cause all sorts of problems as well as lots of work for moodle partners, downstream distributions etc. Please can you file a backport request for getting this into the stable branches. From a brief discussion between integrators we think that a truncate might be preferable here for 2.4.
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan,

          I have updated testing instructions.

          Truncate option was considered on stable, but after talking to Eloy it was discussed to have this on 2.4 as well.
          Will create backport request for same and if you feel it should be truncate then will do the same.

          Show
          Rajesh Taneja added a comment - Thanks Dan, I have updated testing instructions. Truncate option was considered on stable, but after talking to Eloy it was discussed to have this on 2.4 as well. Will create backport request for same and if you feel it should be truncate then will do the same.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          @Rajesh Taneja, apologies perhaps I forgot it or was not clear about the 2.4 backport thingy. There will be some discussion in the backport issue about that.

          @Jonathan Harker, I only can disagree with (some of) your affirmations. TODAY, the differences between char and text storage/retrieval/indexing/sorting are really important. In fact we cannot index TEXT columns, that says it all. So I just hope we won't be using that field ever for sorting / searching. About the "everything > 100 chars should be text, well, again I disagree strongly. Specially, note we are supposed to be cross-db, so always must get the max common denominator for everything DB related.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited @ Rajesh Taneja , apologies perhaps I forgot it or was not clear about the 2.4 backport thingy. There will be some discussion in the backport issue about that. @ Jonathan Harker , I only can disagree with (some of) your affirmations. TODAY, the differences between char and text storage/retrieval/indexing/sorting are really important. In fact we cannot index TEXT columns, that says it all. So I just hope we won't be using that field ever for sorting / searching. About the "everything > 100 chars should be text, well, again I disagree strongly. Specially, note we are supposed to be cross-db, so always must get the max common denominator for everything DB related. Ciao
          Hide
          David Monllaó added a comment -

          It passes, tested only in master

          Show
          David Monllaó added a comment - It passes, tested only in master
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: