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

admin/settings/plugins.php undefined variable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide

      1. To test this you need to install some third-party qtypes. Ideally install https://github.com/moodleou/moodle-qtype_pmatch (which uses an admin externalpage) and https://github.com/maths/moodle-qtype_stack (which uses a normal settings page). (You just need to install the STACK plugins. You don't need to install Maxima.)

      2. Log in as admin.

      3. Verify that you can see / access / use the manange qtypes and manage qbehaviours pages.

      4. Verify that you can see / access / use the settings page for qtype STACK and qtype pmatch.

      5. Log in as a manager, so you have moodle/question:config but but not moodle/site:config

      6. Verify that you CANNOT see / access / use the manange qtypes and manage qbehaviours pages.

      7. Verify that you still can see / access / use the settings page for qtype STACK and qtype pmatch.

      Show
      1. To test this you need to install some third-party qtypes. Ideally install https://github.com/moodleou/moodle-qtype_pmatch (which uses an admin externalpage) and https://github.com/maths/moodle-qtype_stack (which uses a normal settings page). (You just need to install the STACK plugins. You don't need to install Maxima.) 2. Log in as admin. 3. Verify that you can see / access / use the manange qtypes and manage qbehaviours pages. 4. Verify that you can see / access / use the settings page for qtype STACK and qtype pmatch. 5. Log in as a manager, so you have moodle/question:config but but not moodle/site:config 6. Verify that you CANNOT see / access / use the manange qtypes and manage qbehaviours pages. 7. Verify that you still can see / access / use the settings page for qtype STACK and qtype pmatch.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Bug introduced by MDL-35661

      Notice: Undefined variable: allplugins in /html/admin/settings/plugins.php on line 304
       
      Warning: Invalid argument supplied for foreach() in /html/admin/settings/plugins.php on line 304

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Marina, i've integrated this now

            Show
            poltawski Dan Poltawski added a comment - Thanks Marina, i've integrated this now
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Sorry Marina,

            I am not sure how a user can get to question types settings menu if she doesn't have site:config capability.

            Steps I followed:

            1. I assigned user as manager who has question:config capability but not site:config.
            2. Checked settings navigation
            3. plugins node is not visible, hence can't get to question types settings node (which is under plugins.)

            Let me know if I am missing something. Failing for now, to get attention.

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Sorry Marina, I am not sure how a user can get to question types settings menu if she doesn't have site:config capability. Steps I followed: I assigned user as manager who has question:config capability but not site:config. Checked settings navigation plugins node is not visible, hence can't get to question types settings node (which is under plugins.) Let me know if I am missing something. Failing for now, to get attention.
            Hide
            marina Marina Glancy added a comment -

            Tim, added you as a watcher.

            Hi Raj,
            I noticed that error appearing on qa.moodle.net if you log in as manager. I just saw the line in code that produces it and corrected it.

            But what you say is actually a very good point - the question types/behaviours navigation branch is unavailable anyway, because it is added to 'Plugins' branch which does not exist if user does not have site:config capability.

            So this seems to be completely different bug - question types branch should only be loaded if user has site:config capability. So basically the fix should be:

            --- a/admin/settings/plugins.php
            +++ b/admin/settings/plugins.php
            @@ -293,7 +293,7 @@ if ($hassiteconfig) {
             }
             
             // Question type settings
            -if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
            +if ($hassiteconfig && has_capability('moodle/question:config', $systemcontext)) {
                 // Question behaviour settings.
                 $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin')));
                 $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());

            Which if you think raises the question - why do we need a moodle/question:config capability at all? How is it possible that user has site:config capability but does not have moodle/question:config ? (This is a question to Tim)

            So I pushed a completely different fix for this issue. Up to integrators if they want to re-integrate it now or in the next round. This fix just removes a notice displayed to user.

            Show
            marina Marina Glancy added a comment - Tim, added you as a watcher. Hi Raj, I noticed that error appearing on qa.moodle.net if you log in as manager. I just saw the line in code that produces it and corrected it. But what you say is actually a very good point - the question types/behaviours navigation branch is unavailable anyway, because it is added to 'Plugins' branch which does not exist if user does not have site:config capability. So this seems to be completely different bug - question types branch should only be loaded if user has site:config capability. So basically the fix should be: --- a/admin/settings/plugins.php +++ b/admin/settings/plugins.php @@ -293,7 +293,7 @@ if ($hassiteconfig) { } // Question type settings -if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { +if ($hassiteconfig && has_capability('moodle/question:config', $systemcontext)) { // Question behaviour settings. $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin'))); $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours()); Which if you think raises the question - why do we need a moodle/question:config capability at all? How is it possible that user has site:config capability but does not have moodle/question:config ? (This is a question to Tim) So I pushed a completely different fix for this issue. Up to integrators if they want to re-integrate it now or in the next round. This fix just removes a notice displayed to user.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Marina,

            Can you please let me know how you got to see question type settings as manager. I just logged in on QA as manager and can't see plugin node under site administration.
            Not sure what I am missing here.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Marina, Can you please let me know how you got to see question type settings as manager. I just logged in on QA as manager and can't see plugin node under site administration. Not sure what I am missing here.
            Hide
            timhunt Tim Hunt added a comment -

            The use-case for moodle/question:config is that you may have staff who need to be able to configure questions, but who should not be trusted with moodle/site:config.

            The OU does have staff like this. Therefore ...

            -1 for the (most recent) proposed fix.

            The plugins category should be added to the admin tree if any of its child nodes are going to be there.

            Show
            timhunt Tim Hunt added a comment - The use-case for moodle/question:config is that you may have staff who need to be able to configure questions, but who should not be trusted with moodle/site:config. The OU does have staff like this. Therefore ... -1 for the (most recent) proposed fix. The plugins category should be added to the admin tree if any of its child nodes are going to be there.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim for explaining the issue.

            As per conversation on Dev chat, user with question:config and not site:config capability should:

            1. See link in "site administration"
            2. Able to access "manage question page" (which is currently not possible).

            Will create another issue, blocking this one as this can't be tested without user having access to "Manage question type" page.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim for explaining the issue. As per conversation on Dev chat, user with question:config and not site:config capability should: See link in "site administration" Able to access "manage question page" (which is currently not possible). Will create another issue, blocking this one as this can't be tested without user having access to "Manage question type" page.
            Hide
            poltawski Dan Poltawski added a comment -

            I'm going to revert this, because its not clear to me that this simple fix is actually caused by the underlying permissions tree issue.

            Show
            poltawski Dan Poltawski added a comment - I'm going to revert this, because its not clear to me that this simple fix is actually caused by the underlying permissions tree issue.
            Hide
            poltawski Dan Poltawski added a comment -

            OK reverted and reopening.

            Show
            poltawski Dan Poltawski added a comment - OK reverted and reopening.
            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
            timhunt Tim Hunt added a comment -

            I am looking at the code inside moodle.git/master. In admin/settings/plugins.php

            So, the bug is that the code

                require_once("$CFG->libdir/pluginlib.php");
                $allplugins = plugin_manager::instance()->get_plugins();

            is inside

            if ($hassiteconfig) {

            (which is good for performance for non-admins) but $allplugins is used inside both

            if ($hassiteconfig) {
            if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {

            Ah! Note that the code relating to local plugins has already been changed to allow for this. See the comment:

            // extend settings for each local plugin. Note that their settings may be in any part of the
            // settings tree and may be visible not only for administrators. We can not use $allplugins here

            So, there are two possible fixes:

            1. Move

                require_once("$CFG->libdir/pluginlib.php");
                $allplugins = plugin_manager::instance()->get_plugins();

            inside an

            if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {

            check.

            2. Change the qtype code to use get_plugin_list('qtype'), instead of using $allplugins there.

            I think that 1. is better. Do you want to make the fix, or shall I?

            Show
            timhunt Tim Hunt added a comment - I am looking at the code inside moodle.git/master. In admin/settings/plugins.php So, the bug is that the code require_once("$CFG->libdir/pluginlib.php"); $allplugins = plugin_manager::instance()->get_plugins(); is inside if ($hassiteconfig) { (which is good for performance for non-admins) but $allplugins is used inside both if ($hassiteconfig) { if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { Ah! Note that the code relating to local plugins has already been changed to allow for this. See the comment: // extend settings for each local plugin. Note that their settings may be in any part of the // settings tree and may be visible not only for administrators. We can not use $allplugins here So, there are two possible fixes: 1. Move require_once("$CFG->libdir/pluginlib.php"); $allplugins = plugin_manager::instance()->get_plugins(); inside an if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { check. 2. Change the qtype code to use get_plugin_list('qtype'), instead of using $allplugins there. I think that 1. is better. Do you want to make the fix, or shall I?
            Hide
            marina Marina Glancy added a comment - - edited

            Hi Tim,
            this was exactly what I did in my first version of fix. But Raj noticed that Question types and behaviours branches are not available anyway if user does not have site:config capability.
            The whole 'Plugins' branch ('modules') does not appear for user with manager role and I can't find why.

            Also if I try (as manager) to access http://localhost/admin/qtypes.php directly, I get:

            Access denied
             
            More information about this error
            Debug info:
            Error code: accessdenied
            Stack trace:
             
                line 467 of /lib/setuplib.php: moodle_exception thrown
                line 5995 of /lib/adminlib.php: call to print_error()
                line 40 of /admin/qtypes.php: call to admin_externalpage_setup()
             

            I don't know how it works for you in OU Tim.

            Show
            marina Marina Glancy added a comment - - edited Hi Tim, this was exactly what I did in my first version of fix. But Raj noticed that Question types and behaviours branches are not available anyway if user does not have site:config capability. The whole 'Plugins' branch ('modules') does not appear for user with manager role and I can't find why. Also if I try (as manager) to access http://localhost/admin/qtypes.php directly, I get: Access denied   More information about this error Debug info: Error code: accessdenied Stack trace:   line 467 of /lib/setuplib.php: moodle_exception thrown line 5995 of /lib/adminlib.php: call to print_error() line 40 of /admin/qtypes.php: call to admin_externalpage_setup()   I don't know how it works for you in OU Tim.
            Hide
            marina Marina Glancy added a comment -

            anyway, I'm actually on holidays. Not sure if I get online before Wednesday 14.11
            Feel free to take this issue from me. My previous version of fix:

            diff --git a/admin/settings/plugins.php b/admin/settings/plugins.php
            index 29d0665..3a56564 100644
            --- a/admin/settings/plugins.php
            +++ b/admin/settings/plugins.php
            @@ -294,6 +294,10 @@ if ($hassiteconfig) {
             
             // Question type settings
             if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) {
            +    if (!$hassiteconfig) {
            +        require_once("$CFG->libdir/pluginlib.php");
            +        $allplugins = plugin_manager::instance()->get_plugins();
            +    }
                 // Question behaviour settings.
                 $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin')));
                 $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());

            Show
            marina Marina Glancy added a comment - anyway, I'm actually on holidays. Not sure if I get online before Wednesday 14.11 Feel free to take this issue from me. My previous version of fix: diff --git a/admin/settings/plugins.php b/admin/settings/plugins.php index 29d0665..3a56564 100644 --- a/admin/settings/plugins.php +++ b/admin/settings/plugins.php @@ -294,6 +294,10 @@ if ($hassiteconfig) { // Question type settings if ($hassiteconfig || has_capability('moodle/question:config', $systemcontext)) { + if (!$hassiteconfig) { + require_once("$CFG->libdir/pluginlib.php"); + $allplugins = plugin_manager::instance()->get_plugins(); + } // Question behaviour settings. $ADMIN->add('modules', new admin_category('qbehavioursettings', new lang_string('questionbehaviours', 'admin'))); $ADMIN->add('qbehavioursettings', new admin_page_manageqbehaviours());
            Hide
            timhunt Tim Hunt added a comment -

            Ah, I think it is correct that admin_page_manageqtypes requires moodle/site:config. To reproduce this problem, you need a question type that has configuration options (none of the core ones do, yet). So, for example, install qtype_pmatch.

            Then, manager should be able to see Plugins -> question types -> Pmatch, but they cannot.

            The problem is with plugininfo_qtype::load_settings. That is incorrectly doing if ($hassiteconfig). That was introduced by MDL-35661.

            Show
            timhunt Tim Hunt added a comment - Ah, I think it is correct that admin_page_manageqtypes requires moodle/site:config. To reproduce this problem, you need a question type that has configuration options (none of the core ones do, yet). So, for example, install qtype_pmatch. Then, manager should be able to see Plugins -> question types -> Pmatch, but they cannot. The problem is with plugininfo_qtype::load_settings. That is incorrectly doing if ($hassiteconfig). That was introduced by MDL-35661 .
            Hide
            marina Marina Glancy added a comment -

            Submitting for integration.

            It's easier to fix it than argue with Tim.
            See github diff

            Show
            marina Marina Glancy added a comment - Submitting for integration. It's easier to fix it than argue with Tim. See github diff
            Hide
            marina Marina Glancy added a comment -

            Sorry Tim, did not see your comment before I submitted. Please feel free to take this issue from me or create a new one if you think that capability should be changed.

            Show
            marina Marina Glancy added a comment - Sorry Tim, did not see your comment before I submitted. Please feel free to take this issue from me or create a new one if you think that capability should be changed.
            Hide
            timhunt Tim Hunt added a comment -

            Yes. I'll take this and finish it off. Dear integrator, please could you reopen this? Thanks.

            Show
            timhunt Tim Hunt added a comment - Yes. I'll take this and finish it off. Dear integrator, please could you reopen this? Thanks.
            Hide
            poltawski Dan Poltawski added a comment -

            Done

            Show
            poltawski Dan Poltawski added a comment - Done
            Hide
            timhunt Tim Hunt added a comment -

            I think this is it. It works for me, for both admin and manager.

            Show
            timhunt Tim Hunt added a comment - I think this is it. It works for me, for both admin and manager.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Tim & Marina.

            I've integrated this now.

            Show
            poltawski Dan Poltawski added a comment - Thanks Tim & Marina. I've integrated this now.
            Hide
            dmonllao David Monllaó added a comment -

            It passes, I've replicated the problem without the patch and it works as expected with the patch. Tested with default manager role capabilities

            Show
            dmonllao David Monllaó added a comment - It passes, I've replicated the problem without the patch and it works as expected with the patch. Tested with default manager role capabilities
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Many, many thanks for your effort!

            Millions of people will enjoy the results of your work, yay!

            Closing as fixed. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  3/Dec/12