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 Bug
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      6309

      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.

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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Charles Fulton added a comment -

        Re-uploading without the typo.

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

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

        Show
        Jonathan Harker added a comment - Charles' patch added to git repo for review/pull Cheers
        Hide
        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
        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
        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
        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
        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
        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
        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
        Charles Fulton added a comment -

        Rebased 2.2, dropped 1.9 and added 2.1 and master.

        Show
        Charles Fulton added a comment - Rebased 2.2, dropped 1.9 and added 2.1 and master.
        Hide
        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
        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
        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
        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 Agarwal added a comment -

        Hi,
        Working as expected.
        Passing

        Show
        Ankit Agarwal added a comment - Hi, Working as expected. Passing
        Hide
        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
        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: