Moodle
  1. Moodle
  2. MDL-35661

Plugins that only use settings.php to add items to another section can cause an error

    Details

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

      In 2.3 and master:
      install some contributed local plugin and make sure that plugins overview page (xxx/admin/plugins.php) does not show the broken 'settings' link.

      In master only:
      1. Make sure that Plugins section of Site Administration has working 'settings' and 'uninstall' links.

      Note: New functionality does not affect any core plugins.

      2. Install some contributed plugins and make sure that settings, visibility icon and uninstall link are similar to overview page of this particular plugin type.
      Changed interfaces: webservices, plagiarism, editors, message protocols.

      3. Try altering some plugin's settings.php file by adding $settings=null; in the end. Expected behaviour: the settings item is removed from navigation menu and overview page for particular plugin type. After this fix the link is also removed from general plugin overview page.

      Show
      In 2.3 and master: install some contributed local plugin and make sure that plugins overview page (xxx/admin/plugins.php) does not show the broken 'settings' link. In master only: 1. Make sure that Plugins section of Site Administration has working 'settings' and 'uninstall' links. Note: New functionality does not affect any core plugins. 2. Install some contributed plugins and make sure that settings, visibility icon and uninstall link are similar to overview page of this particular plugin type. Changed interfaces: webservices, plagiarism, editors, message protocols. 3. Try altering some plugin's settings.php file by adding $settings=null; in the end. Expected behaviour: the settings item is removed from navigation menu and overview page for particular plugin type. After this fix the link is also removed from general plugin overview page.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-35661-master

      Description

      See MDL-35442:

      1. install the adminer contrib plugin to local
      2. Click on the settings link in the plugins overview page

      This is probably the case for other plugin types settings links too.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ruslan Kabalin added a comment - - edited

            As Marina suggested here, get_settings_url() probably needs to be removed from plugininfo_local.

            Show
            Ruslan Kabalin added a comment - - edited As Marina suggested here , get_settings_url() probably needs to be removed from plugininfo_local.
            Hide
            Marina Glancy added a comment -

            the base problem here is that plugininfo_base::get_settings_url() should check the existence of the item in admin tree before returning the link to it.
            But at the same time the logic on how to generate the setting url, when it works and when not, how to include it in the page is duplicated in several places in code:
            1. on "plugin overview" page using plugin_manager and plugininfo_base classes
            2. in admin/settings/plugins.php where the admit settings tree is generated
            3. in overview pages for particular plugins (different places for each plugin type)

            with the introduction of plugin_manager it really asks to use plugin_manager to deal with settings links/loading in all the places.

            At the moment I moved the functionality to load settings to proper classes extending plugininfo_base and use them in admin/settings/plugins.php

            But still #3 (plugin-specific overview pages) are not changed and it will take a long time to refactor them.

            Show
            Marina Glancy added a comment - the base problem here is that plugininfo_base::get_settings_url() should check the existence of the item in admin tree before returning the link to it. But at the same time the logic on how to generate the setting url, when it works and when not, how to include it in the page is duplicated in several places in code: 1. on "plugin overview" page using plugin_manager and plugininfo_base classes 2. in admin/settings/plugins.php where the admit settings tree is generated 3. in overview pages for particular plugins (different places for each plugin type) with the introduction of plugin_manager it really asks to use plugin_manager to deal with settings links/loading in all the places. At the moment I moved the functionality to load settings to proper classes extending plugininfo_base and use them in admin/settings/plugins.php But still #3 (plugin-specific overview pages) are not changed and it will take a long time to refactor them.
            Hide
            Marina Glancy added a comment -

            David, can you please look at this

            Show
            Marina Glancy added a comment - David, can you please look at this
            Hide
            Marina Glancy added a comment -

            Although I did not change the loading of settings for plugins of the types:

            • local
            • report
            • tool
              because they are loaded regardless of user's capability site:config, since we don't want to load plugin_manager for every user

            Also webservices did not check this capability and seems that they should have. Corrected that (to be confirmed by Jerome)

            Show
            Marina Glancy added a comment - Although I did not change the loading of settings for plugins of the types: local report tool because they are loaded regardless of user's capability site:config, since we don't want to load plugin_manager for every user Also webservices did not check this capability and seems that they should have. Corrected that (to be confirmed by Jerome)
            Hide
            Jérôme Mouneyrac added a comment -

            To confirm Marina's chat questions about the ws settings:

            Show
            Jérôme Mouneyrac added a comment - To confirm Marina's chat questions about the ws settings: in admin/settings/plugins.php, the entire web service settings code should have been under "if ($hassiteconfig) {}", as Marina mentioned to me. the plugin settings class look good to me ( https://github.com/marinaglancy/moodle/commit/95d0e669ae57b0f3157e058f91fc2f7cf658f508 ), even thought in reality none of the current web service plugins have any setting. So even the current code is not really used.
            Hide
            Marina Glancy added a comment -

            For 2.3 I have just fixed the local plugins settings link

            Show
            Marina Glancy added a comment - For 2.3 I have just fixed the local plugins settings link
            Hide
            Marina Glancy added a comment -

            David, ping

            Show
            Marina Glancy added a comment - David, ping
            Hide
            David Mudrak added a comment -

            Hi Marina. Well done. Just some tiny details that caught my sleepy eye:

            https://github.com/marinaglancy/moodle/compare/master...wip-MDL-35661-master#L3R1770 - Ff the method's doc block declares "@return string" and it returns null by default, the docblock should explain in what circumstances the null is returned. Ideally, the return statement might look like "@return string|null" even.

            https://github.com/marinaglancy/moodle/compare/master...wip-MDL-35661-master#L3R1804 - The method signature

            public function load_settings($ADMIN, $parentnodename, $hassiteconfig)
            

            would IMHO better look with something like

            public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig)
            

            (and no - "This way I can simply copy and paste the code" is not a good argument )
            Also note that the order of docblock's @params should respect the order of parameters in the method signature.

            Otherwise +1 for landing. Thanks Marina.

            Show
            David Mudrak added a comment - Hi Marina. Well done. Just some tiny details that caught my sleepy eye: https://github.com/marinaglancy/moodle/compare/master...wip-MDL-35661-master#L3R1770 - Ff the method's doc block declares "@return string" and it returns null by default, the docblock should explain in what circumstances the null is returned. Ideally, the return statement might look like "@return string|null" even. https://github.com/marinaglancy/moodle/compare/master...wip-MDL-35661-master#L3R1804 - The method signature public function load_settings($ADMIN, $parentnodename, $hassiteconfig) would IMHO better look with something like public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig) (and no - "This way I can simply copy and paste the code" is not a good argument ) Also note that the order of docblock's @params should respect the order of parameters in the method signature. Otherwise +1 for landing. Thanks Marina.
            Hide
            Marina Glancy added a comment -

            Thanks, David
            I'll correct @return string
            regarding $ADMIN I can add class name 'admin_root' there but can not rename. This method includes settings.php and inside the settings.php developers refer to $ADMIN very often

            Show
            Marina Glancy added a comment - Thanks, David I'll correct @return string regarding $ADMIN I can add class name 'admin_root' there but can not rename. This method includes settings.php and inside the settings.php developers refer to $ADMIN very often
            Hide
            David Mudrak added a comment -

            Right. Still, I would suggest to use explicit

            public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig) {
                global $ADMIN;
                $ADMIN = $adminroot;
                // cont.
            

            to make the method signature clear off any implicit globals. But this should be consulted with some wise men to check their opinion.

            Show
            David Mudrak added a comment - Right. Still, I would suggest to use explicit public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig) { global $ADMIN; $ADMIN = $adminroot; // cont. to make the method signature clear off any implicit globals. But this should be consulted with some wise men to check their opinion.
            Hide
            Marina Glancy added a comment -

            David, that's scary - assigning something to the global variable.
            I made the changes and wrote commit messages

            Show
            Marina Glancy added a comment - David, that's scary - assigning something to the global variable. I made the changes and wrote commit messages
            Hide
            Marina Glancy added a comment - - edited

            I submitted this for integration.

            There should be some announcement for plugin developers but I don't know where to insert it:

            There are changes on how the settings.php file is included. After upgrading to 2.4 make sure that settings for your contributed plugin work as expected

            Show
            Marina Glancy added a comment - - edited I submitted this for integration. There should be some announcement for plugin developers but I don't know where to insert it: There are changes on how the settings.php file is included. After upgrading to 2.4 make sure that settings for your contributed plugin work as expected
            Hide
            David Mudrak added a comment -

            > that's scary - assigning something to the global variable.

            Right. I did not realize that we actually do not need the global in this case, do we? Just the plain

            public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig) {
             
                $ADMIN = $adminroot;
             
                // include(blah blah blah path to the settings.php);
            

            should work, right? And it keeps the signature nice and does not need any change in settings.php files.

            Show
            David Mudrak added a comment - > that's scary - assigning something to the global variable. Right. I did not realize that we actually do not need the global in this case, do we? Just the plain public function load_settings(admin_root $adminroot, $parentnodename, $hassiteconfig) {   $ADMIN = $adminroot;   // include(blah blah blah path to the settings.php); should work, right? And it keeps the signature nice and does not need any change in settings.php files.
            Hide
            Marina Glancy added a comment -

            ok, that's some rebase work. Will do tomorrow morning

            Show
            Marina Glancy added a comment - ok, that's some rebase work. Will do tomorrow morning
            Hide
            Dan Poltawski added a comment -

            Added api_change label, for this to be noted in release notes.

            Show
            Dan Poltawski added a comment - Added api_change label, for this to be noted in release notes.
            Hide
            Dan Poltawski added a comment -

            Thanks Marina, i've integrated this now.

            Show
            Dan Poltawski added a comment - Thanks Marina, i've integrated this now.
            Hide
            Dan Poltawski added a comment -

            Looks like this is breaking CLI install

            Moodle 2.4dev (Build: 20121015) command line installation program
            -->System
            Default exception handler: Table "glossary_formats" does not exist Debug:
            Error code: ddltablenotexist

            • line 519 of /lib/dml/moodle_database.php: dml_exception thrown
            • line 1310 of /lib/dml/moodle_database.php: call to moodle_database->where_clause()
            • line 919 of /mod/glossary/lib.php: call to moodle_database->get_record()
            • line 52 of /mod/glossary/settings.php: call to glossary_get_available_formats()
            • line 2330 of /lib/pluginlib.php: call to include()
            • line 17 of /admin/settings/plugins.php: call to plugininfo_mod->load_settings()
            • line 6087 of /lib/adminlib.php: call to require()
            • line 6107 of /lib/adminlib.php: call to admin_get_root()
            • line 1442 of /lib/upgradelib.php: call to admin_apply_default_settings()
            • line 454 of /lib/installlib.php: call to install_core()
            • line 677 of /admin/cli/install.php: call to install_cli_database()

            !!! Table "glossary_formats" does not exist !!!
            !!
            Error code: ddltablenotexist !!
            !! Stack trace: * line 519 of /lib/dml/moodle_database.php: dml_exception thrown

            • line 1310 of /lib/dml/moodle_database.php: call to moodle_database->where_clause()
            • line 919 of /mod/glossary/lib.php: call to moodle_database->get_record()
            • line 52 of /mod/glossary/settings.php: call to glossary_get_available_formats()
            • line 2330 of /lib/pluginlib.php: call to include()
            • line 17 of /admin/settings/plugins.php: call to plugininfo_mod->load_settings()
            • line 6087 of /lib/adminlib.php: call to require()
            • line 6107 of /lib/adminlib.php: call to admin_get_root()
            • line 1442 of /lib/upgradelib.php: call to admin_apply_default_settings()
            • line 454 of /lib/installlib.php: call to install_core()
            • line 677 of /admin/cli/install.php: call to install_cli_database()
              !!
            Show
            Dan Poltawski added a comment - Looks like this is breaking CLI install Moodle 2.4dev (Build: 20121015) command line installation program -->System Default exception handler: Table "glossary_formats" does not exist Debug: Error code: ddltablenotexist line 519 of /lib/dml/moodle_database.php: dml_exception thrown line 1310 of /lib/dml/moodle_database.php: call to moodle_database->where_clause() line 919 of /mod/glossary/lib.php: call to moodle_database->get_record() line 52 of /mod/glossary/settings.php: call to glossary_get_available_formats() line 2330 of /lib/pluginlib.php: call to include() line 17 of /admin/settings/plugins.php: call to plugininfo_mod->load_settings() line 6087 of /lib/adminlib.php: call to require() line 6107 of /lib/adminlib.php: call to admin_get_root() line 1442 of /lib/upgradelib.php: call to admin_apply_default_settings() line 454 of /lib/installlib.php: call to install_core() line 677 of /admin/cli/install.php: call to install_cli_database() !!! Table "glossary_formats" does not exist !!! !! Error code: ddltablenotexist !! !! Stack trace: * line 519 of /lib/dml/moodle_database.php: dml_exception thrown line 1310 of /lib/dml/moodle_database.php: call to moodle_database->where_clause() line 919 of /mod/glossary/lib.php: call to moodle_database->get_record() line 52 of /mod/glossary/settings.php: call to glossary_get_available_formats() line 2330 of /lib/pluginlib.php: call to include() line 17 of /admin/settings/plugins.php: call to plugininfo_mod->load_settings() line 6087 of /lib/adminlib.php: call to require() line 6107 of /lib/adminlib.php: call to admin_get_root() line 1442 of /lib/upgradelib.php: call to admin_apply_default_settings() line 454 of /lib/installlib.php: call to install_core() line 677 of /admin/cli/install.php: call to install_cli_database() !!
            Hide
            Marina Glancy added a comment - - edited

            I added a commit checking if module or filter is installed before including settings.php
            For other plugin types the check was either already present or it was not present before this issue as well.

            Therefore there should be no regression.

            Although if some other plugin type accesses not-yet existing DB tables in settings.php it can break installation. Maybe it's worth creating an issue

            Show
            Marina Glancy added a comment - - edited I added a commit checking if module or filter is installed before including settings.php For other plugin types the check was either already present or it was not present before this issue as well. Therefore there should be no regression. Although if some other plugin type accesses not-yet existing DB tables in settings.php it can break installation. Maybe it's worth creating an issue
            Hide
            Dan Poltawski added a comment -

            I've integrated the fix for that, thanks Marina.

            Show
            Dan Poltawski added a comment - I've integrated the fix for that, thanks Marina.
            Hide
            Andrew Davis added a comment -

            The testing instructions are a little hard to follow but I think this is all working as it should. Passing.

            Show
            Andrew Davis added a comment - The testing instructions are a little hard to follow but I think this is all working as it should. Passing.
            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!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: