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

add_to_log should check the return value of insert_record and send and flag erors to admin

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.1
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      When the disk fills up, the add_to_log() function is usually the first thing to fail.

      We can alert the $CFG->supportemail address of the problem when insert fails.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              lazyfish Yu Zhang added a comment -

              Added in 1.9 and 2.0

              Show
              lazyfish Yu Zhang added a comment - Added in 1.9 and 2.0
              Hide
              dougiamas Martin Dougiamas added a comment -

              Dongsheng, can you please alter this so that:

              • the insert query is added to the email so we have more information about the failure
              • when an email is sent it checks for a config flag:
                $lasttime = get_config('admin', 'inserterroremail');
              • if this value is less than one day ago then it does nothing.
              • if the value is more that one day ago then it sends the mail and resets the config flag set_config('inserterrormail', time(), 'admin');
              Show
              dougiamas Martin Dougiamas added a comment - Dongsheng, can you please alter this so that: the insert query is added to the email so we have more information about the failure when an email is sent it checks for a config flag: $lasttime = get_config('admin', 'inserterroremail'); if this value is less than one day ago then it does nothing. if the value is more that one day ago then it sends the mail and resets the config flag set_config('inserterrormail', time(), 'admin');
              Hide
              dongsheng Dongsheng Cai added a comment -

              fixed in 1.9.1, and merged into head, please review.

              Show
              dongsheng Dongsheng Cai added a comment - fixed in 1.9.1, and merged into head, please review.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks for the patch Dongsheng.

              I've detected some problems with it and I've committed a new version. It includes:

              • Fixed logic: If the 'inserterrormail' config variable didn't exist, email wasn't ever sent!
              • Renamed 'inserterrormail' to 'lastloginserterrormail'. More precise var name IMO.
              • Minor formatting changes in the email (just added a couple of linefeeds here and there).

              Sent to 19_STABLE and HEAD. Changing QA to Petr

              About the direct use of the mail() function.... it's a good idea although perhaps it would be ok to use some semaphore (static variable) to avoid recursion and then use the standard email_to_user() Moodle alternative. It handles better a lot of things.

              For your consideration. In its current state, it's enough good to build the WEEKLY properly. Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks for the patch Dongsheng. I've detected some problems with it and I've committed a new version. It includes: Fixed logic: If the 'inserterrormail' config variable didn't exist, email wasn't ever sent! Renamed 'inserterrormail' to 'lastloginserterrormail'. More precise var name IMO. Minor formatting changes in the email (just added a couple of linefeeds here and there). Sent to 19_STABLE and HEAD. Changing QA to Petr About the direct use of the mail() function.... it's a good idea although perhaps it would be ok to use some semaphore (static variable) to avoid recursion and then use the standard email_to_user() Moodle alternative. It handles better a lot of things. For your consideration. In its current state, it's enough good to build the WEEKLY properly. Ciao
              Hide
              skodak Petr Skoda added a comment -

              if (!$result and debugging())

              { echo '<p>Error: Could not insert a new entry to the Moodle log</p>'; // Don't throw an error }

              should be imo improved - DEBUG_ALL level I think

              Show
              skodak Petr Skoda added a comment - if (!$result and debugging()) { echo '<p>Error: Could not insert a new entry to the Moodle log</p>'; // Don't throw an error } should be imo improved - DEBUG_ALL level I think
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Good point. Change to proper use of debugging() - with DEBUG_ALL level. Thanks and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Good point. Change to proper use of debugging() - with DEBUG_ALL level. Thanks and ciao
              Hide
              skodak Petr Skoda added a comment -

              code reviewed, closing - thanks everybody!

              Show
              skodak Petr Skoda added a comment - code reviewed, closing - thanks everybody!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    15/May/08