Moodle
  1. Moodle
  2. MDL-34539

Condition on $section in course/view.php assumes positive numbers and prevents format plugins from using negatives for designated displays

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Regression testing only:

      1. Add a new course
      2. Set format to topics
      3. Set 'course display' to one section per page
      4. Visit a single section
      5. VERIFY: that a log entry has been added for viewing the single section
      Show
      Regression testing only: Add a new course Set format to topics Set 'course display' to one section per page Visit a single section VERIFY: that a log entry has been added for viewing the single section
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Rank:
      42967

      Description

      Around line 101 in course/view.php

      if (!empty($section)) {
      

      assumes positive section numbers (that is, $section > 0) or else throws an exception at

      $modinfo->get_section_info($section, MUST_EXIST);
      

      However, format plugins may use negative section numbers to control the display. The whole condition seems to merely serve logging and so seems unnecessarily too strong.

      The condition could be replaced with

      if ($section and $section > 0) {
      

      to get the same effect and still allow format plugins more flexibility.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for spotting that and providing a solution.

        Show
        Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
        Hide
        Dan Poltawski added a comment -

        Hmm, I think I agree - we are unnecessarily putting a limitation on course formats with this.

        Show
        Dan Poltawski added a comment - Hmm, I think I agree - we are unnecessarily putting a limitation on course formats with this.
        Hide
        Dan Poltawski added a comment -

        The course format could use -1 to indicate something like clear selection, and then choose to display a different section.

        Show
        Dan Poltawski added a comment - The course format could use -1 to indicate something like clear selection, and then choose to display a different section.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks!
        Hide
        Frédéric Massart added a comment - - edited

        Test successful on master and 2.3.

        Regarding those logs, that'd be nice to have some information in the 'Information' column such as the course name as the other logs have (course report log, course view).

        Show
        Frédéric Massart added a comment - - edited Test successful on master and 2.3. Regarding those logs, that'd be nice to have some information in the 'Information' column such as the course name as the other logs have (course report log, course view).
        Hide
        Eloy Lafuente (stronk7) added a comment -

        For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

        Many thanks for your collaboration, yay!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: