Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-24235

Fatal error: Call to undefined function mb_strlen() in /moodle/web/lib/bennu/iCalendar_rfc2445.php on line 57

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.9, 1.9.17, 2.3
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      Test that the calendar still works as expected:

      1. Create an assignment with start/finish dates and make sure events are created
      2. Create a couple of different event types as an admin (user, site, etc)
      3. Export the calendar and make sure that works
      Show
      Test that the calendar still works as expected: Create an assignment with start/finish dates and make sure events are created Create a couple of different event types as an admin (user, site, etc) Export the calendar and make sure that works
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-24235-master

      Description

      We happen to have a moodle server running without the mb_string extension installed, and the calendar feature uses the mb_strlen() function without checking that it actually exists.

      If the requirement for mb_string were a "MUST" have this would be ok, but it is listed as "SHOULD" be installed.

        Gliffy Diagrams

        1. mdl24235_show_warning_fixed.patch
          3 kB
          Charles Fulton
        2. mdl24235_show_warning.patch
          3 kB
          Charles Fulton

          Activity

          Hide
          carolinemoore Caroline Moore added a comment -

          Is there a fix/workaround for this bug? One of my instructors just ran into it, and I'd like to get her up and running.

          Show
          carolinemoore Caroline Moore added a comment - Is there a fix/workaround for this bug? One of my instructors just ran into it, and I'd like to get her up and running.
          Hide
          cfulton Charles Fulton added a comment -

          The best solution is enabling the mbstring extension on the webserver which is pretty low-impact. I think it's possible to replace mb_strlen() with a regex but (a) that's totally a hack, (b) it'll cause an awful performance hit and (c) it means modifying an external library.

          Show
          cfulton Charles Fulton added a comment - The best solution is enabling the mbstring extension on the webserver which is pretty low-impact. I think it's possible to replace mb_strlen() with a regex but (a) that's totally a hack, (b) it'll cause an awful performance hit and (c) it means modifying an external library.
          Hide
          cfulton Charles Fulton added a comment -

          This is also broken in 2.0 in that the bennu library is still included and mbstring is still considered SHOULD instead of MUST. The right way to fix this is updating the 1.9 and 2.0 branches to either require mbstring OR make it clear that calendar exporting will suffer a fatal error.

          Show
          cfulton Charles Fulton added a comment - This is also broken in 2.0 in that the bennu library is still included and mbstring is still considered SHOULD instead of MUST. The right way to fix this is updating the 1.9 and 2.0 branches to either require mbstring OR make it clear that calendar exporting will suffer a fatal error.
          Hide
          cfulton Charles Fulton added a comment -

          I've written a patch which displays a notification instead of the export buttons in view.php if the following conditions are met: (1) calendar export is enabled at the site level and (2) the mbstring extension isn't installed. The notification explains that without mbstring calendar export is unavailable.

          Show
          cfulton Charles Fulton added a comment - I've written a patch which displays a notification instead of the export buttons in view.php if the following conditions are met: (1) calendar export is enabled at the site level and (2) the mbstring extension isn't installed. The notification explains that without mbstring calendar export is unavailable.
          Hide
          cfulton Charles Fulton added a comment -

          Re-uploading without the typo.

          Show
          cfulton Charles Fulton added a comment - Re-uploading without the typo.
          Hide
          jonathan Jonathan Harker added a comment -

          Charles' patch added to git repo for review/pull
          Cheers

          Show
          jonathan Jonathan Harker added a comment - Charles' patch added to git repo for review/pull Cheers
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi guys,

          I've just been looking at this and discussing it with Eloy.
          After a bit of deliberation we've come to agree that this should be fixed by changing bennu to use our textlib rather than mbstring directly.
          Of course this is a Moodle specific change and thus not suitable for upstream but we still think this is the way to go, obviously it would need to be noted in the readme.txt afterwards.
          Does that sound OK to you two?

          Sending it back this week sorry so that we can look into that.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi guys, I've just been looking at this and discussing it with Eloy. After a bit of deliberation we've come to agree that this should be fixed by changing bennu to use our textlib rather than mbstring directly. Of course this is a Moodle specific change and thus not suitable for upstream but we still think this is the way to go, obviously it would need to be noted in the readme.txt afterwards. Does that sound OK to you two? Sending it back this week sorry so that we can look into that. Cheers Sam
          Hide
          cfulton Charles Fulton added a comment -

          Hi Sam,

          If core's okay with modifying bennu then I certainly don't have a problem with it (and I think it was done once before with the ereg functions). It looks pretty simple for both 1.9 and 2.2; switching out about four function calls in the main class.

          Charles

          Show
          cfulton Charles Fulton added a comment - Hi Sam, If core's okay with modifying bennu then I certainly don't have a problem with it (and I think it was done once before with the ereg functions). It looks pretty simple for both 1.9 and 2.2; switching out about four function calls in the main class. Charles
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Charles,

          I've just been going through my Email today sorry for the slow reply.
          Modifying bennu in core is perfectly acceptable for this situation
          Are you happy to make those changes or would you like me to, no probs either way.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Charles, I've just been going through my Email today sorry for the slow reply. Modifying bennu in core is perfectly acceptable for this situation Are you happy to make those changes or would you like me to, no probs either way. Cheers Sam
          Show
          cfulton Charles Fulton added a comment - Patch for 1.9: https://github.com/mackensen/moodle/compare/MOODLE_19_STABLE...MDL-24235-19 . Patch for 2.2: https://github.com/mackensen/moodle/compare/master...MDL-24235-22 .
          Hide
          cfulton Charles Fulton added a comment -

          Rebased 2.2, dropped 1.9 and added 2.1 and master.

          Show
          cfulton Charles Fulton added a comment - Rebased 2.2, dropped 1.9 and added 2.1 and master.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Charles looks 100% spot on, putting it up for integration now (so that it gets into todays integration)

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Charles looks 100% spot on, putting it up for integration now (so that it gets into todays integration) Cheers Sam
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Charles this has been integrated now.

          Just noting I did make an additional commit to fix the 2.1 branch.
          In 2.2+ textlib was changed to use static methods. In 2.1 it still uses an instantiated class and public methods.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Charles this has been integrated now. Just noting I did make an additional commit to fix the 2.1 branch. In 2.2+ textlib was changed to use static methods. In 2.1 it still uses an instantiated class and public methods. Cheers Sam
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi,
          Working as expected.
          Passing

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi, Working as expected. Passing
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

          icao_reverse('arreis olik rebemevon afla letoh ognat');

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/Mar/12