Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.1
    • Component/s: Administration
    • Labels:
    • Rank:
      6482

      Description

      I had need to create a module multi selection admin page so looked to the Site administration > Security > Module security page as a starting point.

      Because I was looking, it seems that the database hit on line 95 of admin/settings/security.php could be avoided for most pages if the lazy loading feature of the base class admin_setting_configselect was used.

      I suggest that the admin_setting_multiconfigselect class is extended to create a new admin_setting_multiconfigselect_modules class where the load_choices function may be overwritten with the database hit. This will improve performance for many pages as it is one less database hit, and it would also provides me with a class that I may re-use.

      Suggested code:
      lib/adminlib.php:
      /**

      • Multiselect for current modules
        *
      • @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
        */
        class admin_setting_configmultiselect_modules extends admin_setting_configmultiselect {
        /**
      • Calls parent::__construct - note array $choices is not required
        */
        public function __construct($name, $visiblename, $description) { parent::__construct($name, $visiblename, $description, array(), null); }

      /**

      • Loads an array of current module choices
        *
      • @return bool always return true
        */
        public function load_choices() {
        if (is_array($this->choices)) { return true; }

        $this->choices = array();

      global $CFG, $DB;
      $records = $DB->get_records('modules', array('visible'=>1), 'name');
      foreach ($records as $record) {
      if (file_exists("$CFG->dirroot/mod/$record->name/lib.php"))

      { $this->choices[$record->id] = $record->name; }

      }
      return true;
      }
      }

      admin/settings/security.php:
      remove lines 95 to 101 and change line 102 to:
      $temp->add(new admin_setting_configmultiselect('defaultallowedmodules', get_string('defaultallowedmodules', 'admin'), get_string('configdefaultallowedmodules', 'admin')));

      Please let me know if this sounds reasonable, or if there are any objections.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          John, is your suggested code in git anywhere?

          Show
          Tim Hunt added a comment - John, is your suggested code in git anywhere?
          Hide
          Tim Hunt added a comment -

          Petr, I am taking the liberty of assigning this to you, because I know you are working on performance things in 2.0, and there is some low-hanging fruit here.

          As well as the issue John found and fixed, I suggest you search for '$DB' in all settings.php files. I find 4 DB queries that are being done on every page for admins, and which can be removed by using the load_choices thing.

          Show
          Tim Hunt added a comment - Petr, I am taking the liberty of assigning this to you, because I know you are working on performance things in 2.0, and there is some low-hanging fruit here. As well as the issue John found and fixed, I suggest you search for '$DB' in all settings.php files. I find 4 DB queries that are being done on every page for admins, and which can be removed by using the load_choices thing.
          Hide
          Tim Hunt added a comment -

          I just did a pull request for John's fix. I will make a new subtask of MDL-23754 for reviewing other similar problems in settings files.

          Show
          Tim Hunt added a comment - I just did a pull request for John's fix. I will make a new subtask of MDL-23754 for reviewing other similar problems in settings files.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: