Moodle
  1. Moodle
  2. MDL-36881

mod_chooser dialog contains blank radio button options that cause an error when selected when activities improperly define _get_types() lib function

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide
      • Open a course page, turn editing on
        • Confirm that the Activity chooser appears correctly - all modules appear
      • Switch of the Activity chooser
        • ensure that the dropdowns also appear correctly - all modules appear
      • Modify mod/choice/lib.php and add:
        function choice_get_types() {
            return array();
        }
        
      • Refresh the page
        • Confirm both dropdowns and the Activity chooser appear correctly but are now missing the 'Choice' module
        • Confirm that a debugging message telling you that the module definition is incorrect was shown
      • Edit mod/choice/lib.php again and update the function:
        function choice_get_types() {
            return null;
        }
        
      • Refresh the page
        • Confirm both dropdowns and the Activity chooser appear correctly but are now missing the 'Choice' module
        • Confirm that a debugging message telling you that the module definition is incorrect was shown
      Show
      Open a course page, turn editing on Confirm that the Activity chooser appears correctly - all modules appear Switch of the Activity chooser ensure that the dropdowns also appear correctly - all modules appear Modify mod/choice/lib.php and add: function choice_get_types() { return array(); } Refresh the page Confirm both dropdowns and the Activity chooser appear correctly but are now missing the 'Choice' module Confirm that a debugging message telling you that the module definition is incorrect was shown Edit mod/choice/lib.php again and update the function: function choice_get_types() { return null ; } Refresh the page Confirm both dropdowns and the Activity chooser appear correctly but are now missing the 'Choice' module Confirm that a debugging message telling you that the module definition is incorrect was shown
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-36881-m24
    • Pull Master Branch:
    • Rank:
      46414

      Description

      If an activity defines a

      {modname}_get_types() lib function when the the activity does not have sub-types, the mod_chooser dialog will contain a blank radio button option that causes an error when selected and the "Add" button is clicked. The browser is directed to /course/jumpto.php without any parameters and the error "Error occurred" is displayed.

      The problem is with activity modules that define a {modname}

      _get_types() lib function and then return something other than an non-empty array. We have found that some 3rd party plugins define the function and return an empty array which triggers this bug. Ideally, plugins would not define this function, but I think that the calling function should better test the return value of the function. The calling function is in course/lib.php: get_module_metadata().

      I've attached a proposed fix that checks that what is returned from the function is a non-empty array. From what we've experienced this prevents the bug.

      1. MDL-36881_no_whitespace_difference.diff
        1 kB
        Sam Chaffee
      2. MDL-36881.diff
        4 kB
        Sam Chaffee

        Activity

        Hide
        Sam Chaffee added a comment -

        Adding a diff for a proposed fix as well as the same fix diff without whitespace differences so that the actual change can be more clearly seen.

        Show
        Sam Chaffee added a comment - Adding a diff for a proposed fix as well as the same fix diff without whitespace differences so that the actual change can be more clearly seen.
        Hide
        Dan Poltawski added a comment -

        Sam, thanks for the report.

        Andrew, I think this is one for you, but feel free to assign it back to me if you can't deal with it.

        Show
        Dan Poltawski added a comment - Sam, thanks for the report. Andrew, I think this is one for you, but feel free to assign it back to me if you can't deal with it.
        Hide
        Andrew Nicols added a comment -

        Sam,

        Thanks for the patch - do you happen to have an example of some of the incorrect output that such a plugin may return from modname_get_types() ?

        If a plugin does return something invalid from modname_get_types, then I don't think we should add it to the module chooser (or drop-down list) at all. It could be that it is a plugin with subtypes but all of those subtypes have been disabled – in which case adding it to the module chooser as a 'normal' plugin would mean that yes, you can select it, but chances are that the modedit will not work correctly.

        I think that we need to keep this as two separate if tests, and to add a further test before we add the $modlist[$course->id][$modname] to the $return array and on failure display a debugging message to inform a developer that they're doing it wrong.

        It'd be good to have some examples of the different ways in which modname_get_types may cause this breakage?

        Andrew

        Show
        Andrew Nicols added a comment - Sam, Thanks for the patch - do you happen to have an example of some of the incorrect output that such a plugin may return from modname_get_types() ? If a plugin does return something invalid from modname_get_types, then I don't think we should add it to the module chooser (or drop-down list) at all. It could be that it is a plugin with subtypes but all of those subtypes have been disabled – in which case adding it to the module chooser as a 'normal' plugin would mean that yes, you can select it, but chances are that the modedit will not work correctly. I think that we need to keep this as two separate if tests, and to add a further test before we add the $modlist [$course->id] [$modname] to the $return array and on failure display a debugging message to inform a developer that they're doing it wrong. It'd be good to have some examples of the different ways in which modname_get_types may cause this breakage? Andrew
        Hide
        Sam Chaffee added a comment -

        Andrew,

        I agree with you that a separate test may be needed to ensure that the group of sub-types is defined properly for activities that do have subtypes.

        As far as different examples go, the specific problem we were experiencing was with activities defining (when they shouldn't have defined the function in the first place) modname_get_types() something like this:

        function modname_get_types() {
            return array();
        }
        

        I just picked an activity that defined the function like this and then changed the return value to other non-empty array values like false, strings, etc to make sure the patch I attached worked. Like you've mentioned though, it doesn't account for cases in which a non-empty array is returned, but still is not correct.

        I can try to dig up a plugin that does this or you can just pick an activity that doesn't define that function currently, define the function, and play with the return values.

        Show
        Sam Chaffee added a comment - Andrew, I agree with you that a separate test may be needed to ensure that the group of sub-types is defined properly for activities that do have subtypes. As far as different examples go, the specific problem we were experiencing was with activities defining (when they shouldn't have defined the function in the first place) modname_get_types() something like this: function modname_get_types() { return array(); } I just picked an activity that defined the function like this and then changed the return value to other non-empty array values like false, strings, etc to make sure the patch I attached worked. Like you've mentioned though, it doesn't account for cases in which a non-empty array is returned, but still is not correct. I can try to dig up a plugin that does this or you can just pick an activity that doesn't define that function currently, define the function, and play with the return values.
        Hide
        Andrew Nicols added a comment -

        Thanks Sam,

        I'll have a play and modify some of the core plugins to see how well they handle things.

        Andrew

        Show
        Andrew Nicols added a comment - Thanks Sam, I'll have a play and modify some of the core plugins to see how well they handle things. Andrew
        Hide
        Rajesh Taneja added a comment -

        Thanks Andrew,

        Patch looks good, but I personally would take type assignment ($types = $gettypesfunc()) out from if.

        Feel free to push it for integration, if you feel otherwise.

        Show
        Rajesh Taneja added a comment - Thanks Andrew, Patch looks good, but I personally would take type assignment ($types = $gettypesfunc()) out from if. Feel free to push it for integration, if you feel otherwise.
        Hide
        Andrew Nicols added a comment -

        Apologies for letting this slide. I've rebased and produced all appropriate branches.

        Thanks for the suggestion, I agree and have adjusted the patch accordingly.

        Show
        Andrew Nicols added a comment - Apologies for letting this slide. I've rebased and produced all appropriate branches. Thanks for the suggestion, I agree and have adjusted the patch accordingly.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
        Hide
        Michael de Raadt added a comment -

        Test results: Success!

        Tested in 2.3, 2.4 and Master.

        Show
        Michael de Raadt added a comment - Test results: Success! Tested in 2.3, 2.4 and Master.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

        (and won't be revisiting it unless some regression is found)

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: