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

Admin presets treat settings as incorrect types

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Critical Critical
    • None
    • 3.11.15, 4.0.9, 4.1.4, 4.2.1
    • Administration
    • None
    • MOODLE_311_STABLE, MOODLE_400_STABLE, MOODLE_401_STABLE, MOODLE_402_STABLE

       The adminpresets subsystem incorrectly maps admin settings to preset handlers.

      When fetching the admin preset for a known setting, for example the bigbluebuttonbn_dpa_info, the \core_adminpresets\manager class a attempts to load an appropriate adminpreset wrapper for that setting. It does so by calling get_setting which normalises the setting to some extent (to account for namespaced vs. non-namespaced setting classes), and then looks for an exact match by prefixing the normalised name with "\core_adminpresets\local\setting\adminpresets_".

      In the dpa_info example, this would be \core_adminpresets\local\setting\adminpresets_admin_setting_description.

      If a match is not found it prefixes the class name with "adminpresets_", and then attempts to find this in a mapping array.

      In the case of an admin_setting_description this mapping is set to adminpresets_admin_setting_configtext.
      Failing this it falls back to adminpresets_setting.

      There are several issues with this approach:

      1. admin_setting_desription is not a child of admin_setting_configtext. They have some different properties
      2. if a plugin extends admin_setting_description it will not be found by the mapping array

      This is a potentially serious problem and is currently hidden by dynamic properties. We are already attempting to access the paramtype value of the setting, which only exists for configtext and its children.
      If we were to correctly fix dynamic properties, then this would leave a whole load of deprecation notices for PHP 8.2.

      It's also a potential issue because settings may be treated as text and validation removed.

      This code really should:

      • use reflection rather than dodgy string manipulation
      • fetch parent class information to resolve values
      • not maintain a fixed map as this is fragile

      The other related issue which needs to be fixed is that the normalisation of the settings leads to incorrect preset type identification.

      If I create my own admin setting in a namespace, such as mod_example\admin\admin_setting_configduration this could actually extend admin_setting_configselect - perfectly legitimately.

      With the way in which class discovery works, it will just take the admin_setting_configduration section of the name, and return the preset type associated with that value.

      This code really needs to rethink how it maps admin settings to presets.

      On way in which this could be done is to instead ask each admin preset whether it handles the provided type. For example:

      // Presets must be ordered from most-specific to least - that is a type whicih extends configtext must come before configtext, whucih must come before the base class.
      $handlers = self::get_ordered_handlers();
      foreach ($handlers as $handler) {
          if ($handler::handles($settingdata)) {
              return new $handler($settingdata, $curretvalue)
          }
      }
       
      // Not found. Throw exception.
      

      The handles method may be as simple as:

      public static function handlers(admin_setting $setting): bool {
          if ($setting instanceof \admin_setting_configtextarea) {
              return true;
          }
          return false;
      

      The tricky part here will be getting the list of ordered handlers correctly.

      In any case, we may need to create some new preset handler types to make this work properly.

            Unassigned Unassigned
            dobedobedoh Andrew Lyons
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:

                Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.