Moodle
  1. Moodle
  2. MDL-32386

get_data() in lib/completionlib.php called with wrong number of parameters

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Enable completion in Advanced features
      Create a new course with Completion enabled
      Create a forum in the course, and enable "Students can manually mark this activity as completed"
      Create and enroll a user in the course
      Login as user, click checkbox beside forum to mark as complete
      Reload page and check the checkbox remains checked and no errors have occured

      Show
      Enable completion in Advanced features Create a new course with Completion enabled Create a forum in the course, and enable "Students can manually mark this activity as completed" Create and enroll a user in the course Login as user, click checkbox beside forum to mark as complete Reload page and check the checkbox remains checked and no errors have occured
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      39244

      Description

      In library lib/completionlib.php function set_module_viewed() calls get_data() on line 621 like this:

      $data = $this->get_data($cm, $userid);

      But the get_data function (same file) argument list is like this:

      public function get_data($cm, $wholecourse=false, $userid=0, $modinfo=null)

      {...}

      So it takes another argument before $userid! So I think we need to call the function like so:

      $data = $this->get_data($cm, false, $userid);

      This creates no problems if we are setting module viewed for $USER, and thus $userid is null.
      But if in some script or otherwise programatically we want to set module viewed for a specific user with a specific $userid, we will have a problem.

        Activity

        Hide
        Petr Škoda added a comment -

        thanks for the report

        Show
        Petr Škoda added a comment - thanks for the report
        Hide
        Aaron Barnes added a comment -

        Thanks for the report, sorry it hasn't been picked up before this!

        I have created a patch with your fix (which is correct), thanks!

        Show
        Aaron Barnes added a comment - Thanks for the report, sorry it hasn't been picked up before this! I have created a patch with your fix (which is correct), thanks!
        Hide
        David Monllaó added a comment -

        Hi Aaron, the patch seems perfect. I changed the pull master diff URL for https://github.com/srynot4sale/moodle/compare/master...MDL-32386, the old one points to another issue

        Show
        David Monllaó added a comment - Hi Aaron, the patch seems perfect. I changed the pull master diff URL for https://github.com/srynot4sale/moodle/compare/master...MDL-32386 , the old one points to another issue
        Hide
        Aaron Barnes added a comment -

        Thanks David!

        Show
        Aaron Barnes added a comment - Thanks David!
        Hide
        Aparup Banerjee added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Hi, Could we have some testing instructions for this issue?

        Show
        Dan Poltawski added a comment - Hi, Could we have some testing instructions for this issue?
        Hide
        Simon Coggins added a comment -

        Looks like you might also have to update the unit tests in lib/simpletest/testcompletionlib.php (on 2.2). It needs the new 2nd argument added into the get_data call on lines 268 and 278.

        In 2.3 it's been converted to PHPUnit so the tests are in lib/tests/completionlib_test.php instead.

        Show
        Simon Coggins added a comment - Looks like you might also have to update the unit tests in lib/simpletest/testcompletionlib.php (on 2.2). It needs the new 2nd argument added into the get_data call on lines 268 and 278. In 2.3 it's been converted to PHPUnit so the tests are in lib/tests/completionlib_test.php instead.
        Hide
        Dan Poltawski added a comment -

        Hmm, thanks Simon.

        Yes, this is also conflicting on master, seemingly with old simpletest bits. So i'm not confident this applies on master.

        Reopening.

        Show
        Dan Poltawski added a comment - Hmm, thanks Simon. Yes, this is also conflicting on master, seemingly with old simpletest bits. So i'm not confident this applies on master. Reopening.
        Hide
        Dan Poltawski added a comment -

        There was 1 failure:

        1) completionlib_testcase::test_set_module_viewed
        Expectation failed for method name is equal to <string:get_data> when invoked at sequence index 1
        Parameter 1 for invocation completion_info::get_data(stdClass Object (...), false, 1337, null) does not match expected value.
        Failed asserting that false matches expected 1337.

        /Users/danp/git/integration/lib/completionlib.php:631
        /Users/danp/git/integration/lib/tests/completionlib_test.php:268
        /Users/danp/git/integration/lib/phpunit/classes/basic_testcase.php:64

        Show
        Dan Poltawski added a comment - There was 1 failure: 1) completionlib_testcase::test_set_module_viewed Expectation failed for method name is equal to <string:get_data> when invoked at sequence index 1 Parameter 1 for invocation completion_info::get_data(stdClass Object (...), false, 1337, null) does not match expected value. Failed asserting that false matches expected 1337. /Users/danp/git/integration/lib/completionlib.php:631 /Users/danp/git/integration/lib/tests/completionlib_test.php:268 /Users/danp/git/integration/lib/phpunit/classes/basic_testcase.php:64
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Aparup Banerjee added a comment -

        Thanks, test works now

        integrated into 22, 23 and master.

        ps: noted the $modinfo stdClass fix in 22 simpletest, thanks! it's better for history/searches if those kinds of fixes are in separate commits (if not issues).

        Show
        Aparup Banerjee added a comment - Thanks, test works now integrated into 22, 23 and master. ps: noted the $modinfo stdClass fix in 22 simpletest, thanks! it's better for history/searches if those kinds of fixes are in separate commits (if not issues).
        Hide
        Jason Fowler added a comment -

        all good! passing test

        Show
        Jason Fowler added a comment - all good! passing test
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I'm so proud...of you, many thanks!

        http://youtu.be/n64CdfDRnZY

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I'm so proud...of you, many thanks! http://youtu.be/n64CdfDRnZY Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: