Moodle
  1. Moodle
  2. MDL-36640

print_log_csv, xls, and ods try to run get_field with a non-numeric ID

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.6
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Logging
    • Labels:
      None
    • Testing Instructions:
      Hide

      For 2.3 and master

      • Open a course and turn editing on
      • Add a new mod_assign with any settings you like
      • View the mod_assign you just created (this should create a suitable log entry)
      • In your DB, check you have some entries for mod_assign (SELECT * FROM mdl_log WHERE module = 'assign'; )
        • Check that at least one of these has a non-integer content in it's info field (if not, try and create some by submitting an assignment)
      • Open the site logs
      • Confirm that you have some logs displayed
      • Change the output to 'Download in text format' and get the logs
        • Confirm that the created txt file contains no HTML output or errors
      • Change the output to 'Download in ODS format'
        • Confirm that the created Open Office file actually opens and contains no errors
      • Change the output to 'Download in Excel format'
        • Confirm that the created excel file actually opens and contains no errors

      For 2.2

      It's harder to test here because the problems I've been seeing were introduced by mod_assign. That said, it should be included because there's probably something out there which does insert text into the info field.

      • Manually insert a new mdl_log entry which belongs to a valid user and course, and has some text in the info field.
      • Open the site logs
      • Confirm that you have some logs displayed
      • Change the output to 'Download in text format' and get the logs
        • Confirm that the created txt file contains no HTML output or errors
      • Change the output to 'Download in ODS format'
        • Confirm that the created Open Office file actually opens and contains no errors
      • Change the output to 'Download in Excel format'
        • Confirm that the created excel file actually opens and contains no errors
      Show
      For 2.3 and master Open a course and turn editing on Add a new mod_assign with any settings you like View the mod_assign you just created (this should create a suitable log entry) In your DB, check you have some entries for mod_assign (SELECT * FROM mdl_log WHERE module = 'assign'; ) Check that at least one of these has a non-integer content in it's info field (if not, try and create some by submitting an assignment) Open the site logs Confirm that you have some logs displayed Change the output to 'Download in text format' and get the logs Confirm that the created txt file contains no HTML output or errors Change the output to 'Download in ODS format' Confirm that the created Open Office file actually opens and contains no errors Change the output to 'Download in Excel format' Confirm that the created excel file actually opens and contains no errors For 2.2 It's harder to test here because the problems I've been seeing were introduced by mod_assign. That said, it should be included because there's probably something out there which does insert text into the info field. Manually insert a new mdl_log entry which belongs to a valid user and course, and has some text in the info field. Open the site logs Confirm that you have some logs displayed Change the output to 'Download in text format' and get the logs Confirm that the created txt file contains no HTML output or errors Change the output to 'Download in ODS format' Confirm that the created Open Office file actually opens and contains no errors Change the output to 'Download in Excel format' Confirm that the created excel file actually opens and contains no errors
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36640-master
    • Rank:
      46135

      Description

      Some add_to_log functions are adding text into the info field of the log table.
      Whilst course/lib.php::print_log() handles these correctly, the corresponding functions for CSV, XLS, and ODS output do not.

      They're all missing a check for is_numeric($log->info) which results in get_field() trying to fetch from a table (mdl_assign in my case) using a string for the ID field and throwing DB exceptions.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Will provide 2.2 and 2.3 branches after PR

          Show
          Andrew Nicols added a comment - Will provide 2.2 and 2.3 branches after PR
          Hide
          Andrew Nicols added a comment -

          I've removed the part of the commit which addressed the same issue in print_mnet_logs because MNet logs are far more broken than this and it's not currently possible to test the fix because of this. See MDL-36642 for the MNet log issues.

          Show
          Andrew Nicols added a comment - I've removed the part of the commit which addressed the same issue in print_mnet_logs because MNet logs are far more broken than this and it's not currently possible to test the fix because of this. See MDL-36642 for the MNet log issues.
          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [*] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Hello Andrew,

          The patch looks good. I can see that the export files are now using the same check as displaying to the screen.

          I only have one comment and that is with the testing instructions:

          • The select statement should probably read (SELECT * FROM mdl_log WHERE module = 'assign'

          Thanks.

          Show
          Adrian Greeve added a comment - [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [*] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hello Andrew, The patch looks good. I can see that the export files are now using the same check as displaying to the screen. I only have one comment and that is with the testing instructions: The select statement should probably read (SELECT * FROM mdl_log WHERE module = 'assign' Thanks.
          Hide
          Andrew Nicols added a comment -

          Thanking you kindly - apparently it's easy to miss those kinds of stupid errors when you spend a while working on an issue :|

          Show
          Andrew Nicols added a comment - Thanking you kindly - apparently it's easy to miss those kinds of stupid errors when you spend a while working on an issue :|
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now.
          Hide
          Frédéric Massart added a comment -

          Failing the test as I am not sure that the error I am having is related or not. If not, then we can pass it.

          On 2.3 and 2.4, when exporting to XLS, the following notice is present in the XLS file:

          Strict standards: Only variables should be assigned by reference in /home/fred/www/repositories/im/moodle/course/lib.php on line 668
          
          Call Stack:
              0.0007     767000   1. {main}() /home/fred/www/repositories/im/moodle/report/log/index.php:0
              0.2147   48671616   2. print_log_xls() /home/fred/www/repositories/im/moodle/report/log/index.php:188
          

          I read through that mod_assign created the problem, but isn't that because it still defines a log_display rule where as it's not supposed to use it any more? Shouldn't we remove those?

          Show
          Frédéric Massart added a comment - Failing the test as I am not sure that the error I am having is related or not. If not, then we can pass it. On 2.3 and 2.4, when exporting to XLS, the following notice is present in the XLS file: Strict standards: Only variables should be assigned by reference in /home/fred/www/repositories/im/moodle/course/lib.php on line 668 Call Stack: 0.0007 767000 1. {main}() /home/fred/www/repositories/im/moodle/report/log/index.php:0 0.2147 48671616 2. print_log_xls() /home/fred/www/repositories/im/moodle/report/log/index.php:188 I read through that mod_assign created the problem, but isn't that because it still defines a log_display rule where as it's not supposed to use it any more? Shouldn't we remove those?
          Hide
          Dan Poltawski added a comment -

          Ping, Andrew?

          Show
          Dan Poltawski added a comment - Ping, Andrew?
          Hide
          Andrew Nicols added a comment -

          Sorry - I hadn't seen this earlier.

          Looking at the line in question, I think that this is beyond the scope of this bug and is unrelated.

          The line in question (668) happens before the changes in the patch and would happen regardless from my reading of it.

          Show
          Andrew Nicols added a comment - Sorry - I hadn't seen this earlier. Looking at the line in question, I think that this is beyond the scope of this bug and is unrelated. The line in question (668) happens before the changes in the patch and would happen regardless from my reading of it.
          Hide
          Andrew Nicols added a comment -

          https://git.luns.net.uk/moodle.git/blob/MDL-36640-master:/course/lib.php#l668

          The line in question is

          $worksheet[$wsnumber] =& $workbook->add_worksheet($sheettitle);
          

          The logs are processed after this point and the log processing is the area which this patch affects.

          Show
          Andrew Nicols added a comment - https://git.luns.net.uk/moodle.git/blob/MDL-36640-master:/course/lib.php#l668 The line in question is $worksheet[$wsnumber] =& $workbook->add_worksheet($sheettitle); The logs are processed after this point and the log processing is the area which this patch affects.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing with MDL-37171 created about the (unrelated) E_STRICT problem. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing with MDL-37171 created about the (unrelated) E_STRICT problem. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: