Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36091

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

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              andyjdavis 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
              andyjdavis 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_frenz 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_frenz 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
              andyjdavis 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
              andyjdavis 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
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

              Looks like this is master only and so adding integration_held.

              Show
              poltawski Dan Poltawski added a comment - Looks like this is master only and so adding integration_held.
              Hide
              poltawski 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
              poltawski 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
              andyjdavis Andrew Davis added a comment -

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

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

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

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

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              andyjdavis 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
              andyjdavis 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 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 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
              poltawski Dan Poltawski added a comment -

              Integrated to master, thanks Andrew

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

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

              Show
              dmonllao David Monllaó added a comment - I've been playing with enrol_manual and I haven't found any problem. Passing it
              Hide
              damyon 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 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:
                    Fix Release Date:
                    14/May/13