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

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

    Details

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

      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.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            skodak Petr Skoda added a comment -

            thanks for the report

            Show
            skodak Petr Skoda added a comment - thanks for the report
            Hide
            sry_not4sale 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
            sry_not4sale 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
            dmonllao 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
            dmonllao 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
            sry_not4sale Aaron Barnes added a comment -

            Thanks David!

            Show
            sry_not4sale Aaron Barnes added a comment - Thanks David!
            Hide
            nebgor 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
            nebgor 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Hi, Could we have some testing instructions for this issue?
            Hide
            simoncoggins 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
            simoncoggins 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            nebgor 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
            nebgor 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
            phalacee Jason Fowler added a comment -

            all good! passing test

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

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

            http://youtu.be/n64CdfDRnZY

            Closing as fixed, ciao

            Show
            stronk7 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:
                  Fix Release Date:
                  10/Sep/12