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

          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