Moodle
  1. Moodle
  2. MDL-36091

Check all the "should never happen" type errors are handled

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.5
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      Testing this is a little fuzzy. Check that debugging is turned on. Log in, use something ajaxy, enrol a user. If you see any of the new debugging messages raise MDLs (having searched for an existing MDL first).

      Show
      Testing this is a little fuzzy. Check that debugging is turned on. Log in, use something ajaxy, enrol a user. If you see any of the new debugging messages raise MDLs (having searched for an existing MDL first).
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-36091_should_never_happen
    • Rank:
      44852

      Description

      If you do this you get a bunch of results.

      grep -R "never happen" .

      We should go through them and check that we are handling them in some way. Throwing an exception or something/anything.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment -

          Adding a branch containing a potential fix. I've just added a number of debugging() calls. That should flush out any spots where things that shouldn't be happening are happening, hopefully without breaking any existing functionality.

          Show
          Andrew Davis added a comment - Adding a branch containing a potential fix. I've just added a number of debugging() calls. That should flush out any spots where things that shouldn't be happening are happening, hopefully without breaking any existing functionality.
          Hide
          Ankit Agarwal added a comment -

          Hi Andrew will it be better to change
          https://github.com/andyjdavis/moodle/blob/d0dfc35de74eb128094677e07aaccc34888a8f20/lib/accesslib.php#L384
          to something more like 'Empty global $USER variable'?
          Same with the later usage in the same file
          Rest looks good.
          [y] Syntax
          [y] Output
          [y] Whitespace
          [y] Language
          [na] Databases
          [?] Testing It would be nice to test each instance, but I donot think that is possible
          [na] Security
          [na] Documentation
          [?] Git Commit msg is missing component name
          [y] Sanity check
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Andrew will it be better to change https://github.com/andyjdavis/moodle/blob/d0dfc35de74eb128094677e07aaccc34888a8f20/lib/accesslib.php#L384 to something more like 'Empty global $USER variable'? Same with the later usage in the same file Rest looks good. [y] Syntax [y] Output [y] Whitespace [y] Language [na] Databases [?] Testing It would be nice to test each instance, but I donot think that is possible [na] Security [na] Documentation [?] Git Commit msg is missing component name [y] Sanity check Thanks
          Hide
          Andrew Davis added a comment - - edited

          I did wonder about those checks. They seem deliberately specific. I opted to leave it alone as much as possible until we have reason to change it.

          Submitting for integration. I'd suggest that this go into master only as these code changes are aimed at flushing out bugs rather than fixing them.

          Show
          Andrew Davis added a comment - - edited I did wonder about those checks. They seem deliberately specific. I opted to leave it alone as much as possible until we have reason to change it. Submitting for integration. I'd suggest that this go into master only as these code changes are aimed at flushing out bugs rather than fixing them.
          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
          Dan Poltawski added a comment -

          Looks like this is master only and so adding integration_held.

          Show
          Dan Poltawski added a comment - Looks like this is master only and so adding integration_held.
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          These debugging calls should be DEBUG_DEVELOPER I think as they probably can't be fixed by anyone but a developer.

          Show
          Dan Poltawski added a comment - Hi Andrew, These debugging calls should be DEBUG_DEVELOPER I think as they probably can't be fixed by anyone but a developer.
          Hide
          Andrew Davis added a comment -

          I've updated the debugging calls to be developer only. Resubmitting for integration. master only.

          Show
          Andrew Davis added a comment - I've updated the debugging calls to be developer only. Resubmitting for integration. master only.
          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
          Damyon Wiese added a comment -

          Hi Andrew,

          Thanks for the patch - it's good to improve the code like this.

          I found some issues though:

          1. This debugging is triggered during unit tests:

          Debugging: Found empty "siteadmins" site setting

          • line 61 of /lib/datalib.php: call to debugging()

          2. Not sure of the value of calling debugging in the ajax renderer - the message will corrupt any ajax response. I think we should remove that one.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Andrew, Thanks for the patch - it's good to improve the code like this. I found some issues though: 1. This debugging is triggered during unit tests: Debugging: Found empty "siteadmins" site setting line 61 of /lib/datalib.php: call to debugging() 2. Not sure of the value of calling debugging in the ajax renderer - the message will corrupt any ajax response. I think we should remove that one. Regards, Damyon
          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
          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
          Andrew Davis added a comment -

          I've removed the debugging code in the ajax and also the one that was popping up during the unit tests. Resubmitting for integration.

          Show
          Andrew Davis added a comment - I've removed the debugging code in the ajax and also the one that was popping up during the unit tests. Resubmitting for integration.
          Hide
          Damyon Wiese 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.

          Cheers!

          Show
          Damyon Wiese 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. Cheers!
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Andrew

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Andrew
          Hide
          David Monllaó added a comment -

          I've been playing with enrol_manual and I haven't found any problem. Passing it

          Show
          David Monllaó added a comment - I've been playing with enrol_manual and I haven't found any problem. Passing it
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: