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
    • Rank:
      44398

      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.

        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: