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

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

    Details

    • Type: Bug
    • Status: Open
    • Priority: 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

      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.

        Gliffy Diagrams

        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
            romram12 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
            romram12 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
            hchathi 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
            hchathi 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
            mjollnir 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
            mjollnir 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
            mjollnir Penny Leach added a comment -

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

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

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

            Show
            hchathi Hubert Chathi added a comment - I'll give it a try. But I need to remember what conditions triggered this bug...
            Hide
            hchathi 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
            hchathi 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
            mjollnir Penny Leach added a comment -

            ping

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

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

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

            ping

            (this is my last moodle hq week)

            Show
            mjollnir Penny Leach added a comment - ping (this is my last moodle hq week)
            Hide
            hchathi 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
            hchathi 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
            mjollnir 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
            mjollnir 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
            hchathi Hubert Chathi added a comment -

            The patch for 1.9 looks good. Thanks!

            Show
            hchathi Hubert Chathi added a comment - The patch for 1.9 looks good. Thanks!
            Hide
            salvetore 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
            salvetore 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
            hchathi 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
            hchathi 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
            salvetore 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
            salvetore 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
            hchathi 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
            hchathi 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
            dobedobedoh Andrew Nicols added a comment -

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

            Show
            dobedobedoh 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: