Moodle
  1. Moodle
  2. MDL-18720

print_mnet_log treats $course as an object, even though an integer is passed in

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.9.4, 2.3.6
    • Fix Version/s: None
    • Component/s: MNet
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_23_STABLE
    • Rank:
      5158

      Description

      In /course/report/log/index.php, print_mnet_log is called with the second argument being the course ID, rather than the entire course object. However, print_mnet_log in /course/lib.php still tries to access $course->id, which of course fails. Probably the easiest fix is to replace "$course->id" with just "$course" within print_mnet_log. Additionally, it may be good to rename the variable to be "$courseid" instead of just "$course", to make things clearer.

      1. 0001-mnet-MDL-18720-pass-the-right-parameters-to-mnet-log.patch
        3 kB
        Penny Leach
      2. MDL-18720-1.9.patch.txt
        3 kB
        Penny Leach
      3. MDL-18720-2.0.patch.txt
        4 kB
        Penny Leach
      4. print_mnet_patch.txt
        0.8 kB
        Udit Sajjanhar

        Issue Links

          Activity

          Hide
          Udit Sajjanhar added a comment -

          the second param for the function print_mnet_log has been changed from $id to $course.

          This approach is better than changing the body of function to use $course instead of $course->id because print_mnet_log calls build_mnet_logs_array which also requires the $course object.

          Show
          Udit Sajjanhar added a comment - the second param for the function print_mnet_log has been changed from $id to $course. This approach is better than changing the body of function to use $course instead of $course->id because print_mnet_log calls build_mnet_logs_array which also requires the $course object.
          Hide
          Hubert Chathi added a comment -

          Uhm, sorry, no. build_mnet_logs_array treats $course as a number, not an object. This is true in both HEAD and the latest 1.9 from CVS. There is a bunch of commented code in build_ment_logs_array that treats it as an object, but the code itself treats it as a number.

          Show
          Hubert Chathi added a comment - Uhm, sorry, no. build_mnet_logs_array treats $course as a number, not an object. This is true in both HEAD and the latest 1.9 from CVS. There is a bunch of commented code in build_ment_logs_array that treats it as an object, but the code itself treats it as a number.
          Hide
          Penny Leach added a comment -

          it looks like build_mnet_logs_array wants an integer and print_mnet_log wants an object.

          I will change the call in index.php to pass $course, and change the parameter name of build_mnet_logs_array to courseid and also add phpdocs.

          Show
          Penny Leach added a comment - it looks like build_mnet_logs_array wants an integer and print_mnet_log wants an object. I will change the call in index.php to pass $course, and change the parameter name of build_mnet_logs_array to courseid and also add phpdocs.
          Hide
          Penny Leach added a comment -

          Can you guys test this patch please? I don't really have the environment set up properly yet.

          Show
          Penny Leach added a comment - Can you guys test this patch please? I don't really have the environment set up properly yet.
          Hide
          Hubert Chathi added a comment -

          I'll give it a try. But I need to remember what conditions triggered this bug...

          Show
          Hubert Chathi added a comment - I'll give it a try. But I need to remember what conditions triggered this bug...
          Hide
          Hubert Chathi added a comment -

          Haven't tried it yet (I'm still not sure how to trigger and debug this bug – it's been a while), but the changes look correct.

          Show
          Hubert Chathi added a comment - Haven't tried it yet (I'm still not sure how to trigger and debug this bug – it's been a while), but the changes look correct.
          Hide
          Penny Leach added a comment -

          ping

          Show
          Penny Leach added a comment - ping
          Hide
          Hubert Chathi added a comment -

          Yes, I'll look into this (after I un-break My Moodle )

          Show
          Hubert Chathi added a comment - Yes, I'll look into this (after I un-break My Moodle )
          Hide
          Penny Leach added a comment -

          ping

          (this is my last moodle hq week)

          Show
          Penny Leach added a comment - ping (this is my last moodle hq week)
          Hide
          Hubert Chathi added a comment -

          Hi Penny,
          Sorry, I finally un-broke My Moodle, and managed to figure out how to trigger this code. Your patch needs this additional diff for /course/lib.php:

          @@ -139,11 +149,11 @@ function build_mnet_logs_array($hostid, $course, $user=0, $date=0, $order="l.tim
                       WHERE
                           ";
           
          -    $where .= "l.hostid = '$hostid'";
          +    $where = "l.hostid = '$hostid'";
           
               // TODO: Is 1 really a magic number referring to the sitename?
          -    if ($course != 1 || $modid != 0) {
          -        $where .= " AND\n                l.course='$course'";
          +    if ($courseid != 1 || $modid != 0) {
          +        $where .= " AND\n                l.course='$courseid'";
               }
           
               if ($modname) {
          

          (the modification to the "$where =" line isn't strictly required, but gets rid of a warning in debugging mode)
          Otherwise, the patch is good.

          Show
          Hubert Chathi added a comment - Hi Penny, Sorry, I finally un-broke My Moodle, and managed to figure out how to trigger this code. Your patch needs this additional diff for /course/lib.php: @@ -139,11 +149,11 @@ function build_mnet_logs_array($hostid, $course, $user=0, $date=0, $order="l.tim WHERE "; - $where .= "l.hostid = '$hostid'" ; + $where = "l.hostid = '$hostid'" ; // TODO: Is 1 really a magic number referring to the sitename? - if ($course != 1 || $modid != 0) { - $where .= " AND\n l.course='$course'" ; + if ($courseid != 1 || $modid != 0) { + $where .= " AND\n l.course='$courseid'" ; } if ($modname) { (the modification to the "$where =" line isn't strictly required, but gets rid of a warning in debugging mode) Otherwise, the patch is good.
          Hide
          Penny Leach added a comment -

          ok, final patches attached for 1.9 and 2.0. please make sure you're happy with them and then i'll commit

          and thanks heaps for helping!

          Show
          Penny Leach added a comment - ok, final patches attached for 1.9 and 2.0. please make sure you're happy with them and then i'll commit and thanks heaps for helping!
          Hide
          Hubert Chathi added a comment -

          The patch for 1.9 looks good. Thanks!

          Show
          Hubert Chathi added a comment - The patch for 1.9 looks good. Thanks!
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          lqjjLKA0p6

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
          Hide
          Hubert Chathi added a comment -

          Yes, it looks like 2.1, at least, still has the problemmatic code. Penny's patch from February 2010 (MDL-18720-2.0.patch.txt) still applies to 2.1, except for the last hunk in course/lib.php, which is just a newline change.

          Show
          Hubert Chathi added a comment - Yes, it looks like 2.1, at least, still has the problemmatic code. Penny's patch from February 2010 ( MDL-18720 -2.0.patch.txt) still applies to 2.1, except for the last hunk in course/lib.php, which is just a newline change.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          4d6f6f646c6521

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; 4d6f6f646c6521
          Hide
          Hubert Chathi added a comment -

          I've added 2.5 as an affected version, because the affected code is still in the latest master. It looks like Penny's patch still applies, except that course/report/log/index.php was moved to report/log/index.php

          Show
          Hubert Chathi added a comment - I've added 2.5 as an affected version, because the affected code is still in the latest master. It looks like Penny's patch still applies, except that course/report/log/index.php was moved to report/log/index.php
          Hide
          Andrew Nicols added a comment -

          Corrected affected version to 2.3 branch as this is the earliest affected stable branch.

          Show
          Andrew Nicols added a comment - Corrected affected version to 2.3 branch as this is the earliest affected stable branch.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated: