Moodle
  1. Moodle
  2. MDL-27824

Add/ Editing Categories / Courses / Sub courses and it crashes with a coding error detected. "paging_bar requires a perpage value"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.2.1, 2.2.5, 2.3.2, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Administration
    • Labels:
    • Environment:
      Windows Server 2008
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. access your DB and select the config table, rename 'coursesperpage' to 'coursesperpagexyz'
      2. as admin, access site admin > courses > add/edit courses
      3. select one of the categories
      4. make sure there's no error occurs
      5. access config table again and rename 'coursesperpagexyz' to 'coursesperpage'
      Show
      access your DB and select the config table, rename 'coursesperpage' to 'coursesperpagexyz' as admin, access site admin > courses > add/edit courses select one of the categories make sure there's no error occurs access config table again and rename 'coursesperpagexyz' to 'coursesperpage'
    • Workaround:
      Hide

      Merely to go back up a level and then return down the path but its not really achieving the objective.

      Show
      Merely to go back up a level and then return down the path but its not really achieving the objective.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Coding error detected, it must be fixed by a programmer: paging_bar requires a perpage value.
       
      Stack trace:
      line 1936 of \lib\outputcomponents.php: coding_exception thrown
      line 2212 of \lib\outputrenderers.php: call to paging_bar->prepare()
      line 70 of \lib\outputrenderers.php: call to core_renderer->render_paging_bar()
      line 2201 of \lib\outputrenderers.php: call to renderer_base->render()
      line 261 of \course\category.php: call to core_renderer->paging_bar()
       
      Stack trace:
      line 1936 of \lib\outputcomponents.php: coding_exception thrown
      line 2212 of \lib\outputrenderers.php: call to paging_bar->prepare()
      line 70 of \lib\outputrenderers.php: call to core_renderer->render_paging_bar()
      line 2201 of \lib\outputrenderers.php: call to renderer_base->render()
      line 261 of \course\category.php: call to core_renderer->paging_bar()

      Home / ⟩ Courses / ⟩ A / ⟩ Course 1

      You should really redirect before you start page output
      line 562 of \lib\outputrenderers.php: call to debugging()
      line 2495 of \lib\weblib.php: call to core_renderer->redirect_message()
      line 699 of \mod\scorm\locallib.php: call to redirect()
      line 14 of \course\format\scorm\format.php: call to scorm_course_format_display()
      line 240 of \course\view.php: call to require()

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Adrian Jones added a comment -

            It does make adding and changing courses very trying and difficult .. double the time it should be taking .. so I don't think its that minor a problem.

            Show
            Adrian Jones added a comment - It does make adding and changing courses very trying and difficult .. double the time it should be taking .. so I don't think its that minor a problem.
            Hide
            Michael de Raadt added a comment -

            Hi, Adrian.

            Thanks for reporting this. I wasn't able to replicate your problem.

            Could you please provide replication instructions and perhaps some more details that would let us determine why this is happening in your case?

            Show
            Michael de Raadt added a comment - Hi, Adrian. Thanks for reporting this. I wasn't able to replicate your problem. Could you please provide replication instructions and perhaps some more details that would let us determine why this is happening in your case?
            Hide
            Adrian Jones added a comment -

            This was a totally clean new install .. on Server 2008 .. with MSSQL and Moodle the lastest build (two weeks ago now .. version number not with Me but I'll check later on).

            The problem happened immediately I started to try adding courses...

            So.. what details do you need ... ? I'd run the debugging expecting that would give the necessary data...

            Show
            Adrian Jones added a comment - This was a totally clean new install .. on Server 2008 .. with MSSQL and Moodle the lastest build (two weeks ago now .. version number not with Me but I'll check later on). The problem happened immediately I started to try adding courses... So.. what details do you need ... ? I'd run the debugging expecting that would give the necessary data...
            Hide
            Michael de Raadt added a comment -

            This issue has now been replicated and appears to be affecting current versions.

            I'm still not able to replicate the problem, so it would be good if someone can help us determine why this is happening to some people and not others.

            Show
            Michael de Raadt added a comment - This issue has now been replicated and appears to be affecting current versions. I'm still not able to replicate the problem, so it would be good if someone can help us determine why this is happening to some people and not others.
            Hide
            Michael de Raadt added a comment -

            Comment from duplicate issue...

            Lucas P added a comment - 17/Jun/12 3:56 AM

            I've checked course/category.php where it says:
            $perpage = optional_param('perpage', $CFG->coursesperpage, PARAM_INT); // how many per page

            So, as I understand its an optional parameter, but then the Paging component is enforcing the parameter to have a value? Am I right?
            ...
            That's right.

            I hardcoded $perpage = 10 and now the page loads.

            So what would be the right way to fix this issue?
            ...
            Hmmm, found this post http://moodle.org/mod/forum/discuss.php?d=163731

            Changing this through the web admin interface makes it work.

            Show
            Michael de Raadt added a comment - Comment from duplicate issue... Lucas P added a comment - 17/Jun/12 3:56 AM I've checked course/category.php where it says: $perpage = optional_param('perpage', $CFG->coursesperpage, PARAM_INT); // how many per page So, as I understand its an optional parameter, but then the Paging component is enforcing the parameter to have a value? Am I right? ... That's right. I hardcoded $perpage = 10 and now the page loads. So what would be the right way to fix this issue? ... Hmmm, found this post http://moodle.org/mod/forum/discuss.php?d=163731 Changing this through the web admin interface makes it work.
            Hide
            Michael de Raadt added a comment -

            Hi, Lucas.

            I've added you as a watcher on this issue.

            Show
            Michael de Raadt added a comment - Hi, Lucas. I've added you as a watcher on this issue.
            Hide
            Michael de Raadt added a comment - - edited

            Perhaps the best solution is to add a default value in the optional_param() call...

            $perpage = optional_param('perpage', $CFG>coursesperpage, PARAM_INT, 10);

            No, that won't work.

            Show
            Michael de Raadt added a comment - - edited Perhaps the best solution is to add a default value in the optional_param() call... $perpage = optional_param('perpage', $CFG > coursesperpage, PARAM_INT, 10); No, that won't work.
            Hide
            Rossiani Wijaya added a comment -

            I can't replicate this issue. I tried by installing a fresh copy of moodle.

            Show
            Rossiani Wijaya added a comment - I can't replicate this issue. I tried by installing a fresh copy of moodle.
            Hide
            Rossiani Wijaya added a comment -

            Hi Adrian and Lucas,

            If you still getting the error for this, could you try visiting the notification page (site admin > notifications).

            This page will check for any missing config setting. For this case it will display the 'new settings - front page settings' page. (attach screenshot). On this page, you can change the value or keep the default value. please make sure to select the 'save changes' button.

            Please let me know if the error still occurs after the trying the above steps.

            Thank you.
            Rosie

            Show
            Rossiani Wijaya added a comment - Hi Adrian and Lucas, If you still getting the error for this, could you try visiting the notification page (site admin > notifications). This page will check for any missing config setting. For this case it will display the 'new settings - front page settings' page. (attach screenshot). On this page, you can change the value or keep the default value. please make sure to select the 'save changes' button. Please let me know if the error still occurs after the trying the above steps. Thank you. Rosie
            Hide
            Aparup Banerjee added a comment -

            Hi,
            i've just had a brief look at this and imo:
            it seems for some reason (upgrade, install, setting issues?) that $CFG->coursesperpage isn't getting set. We need to investigate this root cause here. any hardcoding of default coursesperpage should be only in the setting for the config, if that default isn't working, lets find out why.

            Show
            Aparup Banerjee added a comment - Hi, i've just had a brief look at this and imo: it seems for some reason (upgrade, install, setting issues?) that $CFG->coursesperpage isn't getting set. We need to investigate this root cause here. any hardcoding of default coursesperpage should be only in the setting for the config, if that default isn't working, lets find out why.
            Hide
            Michael de Raadt added a comment -

            Hi, Apu.

            It would be good to know the cause if why this config setting is not set, but I don't think it's something we should invest effort into.

            The ultimate problem is that we cannot be assured that config settings are being set to default values, which happens for various reasons and is a fault in Moodle generally. I think that should be tackled as a separate issue and I will elaborate on that by writing such an issue.

            For now, I believe we should not avoid putting in a simple fix that can be reversed later (following appropriate mechanisms) when a better fix is implemented.

            Show
            Michael de Raadt added a comment - Hi, Apu. It would be good to know the cause if why this config setting is not set, but I don't think it's something we should invest effort into. The ultimate problem is that we cannot be assured that config settings are being set to default values, which happens for various reasons and is a fault in Moodle generally. I think that should be tackled as a separate issue and I will elaborate on that by writing such an issue. For now, I believe we should not avoid putting in a simple fix that can be reversed later (following appropriate mechanisms) when a better fix is implemented.
            Hide
            Michael de Raadt added a comment -

            I think the reason why such missing settings are coming about is because of upgrades where the user fails to click save on new settings.

            That results in errors that are hard to reproduce and are often closed as "cannot reproduce".

            Show
            Michael de Raadt added a comment - I think the reason why such missing settings are coming about is because of upgrades where the user fails to click save on new settings. That results in errors that are hard to reproduce and are often closed as "cannot reproduce".
            Hide
            Rossiani Wijaya added a comment -

            Created patch based on Michael's comment.

            Sending for peer-review.

            Show
            Rossiani Wijaya added a comment - Created patch based on Michael's comment. Sending for peer-review.
            Hide
            Ankit Agarwal added a comment -

            Hi Rosie,
            This looks good.
            You may also want to comment on MDL-35138 to revert your patch once that is integrated.

            Thanks

            Show
            Ankit Agarwal added a comment - Hi Rosie, This looks good. You may also want to comment on MDL-35138 to revert your patch once that is integrated. Thanks
            Hide
            Rossiani Wijaya added a comment -

            Thanks Ankit.

            Submitting for integration review.

            Show
            Rossiani Wijaya added a comment - Thanks Ankit. Submitting for integration review.
            Hide
            Eloy Lafuente (stronk7) 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.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) 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. TIA and ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            Aparup Banerjee added a comment -

            ah, thanks for creating MDL-35138 Michael

            Show
            Aparup Banerjee added a comment - ah, thanks for creating MDL-35138 Michael
            Hide
            Aparup Banerjee added a comment -

            this looks ok but i'm still unsure about the hardcoding (even if temporary) , i've pinged integrators for opinions incase theres better insight. i can't help thinking i'm not seeing something obvious..

            so barring further revelations this looks good to go in , just awaiting any +1's

            Show
            Aparup Banerjee added a comment - this looks ok but i'm still unsure about the hardcoding (even if temporary) , i've pinged integrators for opinions incase theres better insight. i can't help thinking i'm not seeing something obvious.. so barring further revelations this looks good to go in , just awaiting any +1's
            Hide
            Dan Poltawski added a comment -

            My -1 to this solution, because:

            1/ It is fine to depend on CFG values being set IMO, this is standard practice all around the code
            2/ This issue only handles the case where the CFG value is not set.
            3/ It doesn't handle the case for $CFG->coursesperpage set to 0.

            It seems to me like the admin UI allows $CFG->coursesperpage to be set to 0, this is the real bug here. We need to prevent that from happening in the admin UI.

            Show
            Dan Poltawski added a comment - My -1 to this solution, because: 1/ It is fine to depend on CFG values being set IMO, this is standard practice all around the code 2/ This issue only handles the case where the CFG value is not set. 3/ It doesn't handle the case for $CFG->coursesperpage set to 0. It seems to me like the admin UI allows $CFG->coursesperpage to be set to 0, this is the real bug here. We need to prevent that from happening in the admin UI.
            Hide
            Dan Poltawski added a comment -

            Linking duplicate MDL-26042

            Show
            Dan Poltawski added a comment - Linking duplicate MDL-26042
            Hide
            Aparup Banerjee added a comment -

            yep ditto, reopening especially for the bit about '0'.

            but really as mentioned earlier, we need to see why the default for the config setting hasn't been applied (upgrade?)

            i think the best way to go about this bug first of all is to replicate this issue here in the same environment.

            Show
            Aparup Banerjee added a comment - yep ditto, reopening especially for the bit about '0'. but really as mentioned earlier, we need to see why the default for the config setting hasn't been applied (upgrade?) i think the best way to go about this bug first of all is to replicate this issue here in the same environment.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Rajesh Taneja added a comment - - edited

            I agree with Dan about "perpage set to 0" case. But setting it to hard-coded value seems reasonable. If CFG is not set, then it should be set. As we do in lot of places like feedback, forum, assign, wiki etc. we should set this value as well. Normally, if user has not set any preference then we define default, this is kinda similar case where value is not set at system level.

            In addition to that, we support $CFG->coursesperpage = 0 (Site administration - > Front page - > Front page settings), it might be nice to make sure it can't be set to 0. Also, this bug can be reproduced by setting $CFG- >coursesperpage = 0 on Front page settings page, which can be added in test instructions.

            Show
            Rajesh Taneja added a comment - - edited I agree with Dan about "perpage set to 0" case. But setting it to hard-coded value seems reasonable. If CFG is not set, then it should be set. As we do in lot of places like feedback, forum, assign, wiki etc. we should set this value as well. Normally, if user has not set any preference then we define default, this is kinda similar case where value is not set at system level. In addition to that, we support $CFG->coursesperpage = 0 (Site administration - > Front page - > Front page settings), it might be nice to make sure it can't be set to 0. Also, this bug can be reproduced by setting $CFG- >coursesperpage = 0 on Front page settings page, which can be added in test instructions.
            Hide
            Michael de Raadt added a comment -

            I'm bringing this into the current sprint so we don't loose sight of it.

            In relation to relying on the configuration setting, apparently we cannot. This issue addresses a specific problem and should be resolved within its own scope. We have created MDL-35138 to deal with the greater problem.

            Dan raised a duplicate issue. It might be worth looking for other places where this perpage value is being used without a check. That is still within the scope of this current issue.

            Show
            Michael de Raadt added a comment - I'm bringing this into the current sprint so we don't loose sight of it. In relation to relying on the configuration setting, apparently we cannot. This issue addresses a specific problem and should be resolved within its own scope. We have created MDL-35138 to deal with the greater problem. Dan raised a duplicate issue. It might be worth looking for other places where this perpage value is being used without a check. That is still within the scope of this current issue.
            Hide
            Rossiani Wijaya added a comment -

            Hi Guys,

            Thank you for the feedback.

            I updated the patch to use !empty instead of isset().

            Re-submitting for integration review.

            Show
            Rossiani Wijaya added a comment - Hi Guys, Thank you for the feedback. I updated the patch to use !empty instead of isset(). Re-submitting for integration review.
            Hide
            Dan Poltawski added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            Aparup Banerjee 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.

            TIA and ciao

            Show
            Aparup Banerjee 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. TIA and ciao
            Hide
            Dan Poltawski 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.

            TIA and ciao

            Show
            Dan Poltawski 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. TIA and ciao
            Hide
            Aparup Banerjee added a comment -

            ok in the interests of stability for 2.4 release (work is done), ongoing discussion in wider scope (MDL-35138) and further evidence from triaging suggesting this might actually be a broader problem (MDL-36065) ;

            I've integrated this into 22, 23 and master(2.4dev).

            ps:noting some review commits could've been squashed but no time.

            Show
            Aparup Banerjee added a comment - ok in the interests of stability for 2.4 release (work is done), ongoing discussion in wider scope ( MDL-35138 ) and further evidence from triaging suggesting this might actually be a broader problem ( MDL-36065 ) ; I've integrated this into 22, 23 and master(2.4dev). ps:noting some review commits could've been squashed but no time.
            Hide
            Aparup Banerjee added a comment - - edited

            works, passing. (not specific to MSSQL btw)

            Show
            Aparup Banerjee added a comment - - edited works, passing. (not specific to MSSQL btw)
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

            (not really)

            Closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: