Moodle
  1. Moodle
  2. MDL-31552

Change add_to_log() exception to add exception information

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide
      1. In your Moodle instance go to "Settings -> Site administration -> Development -> Debugging"
      2. Set Debug messages to Developer
      3. Check the Display debug messages checkbox
      4. Create a script called testAddLogException.php in your Moodle root directory with the following contents
        <?php
        require_once('config.php');
        add_to_log('a', 'foo', 'bar', 'admin/foo/bar.php', 'foobar', '', $USER->id);
        
      5. Now when visiting <yourMoodleLocation>/testAddLogException.php" class="external-link" rel="nofollow">http://<yourMoodleLocation>/testAddLogException.php you should see debugging message ERROR: invalid input syntax for integer: "a"

      Before the patch you would just see "Error: Could not insert a new entry to the Moodle log"

      Show
      In your Moodle instance go to "Settings -> Site administration -> Development -> Debugging" Set Debug messages to Developer Check the Display debug messages checkbox Create a script called testAddLogException.php in your Moodle root directory with the following contents <?php require_once('config.php'); add_to_log('a', 'foo', 'bar', 'admin/foo/bar.php', 'foobar', '', $USER->id); Now when visiting <yourMoodleLocation> /testAddLogException.php" class="external-link" rel="nofollow">http:// <yourMoodleLocation> /testAddLogException.php you should see debugging message ERROR: invalid input syntax for integer: "a" Before the patch you would just see "Error: Could not insert a new entry to the Moodle log"
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      m_MDL-31552_change_add_to_log_exception_to_add_exception_information
    • Rank:
      38107

      Description

      Currently if you try to make a call to add_to_log() and the call to insert_record_raw() returns a dml_exception. The following error is shown "Error: Could not insert a new entry to the Moodle log" which doesn't really tell you anything about the issue, while information available in the exception is not being used.

        Activity

        Hide
        Andrew Davis added a comment -

        Looks like a good change to me.

        Show
        Andrew Davis added a comment - Looks like a good change to me.
        Hide
        Gerard Caulfield added a comment -

        Thanks Andrew

        Show
        Gerard Caulfield added a comment - Thanks Andrew
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Gerard,

        while the patch looks good, I'm just guessing if it won't be better to show both the old string and the error details together. That will help both:

        • people already parsing the old string from their logs will continue parsing it without problem.
        • people getting the error will have better (human) information than just the DB backend error.

        So something like:

        Error: Could not insert a new entry to the Moodle log: ERROR: invalid input syntax for integer: "a"
        

        Sounds better for those two points. For your consideration.

        Also, note this is one improvement and, given that, only will land to Moodle 2.3 (master), unless the opposite is justified.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Gerard, while the patch looks good, I'm just guessing if it won't be better to show both the old string and the error details together. That will help both: people already parsing the old string from their logs will continue parsing it without problem. people getting the error will have better (human) information than just the DB backend error. So something like: Error: Could not insert a new entry to the Moodle log: ERROR: invalid input syntax for integer: "a" Sounds better for those two points. For your consideration. Also, note this is one improvement and, given that, only will land to Moodle 2.3 (master), unless the opposite is justified. Ciao
        Hide
        Gerard Caulfield added a comment -

        If by chance somebody was parsing for this error, wouldn't the addition of the second part of the string more than likely break their parsing anyway?

        More importantly though, why would somebody be parsing this error? Surely it would only occur in exceptional circumstances such as when somebody has provided the wrong arguments to the add_to_log() function, in which case they would be a developer.

        Just asking so I can learn, I am happy to make the change you have requested.

        As for it just being part of 2.3, that is no problem at all. I just wanted to give the option of back-porting it in case you wished to do so.

        Show
        Gerard Caulfield added a comment - If by chance somebody was parsing for this error, wouldn't the addition of the second part of the string more than likely break their parsing anyway? More importantly though, why would somebody be parsing this error? Surely it would only occur in exceptional circumstances such as when somebody has provided the wrong arguments to the add_to_log() function, in which case they would be a developer. Just asking so I can learn, I am happy to make the change you have requested. As for it just being part of 2.3, that is no problem at all. I just wanted to give the option of back-porting it in case you wished to do so.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hehe,

        well, for sure it's not the most usual thing to do... but in the past I've been, in various systems, monitoring logs for a given msg error. So I can imagine one site that have run under some problems with the logs table and is running some monitor on logs looking for the "An error inserting logs has occurred" string. With your change, the string is out and all those (100% hypothetical, as said) monitors will stop working. That was my logic for the 1st point in my comment above.

        But really, I think the 2nd one is more important. With your change, the user will get the contents of the DB error (from the DB). And each DB has its own error codes/messages, more yet, some are not able to inform properly about the exact error.

        That was the reason for suggesting to keep both the old string (as a general identification of the action/error), and then append to it the DB error. Imagine:

        • "ORA-12345 maximum cursors exceeded"

        versus:

        • "An error inserting logs has occurred: ORA-12345 maximum cursors exceeded"

        I find the 2nd better, showing both the "action" error and the backend DB error.

        Just that, nothing critical, but a reflexion. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hehe, well, for sure it's not the most usual thing to do... but in the past I've been, in various systems, monitoring logs for a given msg error. So I can imagine one site that have run under some problems with the logs table and is running some monitor on logs looking for the "An error inserting logs has occurred" string. With your change, the string is out and all those (100% hypothetical, as said) monitors will stop working. That was my logic for the 1st point in my comment above. But really, I think the 2nd one is more important. With your change, the user will get the contents of the DB error (from the DB). And each DB has its own error codes/messages, more yet, some are not able to inform properly about the exact error. That was the reason for suggesting to keep both the old string (as a general identification of the action/error), and then append to it the DB error. Imagine: "ORA-12345 maximum cursors exceeded" versus: "An error inserting logs has occurred: ORA-12345 maximum cursors exceeded" I find the 2nd better, showing both the "action" error and the backend DB error. Just that, nothing critical, but a reflexion. Ciao
        Hide
        Gerard Caulfield added a comment -

        All good points. Thanks for taking the time to share your wisdom.

        I've changed the patch to reflect your idea and removed the ones for 2.2 and 2.1

        Thanks Eloy

        Show
        Gerard Caulfield added a comment - All good points. Thanks for taking the time to share your wisdom. I've changed the patch to reflect your idea and removed the ones for 2.2 and 2.1 Thanks Eloy
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (master only). Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (master only). Thanks!
        Hide
        Jason Fowler added a comment -

        I'm getting Error: Could not insert a new entry to the Moodle log. Incorrect integer value: 'a' for column 'course' at row 1
        line 1726 of /lib/datalib.php: call to debugging()
        line 3 of /testAddLogException.php: call to add_to_log()

        Is that right?

        Show
        Jason Fowler added a comment - I'm getting Error: Could not insert a new entry to the Moodle log. Incorrect integer value: 'a' for column 'course' at row 1 line 1726 of /lib/datalib.php: call to debugging() line 3 of /testAddLogException.php: call to add_to_log() Is that right?
        Hide
        Jason Fowler added a comment -

        Test passed

        Show
        Jason Fowler added a comment - Test passed
        Hide
        Eloy Lafuente (stronk7) added a comment -

        It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

        Closing as fixed, heading to zzzZZZzzz, niao

        Show
        Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao
        Hide
        Gerard Caulfield added a comment -

        Thanks Eloy, Jason and Andrew.

        Show
        Gerard Caulfield added a comment - Thanks Eloy, Jason and Andrew.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: