Moodle
  1. Moodle
  2. MDL-31360

regression: cannot set permissions on feedback instances

    Details

    • Rank:
      37877

      Description

      Crete a feedback on your 2.2 site - once created enter it and click the "permissions" link for this feedback instance - gets this error:
      Permissions in Feedback: feedback name

      Coding error detected, it must be fixed by a programmer: moodle_database::get_in_or_equal() does not accept empty arrays

      More information about this error
      Stack trace:

      line 595 of /lib/dml/moodle_database.php: coding_exception thrown
      line 6399 of /lib/accesslib.php: call to moodle_database->get_in_or_equal()
      line 7150 of /lib/accesslib.php: call to context_module->get_capabilities()
      line 69 of /admin/roles/lib.php: call to fetch_context_capabilities()
      line 261 of /admin/roles/lib.php: call to capability_table_base->__construct()
      line 191 of /admin/roles/permissions.php: call to permissions_table->__construct()

      looks to be related to the way accesslib uses modulename_get_extra_capabilities() - Feedback mod doesn't have this function so it uses an empty array() - which it doesn't like.

      It seems to me that acceslib shouldn't be relying on this function to exist - it should handle itself a bit better when this function doesn't exist (many 3rd party mods won't have this function either for example ouwiki - see CONTRIB-3360 for details_

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          this probably affects /mod/lti as well as it doesn't have the _get_extra_capabilities function either.

          Show
          Dan Marsden added a comment - this probably affects /mod/lti as well as it doesn't have the _get_extra_capabilities function either.
          Hide
          Sam Marshall added a comment -

          Agree, I think this should work if the function is missing. If the intention is that that function is now a required rather than optional API, then there should be a developer debug message to that effect (and it should be made to work anyway, at least for one version or so).

          Note: I think it is actually a bug in ouwiki that the function is not included. Because ouwiki supports groups it should at least list accessallgroups in there (I think). There may be other system capabilities that ouwiki uses too. I'm going to modify the dependent bug to mention that.

          Show
          Sam Marshall added a comment - Agree, I think this should work if the function is missing. If the intention is that that function is now a required rather than optional API, then there should be a developer debug message to that effect (and it should be made to work anyway, at least for one version or so). Note: I think it is actually a bug in ouwiki that the function is not included. Because ouwiki supports groups it should at least list accessallgroups in there (I think). There may be other system capabilities that ouwiki uses too. I'm going to modify the dependent bug to mention that.
          Hide
          Andrew Davis added a comment -

          Could you get rid of

          if (empty($extracaps)) {
              $extracaps = array();
          }

          and just have

          $extracaps = array();

          immediately about

          $modfile = "$CFG->dirroot/mod/$module->name/lib.php";

          so that $extracaps is initialized in the same way as $subcaps.

          A debug message if the _get_extra_capabilities() function doesn't exist seems like a good idea. This should all still work (dont throw an exception) but a debugging() message would be nice. I did some grepping and it looks like more or less all of the built in activity modules implement it.

          Show
          Andrew Davis added a comment - Could you get rid of if (empty($extracaps)) { $extracaps = array(); } and just have $extracaps = array(); immediately about $modfile = "$CFG->dirroot/mod/$module->name/lib.php" ; so that $extracaps is initialized in the same way as $subcaps. A debug message if the _get_extra_capabilities() function doesn't exist seems like a good idea. This should all still work (dont throw an exception) but a debugging() message would be nice. I did some grepping and it looks like more or less all of the built in activity modules implement it.
          Hide
          Dan Marsden added a comment -

          makes sense to tidy that up at the same time - sure.

          not sure if we need a debugging message for modules that don't have it - I can't see why a module "needs" to have it - especially if it doesn't use any other caps.

          Show
          Dan Marsden added a comment - makes sense to tidy that up at the same time - sure. not sure if we need a debugging message for modules that don't have it - I can't see why a module "needs" to have it - especially if it doesn't use any other caps.
          Hide
          Sam Marshall added a comment -

          Dan: My reason for suggesting the debugging message is that so far I know of two modules that were reported with this problem (ouwiki and feedback). In both cases these modules use capabilities outside their own mod/whatever:*, so they actually have a bug until the function is added, even if core is fixed to not require it.

          Agree a module doesn't specifically 'need' to have it but if this is the common pattern, I thought it might more likely protect against bugs if it does. I could be wrong.

          Show
          Sam Marshall added a comment - Dan: My reason for suggesting the debugging message is that so far I know of two modules that were reported with this problem (ouwiki and feedback). In both cases these modules use capabilities outside their own mod/whatever:*, so they actually have a bug until the function is added, even if core is fixed to not require it. Agree a module doesn't specifically 'need' to have it but if this is the common pattern, I thought it might more likely protect against bugs if it does. I could be wrong.
          Hide
          Aparup Banerjee added a comment -

          Hi all,
          I've had a look and the patch seems perfect to me for integration.

          However re: debugging message, i think aiming the debugging message with DEBUG_DEVELOPER in debugging() would potentially be useful info for developers only (also Andrew doesn't have to keep grepping :-D) . I understand it can be potentially annoying a message but all that an annoyed developer would have to do is just add in the dummy function. This would also lead towards less buggy or at least more wholesome modules.

          I'll leave this here for now for comment.

          cheers, Aparup

          Show
          Aparup Banerjee added a comment - Hi all, I've had a look and the patch seems perfect to me for integration. However re: debugging message, i think aiming the debugging message with DEBUG_DEVELOPER in debugging() would potentially be useful info for developers only (also Andrew doesn't have to keep grepping :-D) . I understand it can be potentially annoying a message but all that an annoyed developer would have to do is just add in the dummy function. This would also lead towards less buggy or at least more wholesome modules. I'll leave this here for now for comment. cheers, Aparup
          Hide
          Aparup Banerjee added a comment -

          reopening for further discussion just so we can decide on the changes to accesslib.php

          chatted with Eloy, if mandatory we'd need a debug mesage to notify devs so:
          we need to establish here if *_get_extra_capabilities() is mandatory for modules or not.

          Show
          Aparup Banerjee added a comment - reopening for further discussion just so we can decide on the changes to accesslib.php chatted with Eloy, if mandatory we'd need a debug mesage to notify devs so: we need to establish here if *_get_extra_capabilities() is mandatory for modules or not.
          Hide
          Dan Marsden added a comment -

          hmmm - I would suggest adding the debugging could be separate to this patch - and shouldn't hold it up from being integrated - esp as it is a regression and prevents it from working/fatal error in stable release.

          also wouldn't it make more sense to display this debugging notice on admin > plugins > activity modules - or during install/upgrade of the plugin? - a developer is unlikely to see a debug message on the permissions link unless they click on it during their dev which I'd guess wouldn't happen too often.

          Show
          Dan Marsden added a comment - hmmm - I would suggest adding the debugging could be separate to this patch - and shouldn't hold it up from being integrated - esp as it is a regression and prevents it from working/fatal error in stable release. also wouldn't it make more sense to display this debugging notice on admin > plugins > activity modules - or during install/upgrade of the plugin? - a developer is unlikely to see a debug message on the permissions link unless they click on it during their dev which I'd guess wouldn't happen too often.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm not really sure if we should consider xxxx_get_extra_capabilities() mandatory or no, but definitively deciding that with cause different tasks to be ignited:

          A) If the function is mandatory:

          • add it to all core modules (feedback only missing it afaik).
          • add it to the newmodule template
          • add it to some static code checker in MP database
          • document it in the modules plugin docs
          • add your patch, to handle modules returning empty array.

          B) If the function is not mandatory:

          • add your patch, to handle empty array.

          So it's just a matter of deciding between A and B. I've not special preference, but the truth is that all core modules have that function already (but the feedback one).

          Also, finally, about your patch, I think we introduced some months ago a 4th param to the get_in_or_equal() method to be able to specify how it should act with empty arrays, perhaps that would simplify your patch a bit (just guessing, haven't checked it).

          Surely the A/B list above implies that your patch is good enough for any of the 2 alternatives,
          if you can take a look to the get_in_or_equal() thingy...

          In the mean time I've asked about opinions for this @ HQ chat...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm not really sure if we should consider xxxx_get_extra_capabilities() mandatory or no, but definitively deciding that with cause different tasks to be ignited: A) If the function is mandatory: add it to all core modules (feedback only missing it afaik). add it to the newmodule template add it to some static code checker in MP database document it in the modules plugin docs add your patch, to handle modules returning empty array. B) If the function is not mandatory: add your patch, to handle empty array. So it's just a matter of deciding between A and B. I've not special preference, but the truth is that all core modules have that function already (but the feedback one). Also, finally, about your patch, I think we introduced some months ago a 4th param to the get_in_or_equal() method to be able to specify how it should act with empty arrays, perhaps that would simplify your patch a bit (just guessing, haven't checked it). Surely the A/B list above implies that your patch is good enough for any of the 2 alternatives, if you can take a look to the get_in_or_equal() thingy... In the mean time I've asked about opinions for this @ HQ chat... Ciao
          Hide
          Dan Marsden added a comment -

          last time I checked /mod/lti didn't have it either.

          Show
          Dan Marsden added a comment - last time I checked /mod/lti didn't have it either.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, I just did one quick grep and saw a lot, assuming they were all but feedback.

          So they are two, then. Let's see if people has something to say/argue and let's delay this for 12h. If you can take a look to the get_in_or_equal() thingy... thanks! See you tomorrow (NZ).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, I just did one quick grep and saw a lot, assuming they were all but feedback. So they are two, then. Let's see if people has something to say/argue and let's delay this for 12h. If you can take a look to the get_in_or_equal() thingy... thanks! See you tomorrow (NZ). Ciao
          Hide
          Dan Marsden added a comment -

          pushed new commit using 4th param - saves us one line compared to prev patch

          Show
          Dan Marsden added a comment - pushed new commit using 4th param - saves us one line compared to prev patch
          Hide
          Aparup Banerjee added a comment -

          cool, thats been integrated into 22 and master. decidedly, we've gone with (B) as 2 mods aren't using it.

          tested both and they're fine too.

          fwiw and ftr, mods using in master atm:
          mod has *_get_extra_capabilities()

          assignment /mod/assignment/lib.php:3844
          chat ./mod/chat/lib.php:1222
          choice /mod/choice/lib.php:772
          data /mod/data/lib.php:2667
          feedback
          folder /mod/folder/lib.php:55
          forum /mod/forum/lib.php:7342
          glossary /mod/glossary/lib.php:2674
          imscp /mod/imscp/lib.php:55
          label /mod/label/lib.php:186
          lesson /mod/lesson/lib.php:761
          lti
          page /mod/page/lib.php:53
          quiz /mod/quiz/lib.php:1522
          resource /mod/resource/lib.php:53
          scorm /mod/scorm/lib.php:800
          survey /mod/survey/lib.php:785
          url /mod/url/lib.php:55
          wiki /mod/wiki/lib.php:574
          workshop /mod/workshop/lib.php:1011

          Show
          Aparup Banerjee added a comment - cool, thats been integrated into 22 and master. decidedly, we've gone with (B) as 2 mods aren't using it. tested both and they're fine too. fwiw and ftr, mods using in master atm: mod has *_get_extra_capabilities() assignment /mod/assignment/lib.php:3844 chat ./mod/chat/lib.php:1222 choice /mod/choice/lib.php:772 data /mod/data/lib.php:2667 feedback folder /mod/folder/lib.php:55 forum /mod/forum/lib.php:7342 glossary /mod/glossary/lib.php:2674 imscp /mod/imscp/lib.php:55 label /mod/label/lib.php:186 lesson /mod/lesson/lib.php:761 lti page /mod/page/lib.php:53 quiz /mod/quiz/lib.php:1522 resource /mod/resource/lib.php:53 scorm /mod/scorm/lib.php:800 survey /mod/survey/lib.php:785 url /mod/url/lib.php:55 wiki /mod/wiki/lib.php:574 workshop /mod/workshop/lib.php:1011
          Hide
          Aparup Banerjee added a comment -

          marking as integrated

          Show
          Aparup Banerjee added a comment - marking as integrated
          Hide
          Aparup Banerjee added a comment -

          and both branches passed !

          (as well as error replicated before patch) - maybe that should be in some test guidelines.

          Show
          Aparup Banerjee added a comment - and both branches passed ! (as well as error replicated before patch) - maybe that should be in some test guidelines.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: