Moodle
  1. Moodle
  2. MDL-31010

Coding error detected in comments report

    Details

    • Testing Instructions:
      Hide

      This bug is a fringe case, hence it is important to replicate it without patch. Please do the following on a branch without the patch:-

      1. Create a Moodle instance with at least two separate courses.
      2. Add a comment block to an activity(Ex:- forum) within Course A.
      3. Add some comments.
      4. View the Comments Report.
      5. Add a comment block to an activity (Ex:- book) Course B.
      6. Add some comments.
      7. View the Comments Report (admin->reports->comment) make sure you see the error
      8. Apply the patch
      9. Refresh the comments report page and make sure no error is generated.
      Show
      This bug is a fringe case, hence it is important to replicate it without patch. Please do the following on a branch without the patch:- Create a Moodle instance with at least two separate courses. Add a comment block to an activity(Ex:- forum) within Course A. Add some comments. View the Comments Report. Add a comment block to an activity (Ex:- book) Course B. Add some comments. View the Comments Report (admin->reports->comment) make sure you see the error Apply the patch Refresh the comments report page and make sure no error is generated.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-31010-master
    • Rank:
      33467

      Description

      As found in http://moodle.org/comment/

      Coding error detected, it must be fixed by a programmer: Invalid component used in plugin_callback():mod_

      Whilst the QA testing site is not affected by this problem, I can't think of any reason why the problem should be specific to moodle.org and so I am creating a MDL issue. Feel free to change it to MDLSITE if I am wrong.

      1. MDL-31010.diff
        1 kB
        Sam Chaffee
      2. patch.diff
        1.0 kB
        Chris Wharton

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Helen.

          As I am not an admin on moodle.org, I was not able to access the page.

          I couldn't replicate this on my integration server, so I'd say it's just an issue with moodle.org.

          Show
          Michael de Raadt added a comment - Hi, Helen. As I am not an admin on moodle.org, I was not able to access the page. I couldn't replicate this on my integration server, so I'd say it's just an issue with moodle.org.
          Hide
          Helen Foster added a comment -

          Changing to MDL issue, as I notice http://demo.moodle.net/comment/ is showing the same error message.

          Show
          Helen Foster added a comment - Changing to MDL issue, as I notice http://demo.moodle.net/comment/ is showing the same error message.
          Hide
          David added a comment -

          Greetings! If this helps as far as confirmation...

          My school system's 2.2 returns the same message ("Coding error detected, it must be fixed by a programmer: Invalid component used in plugin_callback():mod_") from Site Administration > Reports > Comments

          Show
          David added a comment - Greetings! If this helps as far as confirmation... My school system's 2.2 returns the same message ("Coding error detected, it must be fixed by a programmer: Invalid component used in plugin_callback():mod_") from Site Administration > Reports > Comments
          Hide
          Helen Foster added a comment -

          Thanks David for your confirmation.

          Show
          Helen Foster added a comment - Thanks David for your confirmation.
          Hide
          Sean Keogh added a comment -

          Hi,

          This is affecting one of ouor custonmer sites too, on 2.2.2+. This problem was also reported in mdl-32024, but hte case was closed as unable to reproduce - well, it is happening on our customer site too, if you want to have a look.

          Show
          Sean Keogh added a comment - Hi, This is affecting one of ouor custonmer sites too, on 2.2.2+. This problem was also reported in mdl-32024, but hte case was closed as unable to reproduce - well, it is happening on our customer site too, if you want to have a look.
          Hide
          Helen Foster added a comment -

          Sean, thanks for pointing out a duplicate issue, which you'll notice is now linked to this issue.

          I've just logged in to http://demo.moodle.net as an admin and checked http://demo.moodle.net/comment/ to find everything working fine i.e. no error message. I can confirm that moodle.org is still affected by this bug.

          Show
          Helen Foster added a comment - Sean, thanks for pointing out a duplicate issue, which you'll notice is now linked to this issue. I've just logged in to http://demo.moodle.net as an admin and checked http://demo.moodle.net/comment/ to find everything working fine i.e. no error message. I can confirm that moodle.org is still affected by this bug.
          Hide
          Samuel Witzig added a comment -

          we are also affected (Moodle 2.2.2+, 2 weeks ago downloaded from moodle.org)

          Show
          Samuel Witzig added a comment - we are also affected (Moodle 2.2.2+, 2 weeks ago downloaded from moodle.org)
          Hide
          Samuel Witzig added a comment -

          we are also affected (Moodle 2.2.2+, 2 weeks ago downloaded from moodle.org)

          Show
          Samuel Witzig added a comment - we are also affected (Moodle 2.2.2+, 2 weeks ago downloaded from moodle.org)
          Hide
          Tina Schaefer added a comment - - edited

          I have just come across the following error: Coding error detected, it must be fixed by a programmer: Invalid component used in plugin_supports():enrol_

          I was trying to restore a course backup from 2.1.3 to 2.3.

          Show
          Tina Schaefer added a comment - - edited I have just come across the following error: Coding error detected, it must be fixed by a programmer: Invalid component used in plugin_supports():enrol_ I was trying to restore a course backup from 2.1.3 to 2.3.
          Hide
          Charles Fulton added a comment -

          I can donate a stack trace to this bug. The system is Moodle 2.2.3.

          Notice: Undefined index: 1530 in ./comment/locallib.php on line 130 Notice: Trying to get property of non-object in ./comment/locallib.php on line 130
          Coding error detected, it must be fixed by a programmer: Invalid component used in plugin/component_callback():mod_

          More information about this error

          Stack trace:
          line 7895 of /lib/moodlelib.php: coding_exception thrown
          line 7878 of /lib/moodlelib.php: call to component_callback()
          line 166 of /comment/locallib.php: call to plugin_callback()
          line 84 of /comment/index.php: call to comment_manager->print_comments()

          Show
          Charles Fulton added a comment - I can donate a stack trace to this bug. The system is Moodle 2.2.3. Notice: Undefined index: 1530 in ./comment/locallib.php on line 130 Notice: Trying to get property of non-object in ./comment/locallib.php on line 130 Coding error detected, it must be fixed by a programmer: Invalid component used in plugin/component_callback():mod_ More information about this error Stack trace: line 7895 of /lib/moodlelib.php: coding_exception thrown line 7878 of /lib/moodlelib.php: call to component_callback() line 166 of /comment/locallib.php: call to plugin_callback() line 84 of /comment/index.php: call to comment_manager->print_comments()
          Hide
          Charles Fulton added a comment -

          I can now reproduce this error on demand. Follow these steps:

          1. Verify that you can acces the comment reports and that it prints normally.
          2. Enter a course. Add a comments block to a forum (I tested with the news forum).
          3. Make a comment there.
          4. Go back to the comments report. It should now stack trace.

          For whatever reason $this->modinfo doesn't return what's expected and $this->pluginname isn't set properly. Changing $this->pluginname = $this->modinfo->cms[$this->cm->id]>modname to $this>pluginname = $this->cm->modname does resolve the stack trace but I don't know what downstream effects there are, if any.

          Show
          Charles Fulton added a comment - I can now reproduce this error on demand. Follow these steps: 1. Verify that you can acces the comment reports and that it prints normally. 2. Enter a course. Add a comments block to a forum (I tested with the news forum). 3. Make a comment there. 4. Go back to the comments report. It should now stack trace. For whatever reason $this->modinfo doesn't return what's expected and $this->pluginname isn't set properly. Changing $this->pluginname = $this->modinfo->cms [$this->cm->id] >modname to $this >pluginname = $this->cm->modname does resolve the stack trace but I don't know what downstream effects there are, if any.
          Hide
          Helen Foster added a comment -

          Great detective work, thanks Charles!

          Just noting that this remains a problem on moodle.org (using Moodle 2.3.1).

          Show
          Helen Foster added a comment - Great detective work, thanks Charles! Just noting that this remains a problem on moodle.org (using Moodle 2.3.1).
          Hide
          Chris Wharton added a comment - - edited

          We have clients reporting this issue as well. I get an identical stack trace to Charles. But I can't replicate the error on a fresh install for some reason.

          Show
          Chris Wharton added a comment - - edited We have clients reporting this issue as well. I get an identical stack trace to Charles. But I can't replicate the error on a fresh install for some reason.
          Hide
          Chris Wharton added a comment -

          I have attached a patch for a workaround to this issue.

          Show
          Chris Wharton added a comment - I have attached a patch for a workaround to this issue.
          Hide
          Charles Fulton added a comment -

          I'd like to partially retract my comment on how to reproduce this: I think the key is having comment blocks in two different courses, because setup_course() doesn't anticipate having multiple courses so any courses beyond the first aren't initialized properly. I admit even after reading through MDL-19118 I'm unclear on what the setup_course() code is meant to accomplish.

          Show
          Charles Fulton added a comment - I'd like to partially retract my comment on how to reproduce this: I think the key is having comment blocks in two different courses, because setup_course() doesn't anticipate having multiple courses so any courses beyond the first aren't initialized properly. I admit even after reading through MDL-19118 I'm unclear on what the setup_course() code is meant to accomplish.
          Hide
          Sam Chaffee added a comment -

          It looks like setup_course is only there to pass in to get_fast_modinfo, but like Charles mentions, it doesn't work for more than one course because it checks to see if the course property is already set.

          As far I can tell there is no other use for the get_fast_modinfo call (and subsequently the "modinfo" property) except to find the module name. I've search within the comment_manager class itself for any other uses and searched for client uses elsewhere and haven't found any.

          If it is the case that the modinfo property is only used to get the module name, then I think the setup_course method can be removed as well as the call to get_fast_modinfo. Instead, as in Chris' patch, the modname property of the cm object can be used.

          I'm attaching another patch for consideration. If anyone can comment on whether or not the course and modinfo properties of the comment_manager class are needed for something else either now or for planned functionality it would be greatly appreciated.

          Show
          Sam Chaffee added a comment - It looks like setup_course is only there to pass in to get_fast_modinfo, but like Charles mentions, it doesn't work for more than one course because it checks to see if the course property is already set. As far I can tell there is no other use for the get_fast_modinfo call (and subsequently the "modinfo" property) except to find the module name. I've search within the comment_manager class itself for any other uses and searched for client uses elsewhere and haven't found any. If it is the case that the modinfo property is only used to get the module name, then I think the setup_course method can be removed as well as the call to get_fast_modinfo. Instead, as in Chris' patch, the modname property of the cm object can be used. I'm attaching another patch for consideration. If anyone can comment on whether or not the course and modinfo properties of the comment_manager class are needed for something else either now or for planned functionality it would be greatly appreciated.
          Hide
          Charles Fulton added a comment -

          I've pushed Sam's patch into my repository. This approach makes sense to me; can someone from core please weigh in on what setup_course() is supposed to do?

          Show
          Charles Fulton added a comment - I've pushed Sam's patch into my repository. This approach makes sense to me; can someone from core please weigh in on what setup_course() is supposed to do?
          Hide
          Ankit Agarwal added a comment -

          Hi guys,
          This issue is in my next sprint's backlog. I will get back to this issue and your questions as soon as possible. In the mean time please feel free to continue the discussion.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi guys, This issue is in my next sprint's backlog. I will get back to this issue and your questions as soon as possible. In the mean time please feel free to continue the discussion. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks, this is now available in all the repos (git & cvs).

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks, this is now available in all the repos (git & cvs). Closing, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Doh,

          somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status!

          Ciao, Eloy

          Show
          Eloy Lafuente (stronk7) added a comment - Doh, somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status! Ciao, Eloy
          Hide
          Ankit Agarwal added a comment -

          Hi guys,
          Thanks for all your hardwork trying to replicate and fix this issue. Am sorry that I wasn't able to get to this earlier.
          This one was hard to replicate! Anyway these are the exact replication steps:-

          1. Goto a course activity (ex forum) and add a comments block
          2. Make some comments
          3. Goto another course activity (Ex book) in a different course and add a comments block
          4. Make some comments
          5. Access comment report from site admin>reports>comments

          The problem as explained before is once comment_manager::course is set for the first entry it is never updated. So for the next entries in a different course when we try to set pluginname, we are trying to access a context module instance which donot exist in the set comment_manager::course as it has not been updated, generating the error.

          As you guys mentioned I didnt find any other use of the $course variable, but am a little hesitating to remove the function call without knowing the exact usage. Also some contrib plugins might be using the $course variable.

          I suggest making the following changes to setup_course()

                  if (!empty($this->course) && $this->course->id == $courseid) {
                      // already set, stop
                      return;
                  }
          

          That is adding a extra check to see if the passed $courseid is same as the cached one before returning.
          This fixes the existing problem and has least chance of causing any other regression.

          If everyone is happy with this, I will go ahead with the process of patching it up.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi guys, Thanks for all your hardwork trying to replicate and fix this issue. Am sorry that I wasn't able to get to this earlier. This one was hard to replicate! Anyway these are the exact replication steps:- Goto a course activity (ex forum) and add a comments block Make some comments Goto another course activity (Ex book) in a different course and add a comments block Make some comments Access comment report from site admin>reports>comments The problem as explained before is once comment_manager::course is set for the first entry it is never updated. So for the next entries in a different course when we try to set pluginname, we are trying to access a context module instance which donot exist in the set comment_manager::course as it has not been updated, generating the error. As you guys mentioned I didnt find any other use of the $course variable, but am a little hesitating to remove the function call without knowing the exact usage. Also some contrib plugins might be using the $course variable. I suggest making the following changes to setup_course() if (!empty($ this ->course) && $ this ->course->id == $courseid) { // already set, stop return ; } That is adding a extra check to see if the passed $courseid is same as the cached one before returning. This fixes the existing problem and has least chance of causing any other regression. If everyone is happy with this, I will go ahead with the process of patching it up. Thanks
          Hide
          Ankit Agarwal added a comment -

          sending for peer-review.
          Thanks

          Show
          Ankit Agarwal added a comment - sending for peer-review. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Ankit,

          I reviewed and tested the patch in 2.2, 2.3 and 2.4. It looks great and works as expected.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Ankit, I reviewed and tested the patch in 2.2, 2.3 and 2.4. It looks great and works as expected. [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check
          Hide
          Ankit Agarwal added a comment -

          Thanks for the review Rosie.
          Sending for integration

          Show
          Ankit Agarwal added a comment - Thanks for the review Rosie. Sending for integration
          Hide
          Aparup Banerjee added a comment -

          Hi Ankit,
          this patch seems to have come about from a lot of effort in isolating this

          • consider adding some tests (first ones) to the comments api ?
          Show
          Aparup Banerjee added a comment - Hi Ankit, this patch seems to have come about from a lot of effort in isolating this consider adding some tests (first ones) to the comments api ?
          Hide
          Ankit Agarwal added a comment -

          Hi Aparup,
          Am not sure if we can add unit test to test private methods. obviously we can design workarounds to force test things, but am not sure if its worth it.
          I am creating a new issue to look into the possibility of adding unit tests to the comments api as you suggested.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Aparup, Am not sure if we can add unit test to test private methods. obviously we can design workarounds to force test things, but am not sure if its worth it. I am creating a new issue to look into the possibility of adding unit tests to the comments api as you suggested. Thanks
          Hide
          Aparup Banerjee added a comment -

          Thanks Ankit, thats been integrated into 22, 23 and master.

          Perhaps in a new issue you could consider adding some clean up to this comment_manager class. As noted in our chat about the class's lack of properties etc.

          Show
          Aparup Banerjee added a comment - Thanks Ankit, thats been integrated into 22, 23 and master. Perhaps in a new issue you could consider adding some clean up to this comment_manager class. As noted in our chat about the class's lack of properties etc.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Error replicated before updating. Report functions properly after update.

          Show
          Michael de Raadt added a comment - Test result: Success! Error replicated before updating. Report functions properly after update.
          Hide
          Aparup Banerjee added a comment -

          Your issue has dug up some gold.
          It works great i've been told.
          Go forth, be brave, be bold.

          yay! "All your thoughts are belong to everyone."

          Thanks and ciao!

          Show
          Aparup Banerjee added a comment - Your issue has dug up some gold. It works great i've been told. Go forth, be brave, be bold. yay! "All your thoughts are belong to everyone." Thanks and ciao!
          Hide
          Ashley Holman added a comment -

          This error still happens for me. Steps to reproduce:

          • Add a wiki activity to a course and add a comment to it
          • Disable the wiki module (click the eye to hide it on the Manage Activities admin page).
          • View the comments report. The error is presented.

          "Coding error detected, it must be fixed by a programmer: Invalid component used in plugin/component_callback():mod_"

          Show
          Ashley Holman added a comment - This error still happens for me. Steps to reproduce: Add a wiki activity to a course and add a comment to it Disable the wiki module (click the eye to hide it on the Manage Activities admin page). View the comments report. The error is presented. "Coding error detected, it must be fixed by a programmer: Invalid component used in plugin/component_callback():mod_"
          Hide
          Ashley Holman added a comment -

          Above test was done in 2.3. How do I re-open this issue or should I create a new one?

          Show
          Ashley Holman added a comment - Above test was done in 2.3. How do I re-open this issue or should I create a new one?
          Hide
          Aparup Banerjee added a comment -

          Hi Ashley, please create a new one and link to this. Do mention if its only for wiki.

          Show
          Aparup Banerjee added a comment - Hi Ashley, please create a new one and link to this. Do mention if its only for wiki.
          Hide
          Ashley Holman added a comment -

          Reproduced with database module as well. Will create a new issue. Thanks Aparup.

          Show
          Ashley Holman added a comment - Reproduced with database module as well. Will create a new issue. Thanks Aparup.

            People

            • Votes:
              18 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: