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

          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