Moodle
  1. Moodle
  2. MDL-35434

Add File Picker to Admin settings in Themes

    Details

    • Testing Instructions:
      Hide

      Testing difficulty (hard)

      Note for tester - this patch was integrated without the changes to the after burner theme (it will be used by the simple theme only).

      Before testing you must cherry-pick one extra commit from Petrs branch in order to install the required changes to afterburner:

      git fetch git://github.com/skodak/moodle.git w15_MDL-35434_m25_storedfilesetting
      git cherry-pick cf32a715cca877c7580c5e4a4352e2cd7499a7e1

      Alternatively use the Simple theme, if visible in the Theme selector at the Testing stage.

      1. Select Afterburner theme from Theme selector and NOT by URL (this is important).
      2. Enable Theme Designer Mode in Theme settings.
      3. Navigate to Site Administration > Appearance > Themes > and locate the Afterburner custom settings page from the list of theme names listed.
      4. TEST the new file-picker setting by uploading the small Moodle logo from the formal_white theme found at theme/formal_white/pix/logo_small.png
      5. Save settings and refresh the page or navigate to Home page.
      6. TEST that the Moodle logo has replaced the Afterburner logo.
      7. End test.
      Show
      Testing difficulty (hard) Note for tester - this patch was integrated without the changes to the after burner theme (it will be used by the simple theme only). Before testing you must cherry-pick one extra commit from Petrs branch in order to install the required changes to afterburner: git fetch git://github.com/skodak/moodle.git w15_ MDL-35434 _m25_storedfilesetting git cherry-pick cf32a715cca877c7580c5e4a4352e2cd7499a7e1 Alternatively use the Simple theme, if visible in the Theme selector at the Testing stage. Select Afterburner theme from Theme selector and NOT by URL (this is important). Enable Theme Designer Mode in Theme settings. Navigate to Site Administration > Appearance > Themes > and locate the Afterburner custom settings page from the list of theme names listed. TEST the new file-picker setting by uploading the small Moodle logo from the formal_white theme found at theme/formal_white/pix/logo_small.png Save settings and refresh the page or navigate to Home page. TEST that the Moodle logo has replaced the Afterburner logo. End test.
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w15_MDL-35434_m25_storedfilesetting
    • Rank:
      44115

      Description

      Most Moodle themes have settings pages, mainly to add a logo or CSS in order to customise the theme to give it a corporate look.

      However, the absence of a File Picker makes the job of adding a logo, or background image, harder as the image needs to be stored somewhere prior to obtaining the URL so that it can be added to the settings page.

      Having a File Picker would simplify this task and make for better theme customisation.

      1. admin_file_picker_adminlib.php
        6 kB
        Pau Ferrer Ocaña (crazyserver)
      2. admin_file_picker_filelib.php
        0.3 kB
        Pau Ferrer Ocaña (crazyserver)
      1. mdl_35434_fm.png
        39 kB
      2. storedfile.png
        75 kB

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          Adding you as watcher so that you can vote for this new feature.
          Thanks
          Mary

          Show
          Mary Evans added a comment - Adding you as watcher so that you can vote for this new feature. Thanks Mary
          Hide
          Daniel Wahl added a comment - - edited

          This is labeled as fixed in 2.4 but I don't think that's correct - sorry I just saw that it's for 2.4.1, which is 24_STABLE

          Show
          Daniel Wahl added a comment - - edited This is labeled as fixed in 2.4 but I don't think that's correct - sorry I just saw that it's for 2.4.1, which is 24_STABLE
          Hide
          Mary Evans added a comment -

          Hi Danny,

          This is not FIXED yet, but when it does it will go into Master branch which is currenlty 2.5 and also go into 2.4.1.

          As for this idea of a Filepicker in Admin/custom settings page in Themes, we need someone to come up with a suggestion for how to do this. Feel free to add a patch if you know how it can be done?

          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi Danny, This is not FIXED yet, but when it does it will go into Master branch which is currenlty 2.5 and also go into 2.4.1. As for this idea of a Filepicker in Admin/custom settings page in Themes, we need someone to come up with a suggestion for how to do this. Feel free to add a patch if you know how it can be done? Cheers Mary
          Hide
          Mary Evans added a comment -

          Just added you as a watch here Gareth. Please feel free to ask for direction with this if you feel you can find a solution for this much need improvement!

          Cheers
          Mary

          Show
          Mary Evans added a comment - Just added you as a watch here Gareth. Please feel free to ask for direction with this if you feel you can find a solution for this much need improvement! Cheers Mary
          Hide
          Gareth J Barnard added a comment -

          Thanks Mary ,

          I think the solution is to create a custom admin_setting class as documented on http://docs.moodle.org/dev/Admin_settings then copy the file picker code from Moodle forms and get the JavaScript to call 'write_setting' of the new class somehow. This would then save to storage and return a file storage location string that could be saved in the database like 'editimage.php' of the Grid Format - https://github.com/PukunuiAustralia/moodle-courseformat_grid/blob/MOODLE_24_STABLE/editimage.php. Essentially some way of adapting the $mform->get_data() call. Anybody who knows Moodle forms code will have a better idea than I.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Thanks Mary , I think the solution is to create a custom admin_setting class as documented on http://docs.moodle.org/dev/Admin_settings then copy the file picker code from Moodle forms and get the JavaScript to call 'write_setting' of the new class somehow. This would then save to storage and return a file storage location string that could be saved in the database like 'editimage.php' of the Grid Format - https://github.com/PukunuiAustralia/moodle-courseformat_grid/blob/MOODLE_24_STABLE/editimage.php . Essentially some way of adapting the $mform->get_data() call. Anybody who knows Moodle forms code will have a better idea than I. Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          Gareth I have just rememberer another tracker MDL-32143 that was implemented last year, and allows icons to be placed in the moodledata directory. In the ensuing discussion between me and Eric Merrill, Eric put forward a solution for getting theme images into the moodledata too. It's not the same thing as this but related.

          Cheers and thanks again
          Mary

          Show
          Mary Evans added a comment - - edited Gareth I have just rememberer another tracker MDL-32143 that was implemented last year, and allows icons to be placed in the moodledata directory. In the ensuing discussion between me and Eric Merrill , Eric put forward a solution for getting theme images into the moodledata too. It's not the same thing as this but related. Cheers and thanks again Mary
          Hide
          Gareth J Barnard added a comment -

          Thanks Mary I'll have a look.

          Dear Rajesh, Please have a look at my comments on https://moodle.org/mod/forum/discuss.php?d=221707 pertaining to the picker beyond what I've said above.

          Show
          Gareth J Barnard added a comment - Thanks Mary I'll have a look. Dear Rajesh, Please have a look at my comments on https://moodle.org/mod/forum/discuss.php?d=221707 pertaining to the picker beyond what I've said above.
          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this Gareth,

          It make sense to add filepicker for logo and background image, but for css I think both options (filepicker and text area) should be available.
          Currently all input elements on admin pages extends admin_setting and it doesn't use filepicker or mforms. Hence we are probably looking at introducing filepicker functionality in admin_settings.

          For "Scheme selector", you can try look at

          1. Write JS to submit theme settings to admin/settings.php?section=themesettinganomaly to get reflection of your changes.
          2. Try use admin_setting_configselect in settings.php for multiple options.
          3. Modifying customcss (arialist_set_customcss()) and purging cache
          Show
          Rajesh Taneja added a comment - Thanks for reporting this Gareth, It make sense to add filepicker for logo and background image, but for css I think both options (filepicker and text area) should be available. Currently all input elements on admin pages extends admin_setting and it doesn't use filepicker or mforms. Hence we are probably looking at introducing filepicker functionality in admin_settings. For "Scheme selector", you can try look at Write JS to submit theme settings to admin/settings.php?section=themesettinganomaly to get reflection of your changes. Try use admin_setting_configselect in settings.php for multiple options. Modifying customcss (arialist_set_customcss()) and purging cache
          Hide
          Gareth J Barnard added a comment -

          Dear Rajesh,

          Thank you - I've actually got a working 'scheme slider' based upon bxslider and admin_setting_configselect and the colour picker admin setting class. The input is still text so I use a hidden input field populated by the slider's jQuery. The tricky bit was in the setup for images as I could not use pix_url in PHP or [[pix:theme|...]] in css for the sliders images, so used a non-displayed div with attributes to convey wwwroot etc.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Rajesh, Thank you - I've actually got a working 'scheme slider' based upon bxslider and admin_setting_configselect and the colour picker admin setting class. The input is still text so I use a hidden input field populated by the slider's jQuery. The tricky bit was in the setup for images as I could not use pix_url in PHP or [ [pix:theme|...] ] in css for the sliders images, so used a non-displayed div with attributes to convey wwwroot etc. Cheers, Gareth
          Hide
          Rajesh Taneja added a comment -

          Wow, Thanks Gareth

          Show
          Rajesh Taneja added a comment - Wow, Thanks Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Rajesh,

          I'd be quite happy to share this code for inclusion in Moodle core outside of the theme I'm working on....

          The class is...

          class admin_setting_sliderselect extends admin_setting {
          
              /** @var array Array of choices value=>label */
              public $choices;
              private $noimagemode;
          
              /**
               * Constructor
               * @param string $name unique ascii name, either 'mysetting' for settings that in config, or 'myplugin/mysetting' for ones in config_plugins.
               * @param string $visiblename localised
               * @param string $description long localised info
               * @param string|int $defaultsetting
               * @param array $choices array of $value=>$label for each selection
               * @param boolean $noimagemode For when you want to use text instead for supportung mobiles and creating the images.
               */
              public function __construct($name, $visiblename, $description, $defaultsetting, $choices, $noimagemode) {
                  $this->choices = $choices;
                  $this->noimagemode = $noimagemode;
                  parent::__construct($name, $visiblename, $description, $defaultsetting);
              }
          
              /**
               * This function may be used in ancestors for lazy loading of choices
               *
               * Override this method if loading of choices is expensive, such
               * as when it requires multiple db requests.
               *
               * @return bool true if loaded, false if error
               */
              public function load_choices() {
                  /*
                    if (is_array($this->choices)) {
                    return true;
                    }
                    .... load choices here
                   */
                  return true;
              }
          
              /**
               * Check if this is $query is related to a choice
               *
               * @param string $query
               * @return bool true if related, false if not
               */
              public function is_related($query) {
                  if (parent::is_related($query)) {
                      return true;
                  }
                  if (!$this->load_choices()) {
                      return false;
                  }
                  foreach ($this->choices as $key => $value) {
                      if (strpos(textlib::strtolower($key), $query) !== false) {
                          return true;
                      }
                      if (strpos(textlib::strtolower($value), $query) !== false) {
                          return true;
                      }
                  }
                  return false;
              }
          
              /**
               * Return the setting
               *
               * @return mixed returns config if successful else null
               */
              public function get_setting() {
                  return $this->config_read($this->name);
              }
          
              /**
               * Save a setting
               *
               * @param string $data
               * @return string empty of error string
               */
              public function write_setting($data) {
                  if (!$this->load_choices() or empty($this->choices)) {
                      return '';
                  }
                  if (!array_key_exists($data, $this->choices)) {
                      return ''; // ignore it
                  }
          
                  return ($this->config_write($this->name, $data) ? '' : get_string('errorsetting', 'admin'));
              }
          
              /**
               * Returns XHTML select field
               *
               * Ensure the options are loaded, and generate the XHTML for the select
               * element and any warning message. Separating this out from output_html
               * makes it easier to subclass this class.
               *
               * @param string $data the option to show as selected.
               * @param string $current the currently selected option in the database, null if none.
               * @param string $default the default selected option.
               * @return array the HTML for the select element, and a warning message.
               */
              public function output_select_html($data, $current, $default, $extraname = '') {
                  if (!$this->load_choices() or empty($this->choices)) {
                      return array('', '');
                  }
          
                  $warning = '';
                  if (is_null($current)) {
                      // first run
                  } else if (empty($current) and (array_key_exists('', $this->choices) or array_key_exists(0, $this->choices))) {
                      // no warning
                  } else if (!array_key_exists($current, $this->choices)) {
                      $warning = get_string('warningcurrentsetting', 'admin', s($current));
                      if (!is_null($default) and $data == $current) {
                          $data = $default; // use default instead of first value when showing the form
                      }
                  }
          
                  if ($this->noimagemode == true) {
                      $selecthtml = '<select id="' . $this->get_id() . '" name="' . $this->get_full_name() . $extraname . '">';
                      foreach ($this->choices as $key => $value) {
                          // the string cast is needed because key may be integer - 0 is equal to most strings!
                          $selecthtml .= '<option value="' . $key . '"' . ((string) $key == $data ? ' selected="selected"' : '') . '>' . $value . '</option>';
                      }
                      $selecthtml .= '</select>';
                  } else {
                      global $PAGE, $CFG;
                      $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery-1.9.1.min.js');
                      $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery-migrate-1.1.0.js');
                      $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery.bxslider/jquery.bxslider.min.js');
          
                      //$selecthtml = html_writer::empty_tag('input', array('type' => 'hidden', 'id' => $this->get_id(). ' colourswatchinput', 'name' => $this->get_full_name(), 'value' => $this->get_setting()));
                      $selecthtml = html_writer::empty_tag('input', array('type' => 'text', 'style' => 'display:none;', 'id' => $this->get_id(), 'class' => 'colourswatchinput', 'name' => $this->get_full_name(), 'value' => $data));
                      $selecthtml .= html_writer::start_tag('div', array('id' => 'bxslider-container', 'style' => 'width:240px; height: 380px; padding-bottom: 2em;')); // Padding to shove the default info underneath the controls
                      $selecthtml .= html_writer::start_tag('ul', array('class' => 'bxslider', 'style' => 'width:240px; height: 360px;370 padding: 0; margin: 0;'));
                      $index = 0;
                      $startslide = 0;
                      foreach ($this->choices as $key => $value) {
                          $selecthtml .= html_writer::start_tag('li');
                          $selecthtml .= html_writer::empty_tag('img', array('src' => $CFG->wwwroot . '/theme/xxxxxxxx/pix/colourswatch-' . $key . '.png', 'style' => 'width:240px; height: 360px', 'class' => 'image-' . $index, 'image' => $key, 'title' => $value)); // Cannot use pix_url as theme might be different.
                          $selecthtml .= html_writer::end_tag('li');
                          // the string cast is needed because key may be integer - 0 is equal to most strings!
                          if ((string) $key == $data) {
                              $startslide = $index;
                          }
                          $index = $index + 1;
                      }
                      $selecthtml .= html_writer::end_tag('ul');
                      $selecthtml .= html_writer::end_tag('div');
                      // Helper to get bxslider css:
                      $selecthtml .= html_writer::empty_tag('div', array('id' => 'wwwroot', 'style' => 'display:none;', 'wwwroot' => $CFG->wwwroot, 'startslide' => $startslide));
                      $PAGE->requires->js('/theme/xxxxxxx/javascript/xxxxxxx_bxslider.js');
                      $selecthtml .= html_writer::end_tag('div');  // Needed but cannot spot why to put the description outside of form-setting.
                  }
                  return array($selecthtml, $warning);
              }
          
              /**
               * Returns XHTML select field and wrapping div(s)
               *
               * @see output_select_html()
               *
               * @param string $data the option to show as selected
               * @param string $query
               * @return string XHTML field and wrapping div
               */
              public function output_html($data, $query = '') {
                  $default = $this->get_defaultsetting();
                  $current = $this->get_setting();
          
                  list($selecthtml, $warning) = $this->output_select_html($data, $current, $default);
                  if (!$selecthtml) {
                      return '';
                  }
          
                  if (!is_null($default) and array_key_exists($default, $this->choices) and ($this->noimagemode == true)) {
                      $defaultinfo = $this->choices[$default];
                  } else {
                      $defaultinfo = NULL;
                  }
          
                  $return = html_writer::tag('div', $selecthtml, array('class' => 'form-select defaultsnext'));
          
                  return format_admin_setting($this, $this->visiblename, $return, $this->description, true, $warning, $defaultinfo, $query);
              }
          
          }
          

          It does have a fallback 'noimagemode' setting on the constructor so the theme developer can create the images for the slider. This is a taster, I can provide the rest in due time

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Rajesh, I'd be quite happy to share this code for inclusion in Moodle core outside of the theme I'm working on.... The class is... class admin_setting_sliderselect extends admin_setting { /** @ var array Array of choices value=>label */ public $choices; private $noimagemode; /** * Constructor * @param string $name unique ascii name, either 'mysetting' for settings that in config, or 'myplugin/mysetting' for ones in config_plugins. * @param string $visiblename localised * @param string $description long localised info * @param string| int $defaultsetting * @param array $choices array of $value=>$label for each selection * @param boolean $noimagemode For when you want to use text instead for supportung mobiles and creating the images. */ public function __construct($name, $visiblename, $description, $defaultsetting, $choices, $noimagemode) { $ this ->choices = $choices; $ this ->noimagemode = $noimagemode; parent::__construct($name, $visiblename, $description, $defaultsetting); } /** * This function may be used in ancestors for lazy loading of choices * * Override this method if loading of choices is expensive, such * as when it requires multiple db requests. * * @ return bool true if loaded, false if error */ public function load_choices() { /* if (is_array($ this ->choices)) { return true ; } .... load choices here */ return true ; } /** * Check if this is $query is related to a choice * * @param string $query * @ return bool true if related, false if not */ public function is_related($query) { if (parent::is_related($query)) { return true ; } if (!$ this ->load_choices()) { return false ; } foreach ($ this ->choices as $key => $value) { if (strpos(textlib::strtolower($key), $query) !== false ) { return true ; } if (strpos(textlib::strtolower($value), $query) !== false ) { return true ; } } return false ; } /** * Return the setting * * @ return mixed returns config if successful else null */ public function get_setting() { return $ this ->config_read($ this ->name); } /** * Save a setting * * @param string $data * @ return string empty of error string */ public function write_setting($data) { if (!$ this ->load_choices() or empty($ this ->choices)) { return ''; } if (!array_key_exists($data, $ this ->choices)) { return ''; // ignore it } return ($ this ->config_write($ this ->name, $data) ? '' : get_string('errorsetting', 'admin')); } /** * Returns XHTML select field * * Ensure the options are loaded, and generate the XHTML for the select * element and any warning message. Separating this out from output_html * makes it easier to subclass this class. * * @param string $data the option to show as selected. * @param string $current the currently selected option in the database, null if none. * @param string $ default the default selected option. * @ return array the HTML for the select element, and a warning message. */ public function output_select_html($data, $current, $ default , $extraname = '') { if (!$ this ->load_choices() or empty($ this ->choices)) { return array('', ''); } $warning = ''; if (is_null($current)) { // first run } else if (empty($current) and (array_key_exists('', $ this ->choices) or array_key_exists(0, $ this ->choices))) { // no warning } else if (!array_key_exists($current, $ this ->choices)) { $warning = get_string('warningcurrentsetting', 'admin', s($current)); if (!is_null($ default ) and $data == $current) { $data = $ default ; // use default instead of first value when showing the form } } if ($ this ->noimagemode == true ) { $selecthtml = '<select id= "' . $ this ->get_id() . '" name= "' . $ this ->get_full_name() . $extraname . '" >'; foreach ($ this ->choices as $key => $value) { // the string cast is needed because key may be integer - 0 is equal to most strings! $selecthtml .= '<option value= "' . $key . '" ' . ((string) $key == $data ? ' selected= "selected" ' : '') . '>' . $value . '</option>'; } $selecthtml .= '</select>'; } else { global $PAGE, $CFG; $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery-1.9.1.min.js'); $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery-migrate-1.1.0.js'); $PAGE->requires->js('/theme/xxxxxxx/javascript/jquery.bxslider/jquery.bxslider.min.js'); //$selecthtml = html_writer::empty_tag('input', array('type' => 'hidden', 'id' => $ this ->get_id(). ' colourswatchinput', 'name' => $ this ->get_full_name(), 'value' => $ this ->get_setting())); $selecthtml = html_writer::empty_tag('input', array('type' => 'text', 'style' => 'display:none;', 'id' => $ this ->get_id(), 'class' => 'colourswatchinput', 'name' => $ this ->get_full_name(), 'value' => $data)); $selecthtml .= html_writer::start_tag('div', array('id' => 'bxslider-container', 'style' => 'width:240px; height: 380px; padding-bottom: 2em;')); // Padding to shove the default info underneath the controls $selecthtml .= html_writer::start_tag('ul', array('class' => 'bxslider', 'style' => 'width:240px; height: 360px;370 padding: 0; margin: 0;')); $index = 0; $startslide = 0; foreach ($ this ->choices as $key => $value) { $selecthtml .= html_writer::start_tag('li'); $selecthtml .= html_writer::empty_tag('img', array('src' => $CFG->wwwroot . '/theme/xxxxxxxx/pix/colourswatch-' . $key . '.png', 'style' => 'width:240px; height: 360px', 'class' => 'image-' . $index, 'image' => $key, 'title' => $value)); // Cannot use pix_url as theme might be different. $selecthtml .= html_writer::end_tag('li'); // the string cast is needed because key may be integer - 0 is equal to most strings! if ((string) $key == $data) { $startslide = $index; } $index = $index + 1; } $selecthtml .= html_writer::end_tag('ul'); $selecthtml .= html_writer::end_tag('div'); // Helper to get bxslider css: $selecthtml .= html_writer::empty_tag('div', array('id' => 'wwwroot', 'style' => 'display:none;', 'wwwroot' => $CFG->wwwroot, 'startslide' => $startslide)); $PAGE->requires->js('/theme/xxxxxxx/javascript/xxxxxxx_bxslider.js'); $selecthtml .= html_writer::end_tag('div'); // Needed but cannot spot why to put the description outside of form-setting. } return array($selecthtml, $warning); } /** * Returns XHTML select field and wrapping div(s) * * @see output_select_html() * * @param string $data the option to show as selected * @param string $query * @ return string XHTML field and wrapping div */ public function output_html($data, $query = '') { $ default = $ this ->get_defaultsetting(); $current = $ this ->get_setting(); list($selecthtml, $warning) = $ this ->output_select_html($data, $current, $ default ); if (!$selecthtml) { return ''; } if (!is_null($ default ) and array_key_exists($ default , $ this ->choices) and ($ this ->noimagemode == true )) { $defaultinfo = $ this ->choices[$ default ]; } else { $defaultinfo = NULL; } $ return = html_writer::tag('div', $selecthtml, array('class' => 'form-select defaultsnext')); return format_admin_setting($ this , $ this ->visiblename, $ return , $ this ->description, true , $warning, $defaultinfo, $query); } } It does have a fallback 'noimagemode' setting on the constructor so the theme developer can create the images for the slider. This is a taster, I can provide the rest in due time Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          admin_setting_configfilepicker class and the filelib part to catch that files

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - admin_setting_configfilepicker class and the filelib part to catch that files
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          I have written the admin_setting_configfilepicker, you can find it attached in the previous comment. It has to be added to adminlib.php.
          Also I have written the filelib component to read config files. I named this component "config". It needs to be added into filelib.php.

          As an example I can show you the settings.php of a theme:

          	$name = 'theme_liquid/logo';
          	$title = get_string('logo','theme_whatever');
          	$description = get_string('logodesc', 'theme_whatever');
          	$options = array('filename'=>'theme_whatever_logo','accepted_types' => 'image');
          	$setting = new admin_setting_configfilepicker($name, $title, $description, null, $options);
          	$settings->add($setting);
          

          And the get_logo function on the lib of that theme:

          function get_logo(){
          	global $CFG, $OUTPUT;
          	$filename = get_config('theme_whatever','logo');
          	$context = context_system::instance();
          	$fs = get_file_storage();
          	if ($storedfile = $fs->get_file($context->id, 'config', 'logo', 0, '/', $filename)){
          		$path = '/'.$context->id.'/config/logo/0'.$storedfile->get_filepath().$storedfile->get_filename();
          		$logo = file_encode_url($CFG->wwwroot.'/pluginfile.php', $path, false);
          	} else {
                      //Default logo located in the theme pix
                      $logo = $OUTPUT->pix_url('logo', 'theme'); 
                  }
              return $logo;
          }
          

          Hope you enjoy it and that it can be usefull to implement the final feature!

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - I have written the admin_setting_configfilepicker, you can find it attached in the previous comment. It has to be added to adminlib.php. Also I have written the filelib component to read config files. I named this component "config". It needs to be added into filelib.php. As an example I can show you the settings.php of a theme: $name = 'theme_liquid/logo'; $title = get_string('logo','theme_whatever'); $description = get_string('logodesc', 'theme_whatever'); $options = array('filename'=>'theme_whatever_logo','accepted_types' => 'image'); $setting = new admin_setting_configfilepicker($name, $title, $description, null , $options); $settings->add($setting); And the get_logo function on the lib of that theme: function get_logo(){ global $CFG, $OUTPUT; $filename = get_config('theme_whatever','logo'); $context = context_system::instance(); $fs = get_file_storage(); if ($storedfile = $fs->get_file($context->id, 'config', 'logo', 0, '/', $filename)){ $path = '/'.$context->id.'/config/logo/0'.$storedfile->get_filepath().$storedfile->get_filename(); $logo = file_encode_url($CFG->wwwroot.'/pluginfile.php', $path, false ); } else { //Default logo located in the theme pix $logo = $OUTPUT->pix_url('logo', 'theme'); } return $logo; } Hope you enjoy it and that it can be usefull to implement the final feature!
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          Thanks - I think that looks like a viable solution.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , Thanks - I think that looks like a viable solution. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          I tested this and got it to work, after a fashion, but it is not bringing the image into the theme folder. I'm not quite sure where it is going to either, that's to say if the image you can upload is actually uploaded.

          Show
          Mary Evans added a comment - I tested this and got it to work, after a fashion, but it is not bringing the image into the theme folder. I'm not quite sure where it is going to either, that's to say if the image you can upload is actually uploaded.
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          From my understanding of the code and file storage, the uploaded file is being placed in the 'moodledir' which has permissions for writing to as set up on installation. As I think for security reason's the 'theme' folder should not be writable by code. So, file storage put's it in the 'moodledir' and keeps a track of it using data it places in the database. It's the 'get_logo()' function that needs to be used within the theme to get the logo when rendering the page in its 'renderer.php'.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , From my understanding of the code and file storage, the uploaded file is being placed in the 'moodledir' which has permissions for writing to as set up on installation. As I think for security reason's the 'theme' folder should not be writable by code. So, file storage put's it in the 'moodledir' and keeps a track of it using data it places in the database. It's the 'get_logo()' function that needs to be used within the theme to get the logo when rendering the page in its 'renderer.php'. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi,

          Gareth is right. The logo is saved in the moodle file system Thanks for the explanation!

          This solution needs a new file component to reach that moodle files inside the filesystem that I called 'config'. This reason explains why filelib needs to be modified. I don't know If this is the proper solution... Maybe an expert on moodle filesystem could tell us?

          Enjoy!

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi, Gareth is right. The logo is saved in the moodle file system Thanks for the explanation! This solution needs a new file component to reach that moodle files inside the filesystem that I called 'config'. This reason explains why filelib needs to be modified. I don't know If this is the proper solution... Maybe an expert on moodle filesystem could tell us? Enjoy! Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          Looking at the code, where is:

          'filename'=>'theme_whatever_logo'
          

          Used in the admin setting code please?

          I also wonder if this could be made more generic and complete such that 'get_logo()' became 'get_file()' as a static method on the 'admin_setting_configfilepicker' class taking in the filename as a parameter (derived from a config setting on the theme). Then would be less for the theme to code.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , Looking at the code, where is: 'filename'=>'theme_whatever_logo' Used in the admin setting code please? I also wonder if this could be made more generic and complete such that 'get_logo()' became 'get_file()' as a static method on the 'admin_setting_configfilepicker' class taking in the filename as a parameter (derived from a config setting on the theme). Then would be less for the theme to code. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Hi,

          Just created wip-MDL-35434_master. But have some issues to sort:

          1. Unable to delete file and revert back to default.
          2. No working code for non-JavaScript implementation.
          3. Have to do a 'Purge All Caches' to see the logo.
          4. Some code is un-reachable - left in as in development.
          5. Draft file entry not removed from the database.
          6. Cannot find a way of displaying the current file in the file picker.

          Any help with all bar '4' appreciated. Feel free to fork the branch / suggest fixes / comment. But please be aware that when rebasing I may have to squash the commits and do a forced update. But will try and leave the squashing of commits until ready to submit for peer review when the code works as desired.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi, Just created wip- MDL-35434 _master. But have some issues to sort: 1. Unable to delete file and revert back to default. 2. No working code for non-JavaScript implementation. 3. Have to do a 'Purge All Caches' to see the logo. 4. Some code is un-reachable - left in as in development. 5. Draft file entry not removed from the database. 6. Cannot find a way of displaying the current file in the file picker. Any help with all bar '4' appreciated. Feel free to fork the branch / suggest fixes / comment. But please be aware that when rebasing I may have to squash the commits and do a forced update. But will try and leave the squashing of commits until ready to submit for peer review when the code works as desired. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi Gareth!

          Thank you a lot for your interest in this MDL!
          I will try to write my ideas about that issues here:

          1. We have to include a button to do that.
          2. You're right The code contains a comment block but, I'm not used to that code so maybe somebody can verified and test it
          3. This patch is only related to a file picker that can be used on theme settings (so you can used it anywhere). I thought that editing theme settings the theme caches are purged but It seems I was wrong. This problem has to be solved in the admin/settings.php, I think that this script has to detect if you are editing theme settings and purge the caches when changing it. Do you agree?
          4. Which code?
          5. Draft files are not automatically removed by cron?
          6. Maybe this can be related to issue 1. Maybe we can change the filepicker to a filemanager so we will be able to erase files and see it.

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi Gareth! Thank you a lot for your interest in this MDL! I will try to write my ideas about that issues here: 1. We have to include a button to do that. 2. You're right The code contains a comment block but, I'm not used to that code so maybe somebody can verified and test it 3. This patch is only related to a file picker that can be used on theme settings (so you can used it anywhere). I thought that editing theme settings the theme caches are purged but It seems I was wrong. This problem has to be solved in the admin/settings.php, I think that this script has to detect if you are editing theme settings and purge the caches when changing it. Do you agree? 4. Which code? 5. Draft files are not automatically removed by cron? 6. Maybe this can be related to issue 1. Maybe we can change the filepicker to a filemanager so we will be able to erase files and see it. Pau
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi again,

          We can replace:

          if($img = $this->get_setting()){
             //$content .= html_writer::empty_tag('img', array('src'=> $img));
             $content .= html_writer::start_tag('p');
             $content .= $img;
             $content .= html_writer::end_tag('p');
          }
          

          by

          if($img = $this->get_setting()){
             $content .= html_writer::start_tag('p');
             $content .= html_writer::empty_tag('img', array('src'=> $img));
             $content .= html_writer::end_tag('p');
             //TODO: Put here the code to show the button to erase that image
          }
          

          This way the image will be displayed in the form.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi again, We can replace: if ($img = $ this ->get_setting()){ //$content .= html_writer::empty_tag('img', array('src'=> $img)); $content .= html_writer::start_tag('p'); $content .= $img; $content .= html_writer::end_tag('p'); } by if ($img = $ this ->get_setting()){ $content .= html_writer::start_tag('p'); $content .= html_writer::empty_tag('img', array('src'=> $img)); $content .= html_writer::end_tag('p'); //TODO: Put here the code to show the button to erase that image } This way the image will be displayed in the form. Regards, Pau
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment - - edited

          It reviewed the code of the nonJS file picker, i don't know if this is enough. Can anyone test it?

          $nonjsfilepicker = new moodle_url('/repository/draftfiles_manager.php', array(
                      'env'=>'filepicker',
                      'action'=>'browse',
                      'itemid'=>$args->itemid,
                      'subdirs'=>$args->subdirs,
                      'maxbytes'=>$args->maxbytes,
                      'maxfiles'=>$args->maxfiles,
                      'ctx_id'=>$this->_options['context']->id,
                      'course'=>$PAGE->course->id,
                      'sesskey'=>sesskey(),
                      ));
          
          
                  // non js file picker
                  
                  $content .= '<noscript>';
                  $content .= "<div><object type='text/html' data='$nonjsfilepicker' height='160' width='600' style='border:1px solid #000'></object></div>";
                  $content .= '</noscript>';
          
          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - - edited It reviewed the code of the nonJS file picker, i don't know if this is enough. Can anyone test it? $nonjsfilepicker = new moodle_url('/repository/draftfiles_manager.php', array( 'env'=>'filepicker', 'action'=>'browse', 'itemid'=>$args->itemid, 'subdirs'=>$args->subdirs, 'maxbytes'=>$args->maxbytes, 'maxfiles'=>$args->maxfiles, 'ctx_id'=>$ this ->_options['context']->id, 'course'=>$PAGE->course->id, 'sesskey'=>sesskey(), )); // non js file picker $content .= '<noscript>'; $content .= "<div><object type='text/html' data='$nonjsfilepicker' height='160' width='600' style='border:1px solid #000'></object></div>" ; $content .= '</noscript>';
          Hide
          Gareth J Barnard added a comment -

          Dear Pau,

          I can update the patch to include and test the new nonJS code.

          With '4' I mean that the current patch will not get to the image displaying code. As this is meant to be more generic for other files beyond images as I consider that it will be a requirement to get into core given it's code location and:

          'accepted_types' => 'image'
          

          There needs to be a code switch to determine if it is an image then display it.

          I did not know about cron removing draft files (as running a non-automatic cron on a development environment). However, looking at the image handling code in the Grid Course format I thought it would be a code rather than cron issue.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau, I can update the patch to include and test the new nonJS code. With '4' I mean that the current patch will not get to the image displaying code. As this is meant to be more generic for other files beyond images as I consider that it will be a requirement to get into core given it's code location and: 'accepted_types' => 'image' There needs to be a code switch to determine if it is an image then display it. I did not know about cron removing draft files (as running a non-automatic cron on a development environment). However, looking at the image handling code in the Grid Course format I thought it would be a code rather than cron issue. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Hi,

          Current state of play:

          1. Non-JS file picker works.
          2. Purge all caches still needed to see uploaded logo.
          3. Cron does not remove outdated draft files.
          4. Changed to have 'isimagefile' option for showing the image rather than the filename when appropriate.
          5. Need a 'delete' button.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi, Current state of play: 1. Non-JS file picker works. 2. Purge all caches still needed to see uploaded logo. 3. Cron does not remove outdated draft files. 4. Changed to have 'isimagefile' option for showing the image rather than the filename when appropriate. 5. Need a 'delete' button. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi again,

          Here you can find some changes:
          https://github.com/crazyserver/moodle/compare/master...wip-MDL-35434_master

          This solves delete and draft files problem.

          So I think that only purge all caches development is needed.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi again, Here you can find some changes: https://github.com/crazyserver/moodle/compare/master...wip-MDL-35434_master This solves delete and draft files problem. So I think that only purge all caches development is needed. Regards, Pau
          Hide
          Mary Evans added a comment -

          Just tested this Pau and works great!

          The problem with the need to Purge all caches, or refresh screen if Theme Designer Mode is enabled, has always been the case. Changes are not seen in the Custom setting page immediately unless you move away from that settings page. Most people open a new tab to view the Home page, and then after making changes in Custom setting, refresh the Home page in other tab.

          What we need in EVERY Custom Settings page is info about enabling Theme Designer Mode before making changes. Unless there is another way to automatically clear theme cache after making changes. Purge all cache is too aggressive for small changes in my view, especially on a production site.

          Show
          Mary Evans added a comment - Just tested this Pau and works great! The problem with the need to Purge all caches, or refresh screen if Theme Designer Mode is enabled, has always been the case. Changes are not seen in the Custom setting page immediately unless you move away from that settings page. Most people open a new tab to view the Home page, and then after making changes in Custom setting, refresh the Home page in other tab. What we need in EVERY Custom Settings page is info about enabling Theme Designer Mode before making changes. Unless there is another way to automatically clear theme cache after making changes. Purge all cache is too aggressive for small changes in my view, especially on a production site.
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans and Pau Ferrer Ocaña (crazyserver),

          Thanks - I've updated my branch with the delete changes and tidied up the code. I've also moved the delete check box to above the file picker such that it appears underneath the image which I thought would be more appropriate.

          For the 'Purge all caches' issue, I'm currently looking at a function 'theme_reset_all_caches()' called in '/theme/index.php' which I think could be less aggressive.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans and Pau Ferrer Ocaña (crazyserver) , Thanks - I've updated my branch with the delete changes and tidied up the code. I've also moved the delete check box to above the file picker such that it appears underneath the image which I thought would be more appropriate. For the 'Purge all caches' issue, I'm currently looking at a function 'theme_reset_all_caches()' called in '/theme/index.php' which I think could be less aggressive. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Bingo!

          Found:

          $setting->set_updatedcallback('theme_reset_all_caches');
          

          in '/admin/settings/appearance.php' and '/admin/settings/development.php' and it seems to work and not as aggressively as 'Purge All Caches'.

          To put in context:

          // Logo file setting
          $name = 'theme_afterburner/logo';
          $title = get_string('logo', 'theme_afterburner');
          $description = get_string('logodesc', 'theme_afterburner');
          $options = array('accepted_types' => 'image', 'isimagefile' => true);
          $setting = new admin_setting_configfilepicker($name, $title, $description, null, $options);
          $setting->set_updatedcallback('theme_reset_all_caches');
          $settings->add($setting);
          

          Therefore all theme creators should add it if a setting requires the theme cache to be regenerated.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Bingo! Found: $setting->set_updatedcallback('theme_reset_all_caches'); in '/admin/settings/appearance.php' and '/admin/settings/development.php' and it seems to work and not as aggressively as 'Purge All Caches'. To put in context: // Logo file setting $name = 'theme_afterburner/logo'; $title = get_string('logo', 'theme_afterburner'); $description = get_string('logodesc', 'theme_afterburner'); $options = array('accepted_types' => 'image', 'isimagefile' => true ); $setting = new admin_setting_configfilepicker($name, $title, $description, null , $options); $setting->set_updatedcallback('theme_reset_all_caches'); $settings->add($setting); Therefore all theme creators should add it if a setting requires the theme cache to be regenerated. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Also, pull branch updated, please test

          Show
          Gareth J Barnard added a comment - Also, pull branch updated, please test
          Hide
          Richard Oelmann added a comment -

          Gareth - how would that setting work if every setting on the page called the theme_reset_all_caches - on the basis that if you change a css setting then you want to purge caches to see the effect? Shouldn't it be called once at the time the 'Save Settings' button is clicked? E.g. I am working on updating flexi_ii at the moment and if I add that for every setting is that going to call it once or several dozen times?

          Show
          Richard Oelmann added a comment - Gareth - how would that setting work if every setting on the page called the theme_reset_all_caches - on the basis that if you change a css setting then you want to purge caches to see the effect? Shouldn't it be called once at the time the 'Save Settings' button is clicked? E.g. I am working on updating flexi_ii at the moment and if I add that for every setting is that going to call it once or several dozen times?
          Hide
          Gareth J Barnard added a comment -

          Dear Richard Oelmann,

          I'm not sure as only just found the function! However in examination of the function it appears to empty out the cache folder such that Moodle is forced to regenerate it upon page request. Therefore, multiple calls should not be an issue as the first will clear out the folder and the rest will empty out the empty folder. But, the line actually sets up an event handler name so it is possible that the event handling mechanism will remove duplicate events before it runs. Either way not much of an issue for an event that is rare.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Richard Oelmann , I'm not sure as only just found the function! However in examination of the function it appears to empty out the cache folder such that Moodle is forced to regenerate it upon page request. Therefore, multiple calls should not be an issue as the first will clear out the folder and the rest will empty out the empty folder. But, the line actually sets up an event handler name so it is possible that the event handling mechanism will remove duplicate events before it runs. Either way not much of an issue for an event that is rare. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Can be this method called inside write_setting function? $this->set_updatedcallback('theme_reset_all_caches'); ??

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Can be this method called inside write_setting function? $this->set_updatedcallback('theme_reset_all_caches'); ??
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          No because as previously mentioned this is a generic class that can be used by other things besides themes such as mods, blocks and course formats.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , No because as previously mentioned this is a generic class that can be used by other things besides themes such as mods, blocks and course formats. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Davo Smith,

          Given https://moodle.org/mod/forum/discuss.php?d=225900

          Is it a matter of just using:

          $params->env = 'filemanager';
          

          To change a non-form filepicker to a filemanager (changing js too). Or something a bit more involved?

          Any help appreciated, thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Davo Smith , Given https://moodle.org/mod/forum/discuss.php?d=225900 Is it a matter of just using: $params->env = 'filemanager'; To change a non-form filepicker to a filemanager (changing js too). Or something a bit more involved? Any help appreciated, thanks, Gareth
          Hide
          Mary Evans added a comment - - edited

          As mention in dispatches in the themes forum, this works amazingly well. So thanks Pau and Gareth for getting this 'dream' nearer to being a reality in all themes.

          One minor change to the adminlib.php is to these lines of code...it's adding a space before the text for the label so that it leaves a gap after the checkbox and before 'Delete'.

          if ($file) {
              $labelid = $this->get_id().'_delete';
              $content .= '<input type="checkbox" name="'.$this->get_full_name().'_delete" id="'.$labelid.'" value="1" /><label for="'.$labelid.'">
          &nbsp;'.get_string('delete').'</label>';
          }
          }
          
          Show
          Mary Evans added a comment - - edited As mention in dispatches in the themes forum, this works amazingly well. So thanks Pau and Gareth for getting this 'dream' nearer to being a reality in all themes. One minor change to the adminlib.php is to these lines of code...it's adding a space before the text for the label so that it leaves a gap after the checkbox and before 'Delete'. if ($file) { $labelid = $ this ->get_id().'_delete'; $content .= '<input type= "checkbox" name= "'.$ this ->get_full_name().'_delete" id= "'.$labelid.'" value= "1" /><label for = "'.$labelid.'" > &nbsp;'.get_string('delete').'</label>'; } }
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi Gareth,

          I have sent you a pull request that manage the files previously uploaded throught the filepicker.

          After considering it I don't think that using filemanager instead of filepicker is a good idea. If we do this, We will be able to manage more than one file at a time and it would be weird...

          Regrets,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi Gareth, I have sent you a pull request that manage the files previously uploaded throught the filepicker. After considering it I don't think that using filemanager instead of filepicker is a good idea. If we do this, We will be able to manage more than one file at a time and it would be weird... Regrets, Pau
          Hide
          Mary Evans added a comment -

          @Gareth,

          You will need to put back those white spaces you cleaned up with the codechecker in files that have been altered for this patch. The "Integrator" would send it back or at best be annoyed with you, and I would hate that to happen.

          Cheers
          Mary

          Show
          Mary Evans added a comment - @Gareth, You will need to put back those white spaces you cleaned up with the codechecker in files that have been altered for this patch. The "Integrator" would send it back or at best be annoyed with you, and I would hate that to happen. Cheers Mary
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans and Pau Ferrer Ocaña (crazyserver),

          Thanks for the pull request, I will integrate it later if I may as domestic stuff to do first!

          The white space issues are all to do with the new code in converting tabs to spaces etc. So the code looks clean and Moodle standard friendly (including capitals and periods on comments etc.). No other code was harmed in the making of this patch

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans and Pau Ferrer Ocaña (crazyserver) , Thanks for the pull request, I will integrate it later if I may as domestic stuff to do first! The white space issues are all to do with the new code in converting tabs to spaces etc. So the code looks clean and Moodle standard friendly (including capitals and periods on comments etc.). No other code was harmed in the making of this patch Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi again,

          As I tried to say on the forum (but I was flagged as spam -> I'm solving it).
          Solving the 6th issue: '6. Cannot find a way of displaying the current file in the file picker.'. I introduced again the 5th: '5. Draft file entry not removed from the database.'. This is because the only way to display the files in the file picker is to copy it to draft with the function file_prepare_draft_area. Saving the form erases that files but we cannot detect when the form is not saved to erase them.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi again, As I tried to say on the forum (but I was flagged as spam -> I'm solving it). Solving the 6th issue: '6. Cannot find a way of displaying the current file in the file picker.'. I introduced again the 5th: '5. Draft file entry not removed from the database.'. This is because the only way to display the files in the file picker is to copy it to draft with the function file_prepare_draft_area. Saving the form erases that files but we cannot detect when the form is not saved to erase them. Regards, Pau
          Hide
          Mary Evans added a comment - - edited

          I'm not sure what the reasoning is behind wanting to use a different filepicker, but as far as I am concerned the one that is part of this patch as it is now works fine. So follow the old adage, "...if it ain't broke don't fix it!..."

          Show
          Mary Evans added a comment - - edited I'm not sure what the reasoning is behind wanting to use a different filepicker, but as far as I am concerned the one that is part of this patch as it is now works fine. So follow the old adage, "...if it ain't broke don't fix it!..."
          Hide
          Mary Evans added a comment -

          Davo Smith Just added you as a watcher here.

          Show
          Mary Evans added a comment - Davo Smith Just added you as a watcher here.
          Hide
          Petr Škoda added a comment -

          Hi everybody, I had a quick look at the patch, here are my comments:

          The patch does not seem to be browser caching friendly, moodle cache purging will do nothing I am afraid. There needs to be something incrementing in the file URL and a long cache lifetime.

          There is another problem with performance, the file name should be stored somewhere so that each page does not query the DB to find the file name. I think it would be unacceptable if each config file added one DB query to each page load. Another problem is the inclusion of adminlib.php on each page, that is not good for our memory use either.

          I personally do not like the newly invented configfile_ component name, that is wrong, if there are any theme files they MUST be stored under theme_themename component. That would also fix the completely missing access control. I believe that the file serving should be delegated to each component in a standard way.

          In any case thanks for working on this issue, I think it is a solvable problem and it would be great to have a standardised solution for early 2.6dev.

          Ciao

          Show
          Petr Škoda added a comment - Hi everybody, I had a quick look at the patch, here are my comments: The patch does not seem to be browser caching friendly, moodle cache purging will do nothing I am afraid. There needs to be something incrementing in the file URL and a long cache lifetime. There is another problem with performance, the file name should be stored somewhere so that each page does not query the DB to find the file name. I think it would be unacceptable if each config file added one DB query to each page load. Another problem is the inclusion of adminlib.php on each page, that is not good for our memory use either. I personally do not like the newly invented configfile_ component name, that is wrong, if there are any theme files they MUST be stored under theme_themename component. That would also fix the completely missing access control. I believe that the file serving should be delegated to each component in a standard way. In any case thanks for working on this issue, I think it is a solvable problem and it would be great to have a standardised solution for early 2.6dev. Ciao
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi and thank you for your comments,

          So the new issues are:
          1. Solve browser caching when saving
          2. Save the filename somewhere to be reached without DB.
          3. Save the relative path of the file not to depend on adminlib.php
          4. Every component needs to serve their files.
          5. Erase draft files if the form is cancelled.

          And this is what I think:
          1. I have to think about how to do it.
          2. The problem is that every setting can be set on config table or on config_plugins... Config is already preloaded, what do you recommend to preload that?
          3. Is easy to do. We can save the path to reach that file or get all the files from the component-filearea and take the first.
          4. Easy, but it will cause a harder way to use this class. However it is a good security reason.
          5. I'm stuck on this.

          Thanks!!

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi and thank you for your comments, So the new issues are: 1. Solve browser caching when saving 2. Save the filename somewhere to be reached without DB. 3. Save the relative path of the file not to depend on adminlib.php 4. Every component needs to serve their files. 5. Erase draft files if the form is cancelled. And this is what I think: 1. I have to think about how to do it. 2. The problem is that every setting can be set on config table or on config_plugins... Config is already preloaded, what do you recommend to preload that? 3. Is easy to do. We can save the path to reach that file or get all the files from the component-filearea and take the first. 4. Easy, but it will cause a harder way to use this class. However it is a good security reason. 5. I'm stuck on this. Thanks!! Pau
          Hide
          Petr Škoda added a comment -

          1. this is not about saving at all, this is about constructing URL with time based number in URL which is later ignored in pluginfile.php
          2. the correct place to store the file name are the config table and config plugins table, obviously themes would use name of the setting and theme component; MUC should cache all config values for you
          3. moodle_url already contains some pluginfile.php support
          4. yes, there should be support for that already; we should probably just standardise the area name a bit "settings" area sounds good to me
          5. the draft files are deleted automatically after few days, just make sure the draft files are created only when really necessary (that is when displaying the setting, in future it could be some ajax-only magic to improve the perf)

          Show
          Petr Škoda added a comment - 1. this is not about saving at all, this is about constructing URL with time based number in URL which is later ignored in pluginfile.php 2. the correct place to store the file name are the config table and config plugins table, obviously themes would use name of the setting and theme component; MUC should cache all config values for you 3. moodle_url already contains some pluginfile.php support 4. yes, there should be support for that already; we should probably just standardise the area name a bit "settings" area sounds good to me 5. the draft files are deleted automatically after few days, just make sure the draft files are created only when really necessary (that is when displaying the setting, in future it could be some ajax-only magic to improve the perf)
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          1. Ok, I added a the timemodified of the file into the url of the logo.

          $file = make_pluginfile_url($context->id, 'configfile_'.$plugin, $name, 0, $storedfile->get_filepath(), $storedfile->get_filename());
          //Prevent caching
          $file->param('timemodified',$storedfile->get_timemodified());
          

          2. I have modified this inserting on the cache. Can anyone review it?

              if (!empty($theme->settings->logo)) {
                  $cache = cache::make('theme_afterburner', 'settings');
          		if (!$logo = $cache->get('logo')) {
          			require_once($CFG->libdir.'/adminlib.php');
          			$filename = get_config('theme_afterburner','logo'); // 'theme_afterburner/logo' from settings.php.
          			$context = context_system::instance();
          			$logo = admin_setting_configfilepicker::get_file($filename, $context, 'theme_afterburner', 'logo');
          			$cache->set('logo', $logo);
          		}
              } else {
                  $logo = null;
              }
          

          3. I think with the cache we can solve this is already solve. Isn't it? It depends but only is loaded if it cannot get it from the cache.
          4. I don't undestand what you mean:
          Now the component is configfile_X. While X is the name of the component
          Now the filearea is the name of the setting. 'logo' in theme_afterburner case.
          And the filename is the same that the user upload.
          I undestand that you want to use the component name as the component, Filearea has to be 'settings'. How are we going to differ between settings? With the filename? The itemid will always be 0.
          5. Ok, so this issue is solved.

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - 1. Ok, I added a the timemodified of the file into the url of the logo. $file = make_pluginfile_url($context->id, 'configfile_'.$plugin, $name, 0, $storedfile->get_filepath(), $storedfile->get_filename()); //Prevent caching $file->param('timemodified',$storedfile->get_timemodified()); 2. I have modified this inserting on the cache. Can anyone review it? if (!empty($theme->settings->logo)) { $cache = cache::make('theme_afterburner', 'settings'); if (!$logo = $cache->get('logo')) { require_once($CFG->libdir.'/adminlib.php'); $filename = get_config('theme_afterburner','logo'); // 'theme_afterburner/logo' from settings.php. $context = context_system::instance(); $logo = admin_setting_configfilepicker::get_file($filename, $context, 'theme_afterburner', 'logo'); $cache->set('logo', $logo); } } else { $logo = null ; } 3. I think with the cache we can solve this is already solve. Isn't it? It depends but only is loaded if it cannot get it from the cache. 4. I don't undestand what you mean: Now the component is configfile_X. While X is the name of the component Now the filearea is the name of the setting. 'logo' in theme_afterburner case. And the filename is the same that the user upload. I undestand that you want to use the component name as the component, Filearea has to be 'settings'. How are we going to differ between settings? With the filename? The itemid will always be 0. 5. Ok, so this issue is solved.
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          I have now merged Pau's changes into the branch.

          Lost the plot with this now. What is the state of play regarding keeping this simple such that it is easy for anything to use the changes in an OO way? As I'm concerned about the overheads on the Afterburner theme with specifics that are really theme / core generics.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, I have now merged Pau's changes into the branch. Lost the plot with this now. What is the state of play regarding keeping this simple such that it is easy for anything to use the changes in an OO way? As I'm concerned about the overheads on the Afterburner theme with specifics that are really theme / core generics. Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          All we need is a mechanism to upload an image to use as a logo, or page background, or whatever the setting happens to be where an image is required.

          For the purpose of this testing we are only focusing on a logo as this is the most needed element of a theme.

          I suppose the alternative is to use a Moodle form called from the settings page. This would allow you to use the filepicker by default via the form.

          See Aadar theme in New plugins directory.
          https://moodle.org/plugins/download.php/2478/theme_aadar_moodle24_2012112900.zip

          Show
          Mary Evans added a comment - - edited All we need is a mechanism to upload an image to use as a logo, or page background, or whatever the setting happens to be where an image is required. For the purpose of this testing we are only focusing on a logo as this is the most needed element of a theme. I suppose the alternative is to use a Moodle form called from the settings page. This would allow you to use the filepicker by default via the form. See Aadar theme in New plugins directory. https://moodle.org/plugins/download.php/2478/theme_aadar_moodle24_2012112900.zip
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          Ok, fair point

          It is entirely feasible to take our bespoke admin setting class out of adminlib.php and put it somewhere else (and it still inherit from admin_setting in adminlib.php) as I've been able to do with a custom BXSlider implementation (but that uses jQuery). So perhaps in the 'themes' folder and call it 'lib.php' just like 'renderer.php' is repeated in the 'course' folder and below.

          So can we all be in agreement on the move to the 'theme' folder before it happens?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Ok, fair point It is entirely feasible to take our bespoke admin setting class out of adminlib.php and put it somewhere else (and it still inherit from admin_setting in adminlib.php) as I've been able to do with a custom BXSlider implementation (but that uses jQuery). So perhaps in the 'themes' folder and call it 'lib.php' just like 'renderer.php' is repeated in the 'course' folder and below. So can we all be in agreement on the move to the 'theme' folder before it happens? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          The Aadar theme uses the image code that is in the Grid format, and that is not exactly perfect. Somehow I feel this is a full circle that has come around and slapped me on the rear - painful!

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , The Aadar theme uses the image code that is in the Grid format, and that is not exactly perfect. Somehow I feel this is a full circle that has come around and slapped me on the rear - painful! Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Tim Hunt just retweeted from Tim Yates:

          Software engineering:

          1) Incrementally make a simple thing so complex it's unusable
          2) invent simple replacement
          3) GOTO 1

          I've also just updated to have the space before the 'delete' label.

          Show
          Gareth J Barnard added a comment - Tim Hunt just retweeted from Tim Yates: Software engineering: 1) Incrementally make a simple thing so complex it's unusable 2) invent simple replacement 3) GOTO 1 I've also just updated to have the space before the 'delete' label.
          Hide
          Petr Škoda added a comment -

          Hmm, let me repeat some very basic plugin rules:

          a/ all core settings go into 'config' table, plugin settings go into 'config_plugins' table where plugin column must contain frankenstyle name of plugin
          b/ all plugin files that go into 'files' table must use frankenstyle name in 'component' column

          back to the patch details
          1/ do not invent new &timemodified=xx, look at other places that need to invalidate caches, you will see that the changing revision number must go before the file name, otherwise relative links will not work
          2/ do not invent new caches here, see the get_config() implementation details in master
          3/ read the moodle_url implementation, concentrate on the slasharguments there and grep for uses if necessary
          4/ file addressing = context+frankenstyle+area+itemid+path+file

          Anyway you need to decide how to store files because there are different options:
          1/ pick file and rename to fixed name and store, then you would not need to store the file name anywhere, just a flag if it exists or not
          2/ pick file and store it under original name, then you need to store the file name+path as plugin config because repeated fetching of file list from db is unacceptable

          Another issue I saw in the patch: you should not delete draft files after saving, imagine what would happen if user somehow submitted the form twice?

          Show
          Petr Škoda added a comment - Hmm, let me repeat some very basic plugin rules: a/ all core settings go into 'config' table, plugin settings go into 'config_plugins' table where plugin column must contain frankenstyle name of plugin b/ all plugin files that go into 'files' table must use frankenstyle name in 'component' column back to the patch details 1/ do not invent new &timemodified=xx, look at other places that need to invalidate caches, you will see that the changing revision number must go before the file name, otherwise relative links will not work 2/ do not invent new caches here, see the get_config() implementation details in master 3/ read the moodle_url implementation, concentrate on the slasharguments there and grep for uses if necessary 4/ file addressing = context+frankenstyle+area+itemid+path+file Anyway you need to decide how to store files because there are different options: 1/ pick file and rename to fixed name and store, then you would not need to store the file name anywhere, just a flag if it exists or not 2/ pick file and store it under original name, then you need to store the file name+path as plugin config because repeated fetching of file list from db is unacceptable Another issue I saw in the patch: you should not delete draft files after saving, imagine what would happen if user somehow submitted the form twice?
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Thank you for reviewing the current state of the patch and your kind words of wisdom.

          So, this is a plugin issue and as such if the filename is stored should go into the 'config_plugins' table and thus be usable by contributed themes too.

          How should we go about solving the 'configfile_' prefix in 'filelib.php'? Should we have a new thing like 'theme_' string comparison like the mod's and blocks?

          Please be gentle with us, we are just software engineers learning the 'Art of Moodle development'.

          Thanks again,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Thank you for reviewing the current state of the patch and your kind words of wisdom. So, this is a plugin issue and as such if the filename is stored should go into the 'config_plugins' table and thus be usable by contributed themes too. How should we go about solving the 'configfile_' prefix in 'filelib.php'? Should we have a new thing like 'theme_' string comparison like the mod's and blocks? Please be gentle with us, we are just software engineers learning the 'Art of Moodle development'. Thanks again, Gareth
          Hide
          Derek Chirnside added a comment -

          Fascinating insights into the comlexity of a plugin development process.
          @Petr, I presume your rules are somewhere in the dev docs. I saw another two posts recently of people arriving with the blank slate question "I know nothing about Moodle and I want to start to develop"
          @Gareth. Love Tims comments about Software Engineering. I've also known this to occur in management processes.

          -Derek

          Show
          Derek Chirnside added a comment - Fascinating insights into the comlexity of a plugin development process. @Petr, I presume your rules are somewhere in the dev docs. I saw another two posts recently of people arriving with the blank slate question "I know nothing about Moodle and I want to start to develop" @Gareth. Love Tims comments about Software Engineering. I've also known this to occur in management processes. -Derek
          Hide
          Gareth J Barnard added a comment -

          Dear Derek Chirnside,

          I believe the documentation is on http://docs.moodle.org/dev/Plugins and referenced from.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Derek Chirnside , I believe the documentation is on http://docs.moodle.org/dev/Plugins and referenced from. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi Petr Škoda and Gareth J Barnard,

          After reviewing all your comments I think I managed to put all in order but we have to consider some things:
          a/ b/ Got it, but I have modified the filelib untill we decide how to manage those files

          1/ Amazing, I didn't realise that.
          2/ Now on config plugins I save the relative path to the file /{$context->id}/{$this->plugin}/settings/".$file->get_timemodified()."/{$this->name}
          This way we can serve files without needing adminlib and with the cache of get_config
          3/ and 4/ Ok

          Also, I clean the code and now draft files are not deleted.

          Finally, I think we have to manage diferent fileareas for diferent settings now the files are this way:
          --Component: theme_afterburner (depending on the plugin)
          --Filearea: settings (fixed)
          --Filepath: /
          --Filename: logo (Setting name)

          I think we have to manage this other way:
          --Filearea: settings_logo (fixed + settings name)
          --Filename: whatever (we will save it on config)

          What do you think? I will manage to commit the code to my branch now. (https://github.com/crazyserver/moodle/commits/wip-MDL-35434_master)

          Thanks for all your help and comprension!

          Cheers,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi Petr Škoda and Gareth J Barnard, After reviewing all your comments I think I managed to put all in order but we have to consider some things: a/ b/ Got it, but I have modified the filelib untill we decide how to manage those files 1/ Amazing, I didn't realise that. 2/ Now on config plugins I save the relative path to the file /{$context->id}/{$this->plugin}/settings/".$file->get_timemodified()."/{$this->name} This way we can serve files without needing adminlib and with the cache of get_config 3/ and 4/ Ok Also, I clean the code and now draft files are not deleted. Finally, I think we have to manage diferent fileareas for diferent settings now the files are this way: --Component: theme_afterburner (depending on the plugin) --Filearea: settings (fixed) --Filepath: / --Filename: logo (Setting name) I think we have to manage this other way: --Filearea: settings_logo (fixed + settings name) --Filename: whatever (we will save it on config) What do you think? I will manage to commit the code to my branch now. ( https://github.com/crazyserver/moodle/commits/wip-MDL-35434_master ) Thanks for all your help and comprension! Cheers, Pau
          Hide
          Mary Evans added a comment - - edited

          I'm a dressmaker really...Moodle is my hobby! LOL

          Show
          Mary Evans added a comment - - edited I'm a dressmaker really...Moodle is my hobby! LOL
          Hide
          Petr Škoda added a comment -

          I/ There is no need to modify filelib, see the last else block in function file_pluginfile(), it uses the frankenstyle component to find out which lib.php to include for the callback.

          II/ save the file with itemid 0, use the revisions only in urls

          III/ the component + file area should be param in setting constructor

          Show
          Petr Škoda added a comment - I/ There is no need to modify filelib, see the last else block in function file_pluginfile(), it uses the frankenstyle component to find out which lib.php to include for the callback. II/ save the file with itemid 0, use the revisions only in urls III/ the component + file area should be param in setting constructor
          Hide
          Gareth J Barnard added a comment -

          I'm a type safe pre-runtime checked Java Software Engineer and Educator really. Moodle is my hobby too . But being a dressmaker will make you good as a theme designer

          Show
          Gareth J Barnard added a comment - I'm a type safe pre-runtime checked Java Software Engineer and Educator really. Moodle is my hobby too . But being a dressmaker will make you good as a theme designer
          Hide
          Petr Škoda added a comment -

          A good "documentation" for plugins is probably the plugin uninstallation code in function uninstall_plugin() from adminlib.php, it lists many of the plugin features...

          Show
          Petr Škoda added a comment - A good "documentation" for plugins is probably the plugin uninstallation code in function uninstall_plugin() from adminlib.php, it lists many of the plugin features...
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          With '1' I saw the else statement, the snag is that this file picker needs to work with any theme without them having to repeatedly implement the same code (as used in filelib.php for this patch at the moment) and thus there is no specific frankenstyle specific lib.php to call.

          Cheers,

          Gareth

          P.S. This is all about providing as much code outside of Afterburner as possible. Afterburner is only being changed as an example where it can be used.

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , With '1' I saw the else statement, the snag is that this file picker needs to work with any theme without them having to repeatedly implement the same code (as used in filelib.php for this patch at the moment) and thus there is no specific frankenstyle specific lib.php to call. Cheers, Gareth P.S. This is all about providing as much code outside of Afterburner as possible. Afterburner is only being changed as an example where it can be used.
          Hide
          Petr Škoda added a comment - - edited

          I am sorry, we are moving away from plugin specific hacks, at this stage it should be implemented in individual themes using standard mechanisms.

          In the future we could create some helper and then you would do something like this in theme/xx/lib.php:

          function theme_xx_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options=array()) {
             if ($filearea === 'settings' and $context->contextlevel == CONTEXT_SYSTEM) {
                file_send_public_settings_file('theme_xx', settings', $args, $forcedownload, $options);
             } else {
                send_file_not_found();
             }
          }
          
          Show
          Petr Škoda added a comment - - edited I am sorry, we are moving away from plugin specific hacks, at this stage it should be implemented in individual themes using standard mechanisms. In the future we could create some helper and then you would do something like this in theme/xx/lib.php: function theme_xx_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options=array()) { if ($filearea === 'settings' and $context->contextlevel == CONTEXT_SYSTEM) { file_send_public_settings_file('theme_xx', settings', $args, $forcedownload, $options); } else { send_file_not_found(); } }
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Thanks for informing us of the policy decision. Is it within the remit of this tracker to provide such a helper set of classes? Such as creating an abstract class in '/theme/lib.php' which is inherited by '/theme/xx/lib.php' and the latter called by 'filelib.php', where the precedence functional model for this in Moodle is the '/course/format/renderer.php' - '/course/format/topics/renderer.php' model.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Thanks for informing us of the policy decision. Is it within the remit of this tracker to provide such a helper set of classes? Such as creating an abstract class in '/theme/lib.php' which is inherited by '/theme/xx/lib.php' and the latter called by 'filelib.php', where the precedence functional model for this in Moodle is the '/course/format/renderer.php' - '/course/format/topics/renderer.php' model. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          I am not an integrator any more and I do not create any new coding rules. My general advice is to create initial patch that works without core modification and only if that goes through peer review try to abstract it.

          In this particular case it should not be theme specific imo, any helper would go either to adminlib or filelib. I do not think we can hardcode any area names for setting files here because that might open security holes in access control of existing plugins - that means you will have to create theme/xx/lib.php in any case and call the helper there.

          Show
          Petr Škoda added a comment - I am not an integrator any more and I do not create any new coding rules. My general advice is to create initial patch that works without core modification and only if that goes through peer review try to abstract it. In this particular case it should not be theme specific imo, any helper would go either to adminlib or filelib. I do not think we can hardcode any area names for setting files here because that might open security holes in access control of existing plugins - that means you will have to create theme/xx/lib.php in any case and call the helper there.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Thank you. I'm not complaining about the policies just need to know what they are and where to find them .

          I'm going to re-read the 'else' code at the bottom of the 'filelib.php' and see if it will call '/theme/xx/lib.php', then I know I can create an abstract parent in '/theme/lib.php' thus reducing core changes and moving the 'adminlib.php' changes to '/theme/lib.php' too.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Thank you. I'm not complaining about the policies just need to know what they are and where to find them . I'm going to re-read the 'else' code at the bottom of the 'filelib.php' and see if it will call '/theme/xx/lib.php', then I know I can create an abstract parent in '/theme/lib.php' thus reducing core changes and moving the 'adminlib.php' changes to '/theme/lib.php' too. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          I've just manually merged the pull request #2. Please check the changes. If everything works, then I can have a bash at doing the abstract helper tomorrow for the 'filelib.php' issue as mentioned above.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , I've just manually merged the pull request #2. Please check the changes. If everything works, then I can have a bash at doing the abstract helper tomorrow for the 'filelib.php' issue as mentioned above. Thanks, Gareth
          Hide
          Mary Evans added a comment -

          I've just been testing Pau's version, and was not impressed with performance. The image took 3 or 4 attempts to be deleted, and also the version of Afterburner was updated to a higher version, which is not good as I am going to have to manually put it back to the version it was previous, otherwise there will be conflicts.

          This seems to be getting overly messy and not a bit like I had thought originally.

          Show
          Mary Evans added a comment - I've just been testing Pau's version, and was not impressed with performance. The image took 3 or 4 attempts to be deleted, and also the version of Afterburner was updated to a higher version, which is not good as I am going to have to manually put it back to the version it was previous, otherwise there will be conflicts. This seems to be getting overly messy and not a bit like I had thought originally.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          I'm sorry about the version thing. It was needed to create a new cache but now is not needed.

          Delete check is working now.

          I have erased the modifications of the filelib and I've started the development of the theme_xxx_pluginfile function on theme/xxx/lib.php.

          I updated the code.

          I'm sorry but I'll be busy the rest of the week.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - I'm sorry about the version thing. It was needed to create a new cache but now is not needed. Delete check is working now. I have erased the modifications of the filelib and I've started the development of the theme_xxx_pluginfile function on theme/xxx/lib.php. I updated the code. I'm sorry but I'll be busy the rest of the week. Regards, Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans and Pau Ferrer Ocaña (crazyserver),

          Thanks. Pull integrated. I think the lib.php should call the filelib.php functions for sending the file.

          Thanks for all your hard work Pau.

          I'll do some additional refactoring.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans and Pau Ferrer Ocaña (crazyserver) , Thanks. Pull integrated. I think the lib.php should call the filelib.php functions for sending the file. Thanks for all your hard work Pau. I'll do some additional refactoring. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          I'm going to sleep on it and see how this can be a clean 'theme' only change.

          Show
          Gareth J Barnard added a comment - I'm going to sleep on it and see how this can be a clean 'theme' only change.
          Hide
          Mary Evans added a comment - - edited

          @Pau & @Gareth:

          Many thanks for your combined efforts with this, elusive butterfly of an idea to make Moodle themes better and easier to work with and use. However, it seems to me that what was a simple idea is, in all truth, harder to achieve because of hidden pitfalls.

          I'm beginning to think that calling themes 'plugins' was a bad move, as they are nothing like a plugin, at least not in the way blocks and mods are with DB capability.

          Anyway, regardless of what Petr said, the fact it was working perfectly well, is proof that it is still achievable. So please don't loose heart either of you.

          I'm going to be scarce for a few weeks now, too.

          Good night and thanks again.
          Mary

          Show
          Mary Evans added a comment - - edited @Pau & @Gareth: Many thanks for your combined efforts with this, elusive butterfly of an idea to make Moodle themes better and easier to work with and use. However, it seems to me that what was a simple idea is, in all truth, harder to achieve because of hidden pitfalls. I'm beginning to think that calling themes 'plugins' was a bad move, as they are nothing like a plugin, at least not in the way blocks and mods are with DB capability. Anyway, regardless of what Petr said, the fact it was working perfectly well, is proof that it is still achievable. So please don't loose heart either of you. I'm going to be scarce for a few weeks now, too. Good night and thanks again. Mary
          Hide
          Gareth J Barnard added a comment -

          Hi all,

          Tried rebasing on latest master, complete mess, so created a new branch wip-MDL-35434_m2, squashed, rebased. Hopefully code is the same as last commit. Please update for development. Old master branch still left for reference.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi all, Tried rebasing on latest master, complete mess, so created a new branch wip- MDL-35434 _m2, squashed, rebased. Hopefully code is the same as last commit. Please update for development. Old master branch still left for reference. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          Hello Gareth, there are still multiple problems in your patch:

          1/ $fileurl = "/{$context->id}/{$this->plugin}/settings/".$file->get_timemodified()."/{$this->name}"; - do not store the timemodified there, just the file name + path; add the revision counter when constructing the URL later

          2/ the area name should be configurable, we need to store the full file name with extension here, using $this->name seems wrong to me

          3/ do not include adminlib.php, just get the config value (the file name) and construct the moodle_url() from it using theme_get_revision() when you need the file

          Show
          Petr Škoda added a comment - Hello Gareth, there are still multiple problems in your patch: 1/ $fileurl = "/{$context->id}/{$this->plugin}/settings/".$file->get_timemodified()."/{$this->name}"; - do not store the timemodified there, just the file name + path; add the revision counter when constructing the URL later 2/ the area name should be configurable, we need to store the full file name with extension here, using $this->name seems wrong to me 3/ do not include adminlib.php, just get the config value (the file name) and construct the moodle_url() from it using theme_get_revision() when you need the file
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Absolutely! I know these need to be fixed - all I did was rebase to the latest Moodle version and had issues with the previous 17 commits rebasing, which is why I squashed and created a new branch that will hopefully rebase in the future with additional commits upon it.

          Thanks for your review, really helpful .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Absolutely! I know these need to be fixed - all I did was rebase to the latest Moodle version and had issues with the previous 17 commits rebasing, which is why I squashed and created a new branch that will hopefully rebase in the future with additional commits upon it. Thanks for your review, really helpful . Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi all!

          Comments commited to the code!

          Have a great weekend!

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi all! Comments commited to the code! Have a great weekend! Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          Thank you - code pull merged and branch updated.

          Have a great weekend too

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , Thank you - code pull merged and branch updated. Have a great weekend too Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Just refactored such that common functions are now in a 'theme/lib.php' for other themes. But slight issue when you have more than one draft file that the first file is always being picked regardless of the one you upload to replace it. This is because the draft file id is the same for all files, however attempting to use a generated draft file id (commented out code in adminlib.php) results in no file being detected in 'write_setting' and hence no new image is used despite having a proper entry in the files table for the uploaded file.

          Off to watch the Grand National

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Just refactored such that common functions are now in a 'theme/lib.php' for other themes. But slight issue when you have more than one draft file that the first file is always being picked regardless of the one you upload to replace it. This is because the draft file id is the same for all files, however attempting to use a generated draft file id (commented out code in adminlib.php) results in no file being detected in 'write_setting' and hence no new image is used despite having a proper entry in the files table for the uploaded file. Off to watch the Grand National Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          looks better now, there are some more issues there:

          1/ incorrect format of git commit messages, see http://docs.moodle.org/dev/Commit_cheat_sheet (please note the "commit area" is wrong there and should not be used, not everybody agrees but it is in my opinion)

          2/ you are hitting old known problems with the file picker, you are not file picking actually there because file picking is supposed to be used in places where you upload something and process it but do not store it, in this particular case you can not create pluginfile.php link to from admin setting in arbitrary plugin - that is a wrong because only plugin that "owns" the files is allowed to create links. Solution would be to use file manager element, unfortunately the restriction to upload only one file is missing - maybe Marina could help here now, the underlying code is not good.

          3/ do not put any includes at the top of lib.php files, include in functions or methods instead to decrease memory use

          4/ you must whitelist allowed file areas in theme_afterburner_pluginfile() because you do not want to allow access to all theme file areas (I know it is unlikely that themes have private files, but sooner or later somebody would copy the code to some other plugin type)

          Show
          Petr Škoda added a comment - looks better now, there are some more issues there: 1/ incorrect format of git commit messages, see http://docs.moodle.org/dev/Commit_cheat_sheet (please note the "commit area" is wrong there and should not be used, not everybody agrees but it is in my opinion) 2/ you are hitting old known problems with the file picker, you are not file picking actually there because file picking is supposed to be used in places where you upload something and process it but do not store it, in this particular case you can not create pluginfile.php link to from admin setting in arbitrary plugin - that is a wrong because only plugin that "owns" the files is allowed to create links. Solution would be to use file manager element, unfortunately the restriction to upload only one file is missing - maybe Marina could help here now, the underlying code is not good. 3/ do not put any includes at the top of lib.php files, include in functions or methods instead to decrease memory use 4/ you must whitelist allowed file areas in theme_afterburner_pluginfile() because you do not want to allow access to all theme file areas (I know it is unlikely that themes have private files, but sooner or later somebody would copy the code to some other plugin type)
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          Thanks

          RE:

          1. Temporary commit message designed to be informative to the developers of this patch not meant to be a serious contender for inclusion in the main log. As I knew there was work to be done and the integrator wants only one commit when integrating, I'm going to squash the commits into one with a suitable commit message - a de-bling / de-tartify exercise. To be honest, I was being quick as I wanted to see the Grand National on the telly.

          2. All we want to do is upload a single file and have it displayed with the option of removing it and getting back to the default. Why oh why is it so difficult!! Please help as the File API Guru . And should we ignore the whole draft files bit and just go for a set as this file if the setting is saved?

          3. Thank you and will do .

          4. I thought that 'theme_afterburner_pluginfile()' was only calling a common helper function and that helper function (theme_pluginfile()) served up files. And therefore the 'white listing' in 'theme_afterburner_pluginfile()' was already orchestrated through the statement of component and setting. Yes, conceptually, another bit of code could request transmission of the file from the component and setting. I understand what white listing is but have no idea of how to implement it here.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , Thanks RE: 1. Temporary commit message designed to be informative to the developers of this patch not meant to be a serious contender for inclusion in the main log. As I knew there was work to be done and the integrator wants only one commit when integrating, I'm going to squash the commits into one with a suitable commit message - a de-bling / de-tartify exercise. To be honest, I was being quick as I wanted to see the Grand National on the telly. 2. All we want to do is upload a single file and have it displayed with the option of removing it and getting back to the default. Why oh why is it so difficult!! Please help as the File API Guru . And should we ignore the whole draft files bit and just go for a set as this file if the setting is saved? 3. Thank you and will do . 4. I thought that 'theme_afterburner_pluginfile()' was only calling a common helper function and that helper function (theme_pluginfile()) served up files. And therefore the 'white listing' in 'theme_afterburner_pluginfile()' was already orchestrated through the statement of component and setting. Yes, conceptually, another bit of code could request transmission of the file from the component and setting. I understand what white listing is but have no idea of how to implement it here. Thanks, Gareth
          Hide
          Petr Škoda added a comment - - edited

          2. The file API+repository+filepicker guru left last year, nobody except the fearless goddess Marina dares to touch the filepicker & filemanager code now. I will bring forward this topic during the next meeting.

          4.

          function theme_afterburner_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options=array()) {
              if ($filearea === 'logo') {
                  return theme_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, 'theme_afterburner');
              } else {
                  send_file_not_found();
              }
          }
          

          Oh! I overlooked you are using the setting name for the filearea name, that is wrong imo and could cause problems in other plugins, please add filearea as new setting param. I also believe that the 'isimagefile' option is not necessary anymore because you can get the mime type from the stored file.

          Show
          Petr Škoda added a comment - - edited 2. The file API+repository+filepicker guru left last year, nobody except the fearless goddess Marina dares to touch the filepicker & filemanager code now. I will bring forward this topic during the next meeting. 4. function theme_afterburner_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options=array()) { if ($filearea === 'logo') { return theme_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, 'theme_afterburner'); } else { send_file_not_found(); } } Oh! I overlooked you are using the setting name for the filearea name, that is wrong imo and could cause problems in other plugins, please add filearea as new setting param. I also believe that the 'isimagefile' option is not necessary anymore because you can get the mime type from the stored file.
          Hide
          Gareth J Barnard added a comment -

          P.S. RE:2 - I am not slagging off Moodle in any way and not attempting to construct a 'somebody else's problem field' around it. And indeed will do as many changes to code / API code such that this is a useful and flexible addition to the Moodle functionality family. I've even read the File API docs this time . But 'help'

          Show
          Gareth J Barnard added a comment - P.S. RE:2 - I am not slagging off Moodle in any way and not attempting to construct a 'somebody else's problem field' around it. And indeed will do as many changes to code / API code such that this is a useful and flexible addition to the Moodle functionality family. I've even read the File API docs this time . But 'help'
          Hide
          Gareth J Barnard added a comment -

          Correct me if I'm wrong, but isn't $file a stream of bytes when used in 'output_html()' as in 'get_setting()':

          $file->out(false)
          

          And therefore a mime type function could not be operated upon it?

          Show
          Gareth J Barnard added a comment - Correct me if I'm wrong, but isn't $file a stream of bytes when used in 'output_html()' as in 'get_setting()': $file->out( false ) And therefore a mime type function could not be operated upon it?
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Thanks - I thought you were the File API Guru when looked at the document http://docs.moodle.org/dev/File_API_internals.

          I'll implement as much as possible and look at the file area issue as I thought it was a combination of component name and setting name.

          Also still need to fix the draft area file issue.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Thanks - I thought you were the File API Guru when looked at the document http://docs.moodle.org/dev/File_API_internals . I'll implement as much as possible and look at the file area issue as I thought it was a combination of component name and setting name. Also still need to fix the draft area file issue. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          No worries. The $file is instance of stored_file class, you can ask what mimetype it has via $file->get_mimetype().

          Show
          Petr Škoda added a comment - No worries. The $file is instance of stored_file class, you can ask what mimetype it has via $file->get_mimetype().
          Hide
          Mary Cooch added a comment -

          Adding docs_required label - I anticipate this will be a very popular feature.

          Show
          Mary Cooch added a comment - Adding docs_required label - I anticipate this will be a very popular feature.
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Lots of fixes:

          1. require_once when needed.
          2. mime type detection.
          3. 'theme_afterburner_logo' file area type naming.
          4. white list.

          But still have multiple draft file issue.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Lots of fixes: 1. require_once when needed. 2. mime type detection. 3. 'theme_afterburner_logo' file area type naming. 4. white list. But still have multiple draft file issue. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          @Gareth: That was scary!

          Everything appeared to work OK until I clicked on Home in the navbar, and got this list above the header...I've cut it short to save space here.

          http://localhost/moodle/pluginfile.php/1/theme_afterburner/theme_afterburner_logo/-1/img2.jpghttp://localhost/moodle/pluginfile.php/1/theme_afterburner/theme_afterburner_logo/-1/img2... ad infinitum...

          Show
          Mary Evans added a comment - @Gareth: That was scary! Everything appeared to work OK until I clicked on Home in the navbar, and got this list above the header...I've cut it short to save space here. http://localhost/moodle/pluginfile.php/1/theme_afterburner/theme_afterburner_logo/-1/img2.jpghttp://localhost/moodle/pluginfile.php/1/theme_afterburner/theme_afterburner_logo/-1/img2 ... ad infinitum...
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          Ok, thanks

          I need to have a think and do some research on this as I consider it's all to do with the theme version being used as a means of forcing browser update when the image changes (cache issue).

          I am also considering that perhaps the file manager might be better as that is in effect what we are doing. We want to upload and maintain a file and 'manage' it. Plus a plugin may wish to have a range of files in its area upon which to use, such as an image / css combination. But in the mean time if the file manager solves the draft file issue, then good.

          Also whatever the eventual implementation is the file does need to stay in the 'moodledata' area such that no script attack can occur as that area is not normally served by the web server unless a mistake is made on installation. But I believe there is a warning about that.

          With the draft file issue / theme version / cache issue, I am wondering if generating a draft file identifier is the way to go and then store it as another config plugin setting to be used with the cache issue in the moodle url.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Ok, thanks I need to have a think and do some research on this as I consider it's all to do with the theme version being used as a means of forcing browser update when the image changes (cache issue). I am also considering that perhaps the file manager might be better as that is in effect what we are doing. We want to upload and maintain a file and 'manage' it. Plus a plugin may wish to have a range of files in its area upon which to use, such as an image / css combination. But in the mean time if the file manager solves the draft file issue, then good. Also whatever the eventual implementation is the file does need to stay in the 'moodledata' area such that no script attack can occur as that area is not normally served by the web server unless a mistake is made on installation. But I believe there is a warning about that. With the draft file issue / theme version / cache issue, I am wondering if generating a draft file identifier is the way to go and then store it as another config plugin setting to be used with the cache issue in the moodle url. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Fixed negative item id issue, but still need to think of storing a generated 'id' for this for the cache issue. Draft file issue still remains.

          Show
          Gareth J Barnard added a comment - Fixed negative item id issue, but still need to think of storing a generated 'id' for this for the cache issue. Draft file issue still remains.
          Hide
          Gareth J Barnard added a comment -

          Ok, I think I've solved the draft file issue by not showing the current draft file name. The code does need 'cleaning' but I think its ok.

          Existing draft files are left for 'cron' to clear up. Actually, is there a way to access existing draft files in the file picker? As cannot see a way.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Ok, I think I've solved the draft file issue by not showing the current draft file name. The code does need 'cleaning' but I think its ok. Existing draft files are left for 'cron' to clear up. Actually, is there a way to access existing draft files in the file picker? As cannot see a way. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi all,

          Maybe tomorrow or on wednesday I can take some time to work with that issue. But today I can at least discuss some changes:
          1/ Why the file area is now: $this->plugin.'_'.$this->name ? I think using plugin to component and name to filearea was right! :-P

          2/ Revision on theme_get_file_from_setting is now deleted and I think the cache problems will be back, I propose:

                  $revision = theme_get_revision();
                  if($revision < 0) $revision = 0;
                  $thefile = moodle_url::make_pluginfile_url($context->id, $theme, $theme.'_'.$setting, $revision, '/', $filename);
          

          And on the get_setting function from the adminlib.php: It will have cache problems, now the get_timemodified is deleted, I think it would be right to take it back.

          $file = moodle_url::make_pluginfile_url($context->id, $this->plugin, $this->plugin.'_'.$this->name, $file->get_timemodified, '/', $filename);
          

          3/ I don't know how to solve the issue about sorting draft files, we may try to use filemanager. If you're not hurry on wednesday I'll do it.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi all, Maybe tomorrow or on wednesday I can take some time to work with that issue. But today I can at least discuss some changes: 1/ Why the file area is now: $this->plugin.'_'.$this->name ? I think using plugin to component and name to filearea was right! :-P 2/ Revision on theme_get_file_from_setting is now deleted and I think the cache problems will be back, I propose: $revision = theme_get_revision(); if ($revision < 0) $revision = 0; $thefile = moodle_url::make_pluginfile_url($context->id, $theme, $theme.'_'.$setting, $revision, '/', $filename); And on the get_setting function from the adminlib.php: It will have cache problems, now the get_timemodified is deleted, I think it would be right to take it back. $file = moodle_url::make_pluginfile_url($context->id, $ this ->plugin, $ this ->plugin.'_'.$ this ->name, $file->get_timemodified, '/', $filename); 3/ I don't know how to solve the issue about sorting draft files, we may try to use filemanager. If you're not hurry on wednesday I'll do it. Regards, Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          RE:

          1. It is now '$this->plugin.'_'.$this->name' because Petr was unhappy with just '$this->name' for the file area as that was just 'logo' resulting in a possible conflict with other file areas. Must be something to do with the design of the File API.

          2. I removed 'theme_get_file_from_setting()' because there is an inherent disparity with the number generated with '$file->get_timemodified' meaning that the item id is different when the image is displayed on the admin setting and when the theme requests it. The two numbers are not the same measure of unit. The cache issue has not come back as far as I can tell. If it does, then the same number needs to be used for both URL's as looking at the spec of 'make_pluginfile_url' this is the 'itemid'. Also I believe using 'theme_get_file_from_setting()' was the cause of Mary's '-1' issue reported above.

          3. I've fixed the issue on sorting of draft files. Please see previous comments.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , RE: 1. It is now '$this->plugin.'_'.$this->name' because Petr was unhappy with just '$this->name' for the file area as that was just 'logo' resulting in a possible conflict with other file areas. Must be something to do with the design of the File API. 2. I removed 'theme_get_file_from_setting()' because there is an inherent disparity with the number generated with '$file->get_timemodified' meaning that the item id is different when the image is displayed on the admin setting and when the theme requests it. The two numbers are not the same measure of unit. The cache issue has not come back as far as I can tell. If it does, then the same number needs to be used for both URL's as looking at the spec of 'make_pluginfile_url' this is the 'itemid'. Also I believe using 'theme_get_file_from_setting()' was the cause of Mary's '-1' issue reported above. 3. I've fixed the issue on sorting of draft files. Please see previous comments. Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hey!

          1/ Ok, It was only my curiosity

          2/ So, what do you think about my revision patch?

          3/ Yeah, I see it but this way has the issue that the previous files are not showing. Maybe we can recover the way it was in the firsts commits: writing the filename above the filepicker.

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hey! 1/ Ok, It was only my curiosity 2/ So, what do you think about my revision patch? 3/ Yeah, I see it but this way has the issue that the previous files are not showing. Maybe we can recover the way it was in the firsts commits: writing the filename above the filepicker. Regards, Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          RE:

          1. No worries .

          2. Will not work for the reasons I stated.

          3. Previous code only showed the latest draft file and no others. Plus code caused the latest draft file upload not to be used in preference for the first draft file upload. If we want to show all current draft file uploads and pick one, then that is a whole new ball game and perhaps the next step beyond this tracker.

          Cheers ,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , RE: 1. No worries . 2. Will not work for the reasons I stated. 3. Previous code only showed the latest draft file and no others. Plus code caused the latest draft file upload not to be used in preference for the first draft file upload. If we want to show all current draft file uploads and pick one, then that is a whole new ball game and perhaps the next step beyond this tracker. Cheers , Gareth
          Hide
          Gareth J Barnard added a comment -

          Actually, looking at http://docs.moodle.org/dev/File_API_internals#pluginfile.php I don't think I've got the $filearea right.

          Show
          Gareth J Barnard added a comment - Actually, looking at http://docs.moodle.org/dev/File_API_internals#pluginfile.php I don't think I've got the $filearea right.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Looking at http://docs.moodle.org/dev/File_API_internals#pluginfile.php

          Should:

          /pluginfile.php/contextid/areaname/arbitrary/params/or/dirs/filename.ext
          

          be

          /pluginfile.php/contextid/component/areaname/arbitrary/params/or/dirs/filename.ext
          

          given

          public static function make_pluginfile_url($contextid, $component, $area, $itemid, $pathname, $filename, $forcedownload = false) {
              global $CFG;
              $urlbase = "$CFG->httpswwwroot/pluginfile.php";
              if ($itemid === NULL) {
                  return self::make_file_url($urlbase, "/$contextid/$component/$area".$pathname.$filename, $forcedownload);
              } else {
                  return self::make_file_url($urlbase, "/$contextid/$component/$area/$itemid".$pathname.$filename, $forcedownload);
              }
          }
          

          ?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Looking at http://docs.moodle.org/dev/File_API_internals#pluginfile.php Should: /pluginfile.php/contextid/areaname/arbitrary/params/or/dirs/filename.ext be /pluginfile.php/contextid/component/areaname/arbitrary/params/or/dirs/filename.ext given public static function make_pluginfile_url($contextid, $component, $area, $itemid, $pathname, $filename, $forcedownload = false ) { global $CFG; $urlbase = "$CFG->httpswwwroot/pluginfile.php" ; if ($itemid === NULL) { return self::make_file_url($urlbase, "/$contextid/$component/$area" .$pathname.$filename, $forcedownload); } else { return self::make_file_url($urlbase, "/$contextid/$component/$area/$itemid" .$pathname.$filename, $forcedownload); } } ? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          And because of "I overlooked you are using the setting name for the filearea name, that is wrong imo and could cause problems in other plugins, please add filearea as new setting param" I changed the filearea name from 'logo' to 'theme_afterburner_logo', but if 'component' is already there in the 'component' field of the 'files' table, why would there be a conflict with other plugins?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , And because of "I overlooked you are using the setting name for the filearea name, that is wrong imo and could cause problems in other plugins, please add filearea as new setting param" I changed the filearea name from 'logo' to 'theme_afterburner_logo', but if 'component' is already there in the 'component' field of the 'files' table, why would there be a conflict with other plugins? Cheers, Gareth
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Gareth,

          2/ It will work, as you said we have to use the same 'mesure unit' or we could use the get_themerevision (with the patch I proposed to correct the -1 issue) on theme/lib.php and use 0 on get_setting and have the browser caching problem... or we can use the get_timemodified on both (That Petr says it's not great).

          3/ I was talking about showing the image or the filename as we did a lot before

          Regards,

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Gareth, 2/ It will work, as you said we have to use the same 'mesure unit' or we could use the get_themerevision (with the patch I proposed to correct the -1 issue) on theme/lib.php and use 0 on get_setting and have the browser caching problem... or we can use the get_timemodified on both (That Petr says it's not great). 3/ I was talking about showing the image or the filename as we did a lot before Regards, Pau
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          RE:

          2. Odd, as '0' working for me - need to test. If not working, then we need some way of storing the value generated alongside the filename such that he URL can be generated correctly in both instances.

          3. Ok, but code to display above file picker JS does that now .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , RE: 2. Odd, as '0' working for me - need to test. If not working, then we need some way of storing the value generated alongside the filename such that he URL can be generated correctly in both instances. 3. Ok, but code to display above file picker JS does that now . Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Whilst posting on the forums I noticed the file manager as shown on 'mdl_35434_fm.png' had file restriction of '1', so could this be better for the future?

          Show
          Gareth J Barnard added a comment - Whilst posting on the forums I noticed the file manager as shown on 'mdl_35434_fm.png' had file restriction of '1', so could this be better for the future?
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Just tested with theme designer off and no generated item number for theme caching as appears to be ok without it in the current state. Tested on local machine with an admin user and a student user and on a virtual machine that was not logged in.

          I think this is ready for peer review as also tarted up the code in line with coding standards. Have created single commit wip-MDL-35334_m3 for this purpose. Who's willing?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Just tested with theme designer off and no generated item number for theme caching as appears to be ok without it in the current state. Tested on local machine with an admin user and a student user and on a virtual machine that was not logged in. I think this is ready for peer review as also tarted up the code in line with coding standards. Have created single commit wip- MDL-35334 _m3 for this purpose. Who's willing? Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          Hi, rules for setting names and filearea names are different and those are separate concepts, I really think you should not be constructing the area name form anything and just let developer decide what area name should each setting use via constructor parameter. The image detection seems problematic because you can embed only limited number of image formats. In the long term we should definitely migrate this to a new file manger element that deals with one file selections, that is why I think there should not be any public API such as get_setting() and instead everything should be self contained in output_html() with some TODO explaining the problems and future directions, guessing the link to plugin file from core is wrong.

          Show
          Petr Škoda added a comment - Hi, rules for setting names and filearea names are different and those are separate concepts, I really think you should not be constructing the area name form anything and just let developer decide what area name should each setting use via constructor parameter. The image detection seems problematic because you can embed only limited number of image formats. In the long term we should definitely migrate this to a new file manger element that deals with one file selections, that is why I think there should not be any public API such as get_setting() and instead everything should be self contained in output_html() with some TODO explaining the problems and future directions, guessing the link to plugin file from core is wrong.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          If "guessing the link to plugin file from core is wrong" should we be creating a pure 'moodle_url' instead?

          I can do and will do the filearea name in the constructor.

          TBH I've no idea what to write for the 'TODO' as just learning the issues with the API this issue is flaming. And as 'public abstract function get_setting();' is in 'admin_setting' changing it to 'protected' I consider to be outside of the scope of this issue and pertains to a whole separate refactoring.

          The image detection currently operates on MIME types as requested and encapsulates all images as matches on the 'image' prefix of the standard.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , If "guessing the link to plugin file from core is wrong" should we be creating a pure 'moodle_url' instead? I can do and will do the filearea name in the constructor. TBH I've no idea what to write for the 'TODO' as just learning the issues with the API this issue is flaming. And as 'public abstract function get_setting();' is in 'admin_setting' changing it to 'protected' I consider to be outside of the scope of this issue and pertains to a whole separate refactoring. The image detection currently operates on MIME types as requested and encapsulates all images as matches on the 'image' prefix of the standard. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          The problems with links is that only the owner of file (plugin here) is allowed to create the pluginfile.php links to files, core (setting here) should be using only draft files.

          Show
          Petr Škoda added a comment - The problems with links is that only the owner of file (plugin here) is allowed to create the pluginfile.php links to files, core (setting here) should be using only draft files.
          Hide
          Petr Škoda added a comment -

          I am trying to come up with some design based on filemanager element, I will post here in few hours...

          Show
          Petr Škoda added a comment - I am trying to come up with some design based on filemanager element, I will post here in few hours...
          Hide
          Petr Škoda added a comment -

          perfect! filemanager supports "1 file limit" now...

          Show
          Petr Škoda added a comment - perfect! filemanager supports "1 file limit" now...
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          So, how do I change the code in 'admin_setting_configfilepicker' given that there is a need to display the image in the setting? So, if I use a draft file referencing moodle url in the 'get_setting()' method then this is initially fine as the draft file entry exists in the 'files' table. However when the cron job operates and removes the draft files (as previously hinted at) then the image url is no longer valid to the draft file area even though the setting knows where it is in the plugin area. And the image is no longer displayed and we are left with the situation where the setting can only legitimately display the filename of the image as that is possible without the need to construct a URL - but giving loss of functionality to the end user.

          However, I could argue that ownership of the setting object in question is actually the plugin (theme) as it is contained within it's own 'settings.php' file:

          $setting = new admin_setting_configfilepicker($name, $title, $description, null, 'theme_afterburner_logo', $options);
          

          such that the instantiation of the object is within the scope of the plugin (theme) and not core on a level one above that implied by '/lib/adminlib.php' being in core. Therefore it could be stated that the owner of the file (the theme) by implied action is calling 'get_setting()' for a pluginfile.php URL for which it is entitled to do so under the rules. Because 'admin_setting_configfilepicker' is a helper class working for the plugin. And reuse of 'admin_setting_configfilepicker' will be for any other plugin unless there are plans afoot for this to be used within core. If so, then there needs to be some way of using 'moodle_url' in both instances as I believe at one point in the plethora of development commits Pau Ferrer Ocaña (crazyserver) had.

          Cheers,

          Gareth

          P.S. Just done the constructor change

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , So, how do I change the code in 'admin_setting_configfilepicker' given that there is a need to display the image in the setting? So, if I use a draft file referencing moodle url in the 'get_setting()' method then this is initially fine as the draft file entry exists in the 'files' table. However when the cron job operates and removes the draft files (as previously hinted at) then the image url is no longer valid to the draft file area even though the setting knows where it is in the plugin area. And the image is no longer displayed and we are left with the situation where the setting can only legitimately display the filename of the image as that is possible without the need to construct a URL - but giving loss of functionality to the end user. However, I could argue that ownership of the setting object in question is actually the plugin (theme) as it is contained within it's own 'settings.php' file: $setting = new admin_setting_configfilepicker($name, $title, $description, null , 'theme_afterburner_logo', $options); such that the instantiation of the object is within the scope of the plugin (theme) and not core on a level one above that implied by '/lib/adminlib.php' being in core. Therefore it could be stated that the owner of the file (the theme) by implied action is calling 'get_setting()' for a pluginfile.php URL for which it is entitled to do so under the rules. Because 'admin_setting_configfilepicker' is a helper class working for the plugin. And reuse of 'admin_setting_configfilepicker' will be for any other plugin unless there are plans afoot for this to be used within core. If so, then there needs to be some way of using 'moodle_url' in both instances as I believe at one point in the plethora of development commits Pau Ferrer Ocaña (crazyserver) had. Cheers, Gareth P.S. Just done the constructor change
          Hide
          Gareth J Barnard added a comment -

          P.P.S. Thanks for a possible file manager solution Petr Škoda

          Show
          Gareth J Barnard added a comment - P.P.S. Thanks for a possible file manager solution Petr Škoda
          Hide
          Petr Škoda added a comment -

          I did not implement the file storage yet, but here is the visual part of my attempt.

          • standard file manager element
          • limited to 1 file
          • build-in preview
          • intuitive delete
          Show
          Petr Škoda added a comment - I did not implement the file storage yet, but here is the visual part of my attempt. standard file manager element limited to 1 file build-in preview intuitive delete
          Hide
          Gareth J Barnard added a comment -

          Petr Škoda Cool - If you wanted to do a GitHub pull request on the branch, please be aware I've just squashed and rebased wip-MDL-35434_m3 for the constructor filearea change.

          Show
          Gareth J Barnard added a comment - Petr Škoda Cool - If you wanted to do a GitHub pull request on the branch, please be aware I've just squashed and rebased wip- MDL-35434 _m3 for the constructor filearea change.
          Hide
          Mary Evans added a comment - - edited

          Hi Petr,

          Thanks for your time helping solve this riddle.

          Can I ask one question?

          Will the changes you are implementing allow for images to be added via the admin_setting_confightmleditor?

          I only ask as this calls up the filepicker, but that is where it ends.

          If the answer is YES, then this will solve other problems with the admin settings too.

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited Hi Petr, Thanks for your time helping solve this riddle. Can I ask one question? Will the changes you are implementing allow for images to be added via the admin_setting_confightmleditor? I only ask as this calls up the filepicker, but that is where it ends. If the answer is YES, then this will solve other problems with the admin settings too. Cheers Mary
          Hide
          Petr Škoda added a comment -

          Oh no Marry, html editor is a completely different problem, that would be a lot more difficult. Please use standard forms for that if necessary.

          Show
          Petr Škoda added a comment - Oh no Marry, html editor is a completely different problem, that would be a lot more difficult. Please use standard forms for that if necessary.
          Hide
          Petr Škoda added a comment -

          Here is my attempt at this: https://github.com/skodak/moodle/compare/w15_MDL-35434_m25_storedfilesetting

          So far it seems to work fine, but I did not test it much.

          Show
          Petr Škoda added a comment - Here is my attempt at this: https://github.com/skodak/moodle/compare/w15_MDL-35434_m25_storedfilesetting So far it seems to work fine, but I did not test it much.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Interesting - Clever use of refactoring on post write update method to facilitate overloading in the new class. Is this for the setting of the theme reset cache event? If so, I assume that this still need to be set in 'settings.php'. And I cannot see where 'protected $fileupdatecallback;' is used as '$this->updatedcallback' is used?

          I will have a go at an integrated version with the Afterburner theme.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Interesting - Clever use of refactoring on post write update method to facilitate overloading in the new class. Is this for the setting of the theme reset cache event? If so, I assume that this still need to be set in 'settings.php'. And I cannot see where 'protected $fileupdatecallback;' is used as '$this->updatedcallback' is used? I will have a go at an integrated version with the Afterburner theme. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          Ooops, thanks I have amended the commit (unused property removed, more docs). Yes it should be used in a similar way as the original filepicker-based setting (including the callback for theme reset).

          Show
          Petr Škoda added a comment - Ooops, thanks I have amended the commit (unused property removed, more docs). Yes it should be used in a similar way as the original filepicker-based setting (including the callback for theme reset).
          Hide
          Petr Škoda added a comment -

          I was thinking also about the upgrade, it should be technically possible to download the image content using url in upgrade script and store it as the new setting+storedfile. Maybe we should get in the setting class first and then start updating themes in separate issue - up to you...

          Show
          Petr Škoda added a comment - I was thinking also about the upgrade, it should be technically possible to download the image content using url in upgrade script and store it as the new setting+storedfile. Maybe we should get in the setting class first and then start updating themes in separate issue - up to you...
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Cheers

          I think the key thing here is the demand for the setting from an API point of view. This to me means:

          1. Usable.
          2. Well thought out interface that will not change even with removal of future bugs.
          3. Supply one working example for others to follow as a proof of concept, then turn the handle later.

          So, I believe that we should get in the best solution as quickly as possible. Also, given the potential demand request that once integrated and proven that it be back-ported. I know that improvements are not normally back-ported but in reading the documentation they can be if desired.

          Looking at your screen shot, I consider the file manager solution to be the most intuitive from a user's point of view. I additionally consider that there should be validation to prevent the 'typo' type errors that can happen however experienced an admin you are.

          I value your, Mary Evans and Pau Ferrer Ocaña (crazyserver)'s opinion on this.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Cheers I think the key thing here is the demand for the setting from an API point of view. This to me means: 1. Usable. 2. Well thought out interface that will not change even with removal of future bugs. 3. Supply one working example for others to follow as a proof of concept, then turn the handle later. So, I believe that we should get in the best solution as quickly as possible. Also, given the potential demand request that once integrated and proven that it be back-ported. I know that improvements are not normally back-ported but in reading the documentation they can be if desired. Looking at your screen shot, I consider the file manager solution to be the most intuitive from a user's point of view. I additionally consider that there should be validation to prevent the 'typo' type errors that can happen however experienced an admin you are. I value your, Mary Evans and Pau Ferrer Ocaña (crazyserver) 's opinion on this. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          Should I make a new issue and submit my branch for integration?

          Show
          Petr Škoda added a comment - Should I make a new issue and submit my branch for integration?
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          I don't believe that is the solution. Rather that mine, Pau's and your code should be integrated into a complete solution with the Afterburner changes.

          I know that you and Pau Ferrer Ocaña (crazyserver) have done most of the work. So would only like a mention for assistance and creation of the theme root lib.php code. If you consider that taking over this tracker is appropriate rather than duplicating another tracker issue, then do so. All I ask is that it is definitely a 'complete solution' and that both Mary Evans and Pau Ferrer Ocaña (crazyserver) is happy with the solution.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , I don't believe that is the solution. Rather that mine, Pau's and your code should be integrated into a complete solution with the Afterburner changes. I know that you and Pau Ferrer Ocaña (crazyserver) have done most of the work. So would only like a mention for assistance and creation of the theme root lib.php code. If you consider that taking over this tracker is appropriate rather than duplicating another tracker issue, then do so. All I ask is that it is definitely a 'complete solution' and that both Mary Evans and Pau Ferrer Ocaña (crazyserver) is happy with the solution. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          P.S. However, if you consider that we need both file picker and file manager then it would be appropriate to have both, but then surely your code would depend in some part on this tracker's code?

          Show
          Gareth J Barnard added a comment - P.S. However, if you consider that we need both file picker and file manager then it would be appropriate to have both, but then surely your code would depend in some part on this tracker's code?
          Hide
          Petr Škoda added a comment - - edited

          I am confused, my code for the new setting is original, self-contained and completely separate from themes, it can be used from any other plugin type. I created it only because nobody seemed to be able to understand my objections and recommendation. Feel free to cherry pick it into your branch, or create a new issue for me with a name "New admin setting for stored file", please note that it is the recommended way because you should not mix core changes with plugin modification in one PULL request. In any case thanks everybody for your work on themes!!

          here is my branch again so that you do not need to scroll up to review it: https://github.com/skodak/moodle/compare/w15_MDL-35434_m25_storedfilesetting

          Show
          Petr Škoda added a comment - - edited I am confused, my code for the new setting is original, self-contained and completely separate from themes, it can be used from any other plugin type. I created it only because nobody seemed to be able to understand my objections and recommendation. Feel free to cherry pick it into your branch, or create a new issue for me with a name "New admin setting for stored file", please note that it is the recommended way because you should not mix core changes with plugin modification in one PULL request. In any case thanks everybody for your work on themes!! here is my branch again so that you do not need to scroll up to review it: https://github.com/skodak/moodle/compare/w15_MDL-35434_m25_storedfilesetting
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          Sorry for the confusion, not intended. I had not realised it's self containment. I also had not even considered that there was a differential separation between a core theme and core API.

          I also thought that to be complete, usable and demonstrable with minimal code effort on the tester that the Afterburner code should be amended as it is a candidate for demonstrating that the improvement works. Also as a template for others to follow.

          Regarding "I created it only because nobody seemed to be able to understand my objections and recommendation", I'm still learning the nuances of PHP and the Moodle API. There is a lot to take in and learn, so please be patient with me. I come from a Java / C++ type defined OO background so sometimes this makes PHP harder to read and comprehend. And when you don't completely see the whole contents of the scope of inclusion when scripts load other scripts.

          I honestly don't know what is the best way forward with this now. As you do have a longer and more comprehensive knowledge of the machinations of Moodle, please advise .

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , Sorry for the confusion, not intended. I had not realised it's self containment. I also had not even considered that there was a differential separation between a core theme and core API. I also thought that to be complete, usable and demonstrable with minimal code effort on the tester that the Afterburner code should be amended as it is a candidate for demonstrating that the improvement works. Also as a template for others to follow. Regarding "I created it only because nobody seemed to be able to understand my objections and recommendation", I'm still learning the nuances of PHP and the Moodle API. There is a lot to take in and learn, so please be patient with me. I come from a Java / C++ type defined OO background so sometimes this makes PHP harder to read and comprehend. And when you don't completely see the whole contents of the scope of inclusion when scripts load other scripts. I honestly don't know what is the best way forward with this now. As you do have a longer and more comprehensive knowledge of the machinations of Moodle, please advise . Thanks, Gareth
          Hide
          Petr Škoda added a comment -

          I am very glad that more people are working on problems like this these says. I was getting impatient because it is after freeze and there is not much time left.

          I propose following:
          1/ You can try my setting together with the rest of your current code.
          2/ If it works for you I will review your theme code after you publish your branch on top of my commit.
          3/ Others are of course welcome to try it out in other themes/plugins too.
          4/ Finally I will create two PULL requests and I will make sure it gets through integration ASAP. (git commits track the authors, the theme git commit messages should include most active participans).

          We must make a final decision before Friday. Ok?

          Show
          Petr Škoda added a comment - I am very glad that more people are working on problems like this these says. I was getting impatient because it is after freeze and there is not much time left. I propose following: 1/ You can try my setting together with the rest of your current code. 2/ If it works for you I will review your theme code after you publish your branch on top of my commit. 3/ Others are of course welcome to try it out in other themes/plugins too. 4/ Finally I will create two PULL requests and I will make sure it gets through integration ASAP. (git commits track the authors, the theme git commit messages should include most active participans). We must make a final decision before Friday. Ok?
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          Thank you. I had also not realised that there was a deadline, as the code freeze was in place and this was for Moodle 2.6.

          I will do my best to undertake the actions by Friday if not sooner, but life is life and cannot be entirely predicted.

          To clarify, I need to 'pull' in your branch to test but only commit the code that uses it? Then you do the dual pull? Or is it easier to combine the code but have the dual pull for the log.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , Thank you. I had also not realised that there was a deadline, as the code freeze was in place and this was for Moodle 2.6. I will do my best to undertake the actions by Friday if not sooner, but life is life and cannot be entirely predicted. To clarify, I need to 'pull' in your branch to test but only commit the code that uses it? Then you do the dual pull? Or is it easier to combine the code but have the dual pull for the log. Thanks, Gareth
          Hide
          Mary Evans added a comment -

          If this gets through for 2.5 then I can use in Simple (Bootstrap) theme MDL-39021 which is hopefully being peer reviewed.

          Show
          Mary Evans added a comment - If this gets through for 2.5 then I can use in Simple (Bootstrap) theme MDL-39021 which is hopefully being peer reviewed.
          Hide
          Petr Škoda added a comment -

          Gareth: checkout my branch and cherry pick your stuff on top of it and amend it, it is not going to involve any git pulls.

          Show
          Petr Škoda added a comment - Gareth: checkout my branch and cherry pick your stuff on top of it and amend it, it is not going to involve any git pulls.
          Hide
          Petr Škoda added a comment - - edited

          I have tried to implement the theme setting file support as small as possible + added afterburner conversion. Please note upgrade of older setting is not included.

          https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting

          Does it make sense?

          Show
          Petr Škoda added a comment - - edited I have tried to implement the theme setting file support as small as possible + added afterburner conversion. Please note upgrade of older setting is not included. https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting Does it make sense?
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Sorry for the delayed reply. I have some domestic arrangements to do so will be quick.

          A few questions:

          1. Now that you have done this, do I need to do anything bar review / test your code? As I believe you have a better implementation in terms of PHP than I can currently do at the moment. I'm not sure about Pau.
          2. I am concerned about the use of '$itemid = theme_get_revision();' as I think it will not work in relation to showing the current displayed image in the file manager. i.e.
          2.1 What happens to the old setting file when a new file is uploaded and the revision is incremented?
          2.2 Is the image still displayed in the file manager window once cron has run and the draft files have been eliminated?
          3. I have no problem with the older setting being upgraded unless this is the current released setting (and not the tinkered one in this patch) and it would lead to orphan data in the database.

          I will test and look at this further as soon as I finish my domestic issues.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Sorry for the delayed reply. I have some domestic arrangements to do so will be quick. A few questions: 1. Now that you have done this, do I need to do anything bar review / test your code? As I believe you have a better implementation in terms of PHP than I can currently do at the moment. I'm not sure about Pau. 2. I am concerned about the use of '$itemid = theme_get_revision();' as I think it will not work in relation to showing the current displayed image in the file manager. i.e. 2.1 What happens to the old setting file when a new file is uploaded and the revision is incremented? 2.2 Is the image still displayed in the file manager window once cron has run and the draft files have been eliminated? 3. I have no problem with the older setting being upgraded unless this is the current released setting (and not the tinkered one in this patch) and it would lead to orphan data in the database. I will test and look at this further as soon as I finish my domestic issues. Thanks, Gareth
          Hide
          Petr Škoda added a comment - - edited

          1. Thanks. Let's wait two days and see if anybody comes up with any problems or better ideas.
          2. File manager is using draft files area with forcedownload which prevents any caching, it is using draftfile.php, not pluginfile.php
          2.1 Ooops, you are right, I added 0 lifetime when theme rev is -1
          2.2 New draft area is created for each edit (of forms or settings), the areas are purged after a few days.

          branch updated: https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting

          Show
          Petr Škoda added a comment - - edited 1. Thanks. Let's wait two days and see if anybody comes up with any problems or better ideas. 2. File manager is using draft files area with forcedownload which prevents any caching, it is using draftfile.php, not pluginfile.php 2.1 Ooops, you are right, I added 0 lifetime when theme rev is -1 2.2 New draft area is created for each edit (of forms or settings), the areas are purged after a few days. branch updated: https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting
          Hide
          Gareth J Barnard added a comment -

          P.S.

          Why is itemid...

          In 'post_write_settings' $this->itemid, but in 'setting_file_url' theme_get_revision() and in 'setting_file_serve' 0? As in the DB it would be one of the values for the theme's file area.

          Show
          Gareth J Barnard added a comment - P.S. Why is itemid... In 'post_write_settings' $this->itemid, but in 'setting_file_url' theme_get_revision() and in 'setting_file_serve' 0? As in the DB it would be one of the values for the theme's file area.
          Hide
          Gareth J Barnard added a comment -

          P.P.S Off to to domestic stuff back as soon as can.

          Show
          Gareth J Barnard added a comment - P.P.S Off to to domestic stuff back as soon as can.
          Hide
          Petr Škoda added a comment -

          The setting class allows you to specify itemid != 0 which may be theoretically useful in other plugins. The theme files expect itemid == 0 because they abuse it for cache invalidation. It would be also possible to just prepend the revision before the filepath and strip it later, we use similar approach in other areas.

          Show
          Petr Škoda added a comment - The setting class allows you to specify itemid != 0 which may be theoretically useful in other plugins. The theme files expect itemid == 0 because they abuse it for cache invalidation. It would be also possible to just prepend the revision before the filepath and strip it later, we use similar approach in other areas.
          Hide
          Pau Ferrer Ocaña (crazyserver) added a comment -

          Hi all!

          Sorry, but I'm really busy this days... Petr solution sounds great! I think it can replace our solution because having both it will be a waist of resources to win nothing...

          I'm trying to test it but I don't have more time to look at it and the moodle on the branch https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting does not install... I'll try it later.

          Ciao!

          Pau

          Show
          Pau Ferrer Ocaña (crazyserver) added a comment - Hi all! Sorry, but I'm really busy this days... Petr solution sounds great! I think it can replace our solution because having both it will be a waist of resources to win nothing... I'm trying to test it but I don't have more time to look at it and the moodle on the branch https://github.com/skodak/moodle/compare/95cd436...w15_MDL-35434_m25_storedfilesetting does not install... I'll try it later. Ciao! Pau
          Hide
          Petr Škoda added a comment -

          In recent installs you may need to purge your dataroot or at least the MUC cache dirs.

          Show
          Petr Škoda added a comment - In recent installs you may need to purge your dataroot or at least the MUC cache dirs.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Thanks for the itemid explanation. I think I need to re-read the code to be sure I understand it

          Anyway, I've been able to checkout your branch and do some testing. Seems to work the same as before except that you have to delete the old image before you use a new one rather than overwriting the old one.

          Also, when the 'user/draft' entry is removed in the 'files' table (as it would be on a cron job) leaving the 'theme_afterburner/logo' entry and the setting's page is viewed (need to check outside of the settings page too), then the new logo disappears and reverts back to the default!

          Plus there seems to be redundant set of 'core/preview' entries in the 'files' table stacking up when the image is changed. Unless these are removed on cron too?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Thanks for the itemid explanation. I think I need to re-read the code to be sure I understand it Anyway, I've been able to checkout your branch and do some testing. Seems to work the same as before except that you have to delete the old image before you use a new one rather than overwriting the old one. Also, when the 'user/draft' entry is removed in the 'files' table (as it would be on a cron job) leaving the 'theme_afterburner/logo' entry and the setting's page is viewed (need to check outside of the settings page too), then the new logo disappears and reverts back to the default! Plus there seems to be redundant set of 'core/preview' entries in the 'files' table stacking up when the image is changed. Unless these are removed on cron too? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Ok, just tested and the logo only disappears on all screens if the user draft file entry no longer exists (but the theme_afterburner logo does) when the admin revisits the settings page. Therefore, this is a bug to fix please

          Show
          Gareth J Barnard added a comment - Ok, just tested and the logo only disappears on all screens if the user draft file entry no longer exists (but the theme_afterburner logo does) when the admin revisits the settings page. Therefore, this is a bug to fix please
          Hide
          Petr Škoda added a comment -

          Cron should not delete any active draft area, it is looking for older areas only. (I hope nobody changed the draftarea logic afterwards, if yes it is a HUGE problem.) The preview is another feature which should "just work", if not it is a bug, I did not study that code yet, sorry.

          Show
          Petr Škoda added a comment - Cron should not delete any active draft area, it is looking for older areas only. (I hope nobody changed the draftarea logic afterwards, if yes it is a HUGE problem.) The preview is another feature which should "just work", if not it is a bug, I did not study that code yet, sorry.
          Hide
          Petr Škoda added a comment - - edited

          I looked at the code, file_prepare_draft_area() uses current time as area timestamp, file_storage::cron() should be deleting only draft areas older than 4 days. If anything deletes a draft area in less than 4 days it is a BUG. The previews are deleted when they are no longer referenced by any content hash.

          Show
          Petr Škoda added a comment - - edited I looked at the code, file_prepare_draft_area() uses current time as area timestamp, file_storage::cron() should be deleting only draft areas older than 4 days. If anything deletes a draft area in less than 4 days it is a BUG. The previews are deleted when they are no longer referenced by any content hash.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Why are you apologising! This is software development

          To be honest I did a simulated test of cron removing the draft area file entries in the database as we don't have time to wait for four days. However, if you believe that it will delete the draft area files after four days, then when you return to the theme settings then the uploaded logo is lost.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Why are you apologising! This is software development To be honest I did a simulated test of cron removing the draft area file entries in the database as we don't have time to wait for four days. However, if you believe that it will delete the draft area files after four days, then when you return to the theme settings then the uploaded logo is lost. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Pau Ferrer Ocaña (crazyserver),

          If you add 'git://github.com/skodak/moodle.git' as a remote on your master development area then you should be able to fetch and checkout the 'w15_MDL-35434_m25_storedfilesetting' branch with your current master install then everything should be ok to test.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Pau Ferrer Ocaña (crazyserver) , If you add 'git://github.com/skodak/moodle.git' as a remote on your master development area then you should be able to fetch and checkout the 'w15_ MDL-35434 _m25_storedfilesetting' branch with your current master install then everything should be ok to test. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          I have tweaked my first commit to include a nasty hack that detects randomly purged draft areas, ciao, time for swimming here.

          Show
          Petr Škoda added a comment - I have tweaked my first commit to include a nasty hack that detects randomly purged draft areas, ciao, time for swimming here.
          Hide
          Petr Škoda added a comment -

          Hmmm, we could probably fine tune the draftarea expiration time using session timeout and session table data, that should eliminate all forms issues too.

          Show
          Petr Škoda added a comment - Hmmm, we could probably fine tune the draftarea expiration time using session timeout and session table data, that should eliminate all forms issues too.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          I'm going to test the latest changes. TBH, whatever the timeout, then I don't think that matters so much as the file manager showing the current incarnation of the logo whenever the settings page is visited.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , I'm going to test the latest changes. TBH, whatever the timeout, then I don't think that matters so much as the file manager showing the current incarnation of the logo whenever the settings page is visited. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Cool , the hack appears to work with the settings .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Cool , the hack appears to work with the settings . Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Petr Škoda,

          Given the nature of '$theme->setting_file_serve()', should "Please note you need to implement your own pluginfile.php callback" read "Please note you need to implement your own '_pluginfile' callback function". As it is really now not so obvious that 'pluginfile.php' is being used? And indeed 'setting_file_serve()' does not use 'pluginfile.php'. But it is used by 'setting_file_url()' but then it is not called by the callback function but by the css pre-processor for the theme.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Petr Škoda , Given the nature of '$theme->setting_file_serve()', should "Please note you need to implement your own pluginfile.php callback" read "Please note you need to implement your own '_pluginfile' callback function". As it is really now not so obvious that 'pluginfile.php' is being used? And indeed 'setting_file_serve()' does not use 'pluginfile.php'. But it is used by 'setting_file_url()' but then it is not called by the callback function but by the css pre-processor for the theme. Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          done, thanks

          Show
          Petr Škoda added a comment - done, thanks
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Should "// Filemanager form element implementation is far from optimal, we need to rework this if we ever fix it..." end in a ':' as it is the start of an indication of code that is to follow? Oh, wait! It's against colons in 'code checker'!

          Seriously, this is awesome work, and I'd be willing to help with the API documentation. As I think expanding the comment '_pluginfile' to say that it needs to be in 'lib.php' is too far for comments in code. But a full Moodle docs explanation would be worthwhile.

          Also, should there be a tracker on the Afterburner Theme to say that the 'logo' change would be better implemented by a 'file manager' and that it is implemented in this tracker to fall in line with policy?

          And! Would you like to take this tracker over and change the pull details?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Should "// Filemanager form element implementation is far from optimal, we need to rework this if we ever fix it..." end in a ':' as it is the start of an indication of code that is to follow? Oh, wait! It's against colons in 'code checker'! Seriously, this is awesome work, and I'd be willing to help with the API documentation. As I think expanding the comment '_pluginfile' to say that it needs to be in 'lib.php' is too far for comments in code. But a full Moodle docs explanation would be worthwhile. Also, should there be a tracker on the Afterburner Theme to say that the 'logo' change would be better implemented by a 'file manager' and that it is implemented in this tracker to fall in line with policy? And! Would you like to take this tracker over and change the pull details? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          In 'admin_setting_configstoredfile' should 'post_write_settings' do the same as the parent class:

          // Comparison must work for arrays too.
          if (serialize($original) === serialize($this->get_setting())) {
              return false;
          }
          

          ?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , In 'admin_setting_configstoredfile' should 'post_write_settings' do the same as the parent class: // Comparison must work for arrays too. if (serialize($original) === serialize($ this ->get_setting())) { return false ; } ? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          Looking at:

          $logo = $theme->setting_file_url('logo', 'logo');
          

          That '$setting' is in fact the processed internal version of:

          $name = 'theme_afterburner/logo';
          ...
          $setting = new admin_setting_configstoredfile($name, $title, $description, 'logo');
          

          But the file area stays the same. As a point of note with Object Orientation should the caller (i.e. the theme) not know about the internal processing on the '$name' parameter as it considers the value of the '$setting' to be the setting name which is actually 'theme_afterburner/logo'. So, this requires careful documentation as might not be apparent to a developer with an OO brain.

          Just a thought , cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , Looking at: $logo = $theme->setting_file_url('logo', 'logo'); That '$setting' is in fact the processed internal version of: $name = 'theme_afterburner/logo'; ... $setting = new admin_setting_configstoredfile($name, $title, $description, 'logo'); But the file area stays the same. As a point of note with Object Orientation should the caller (i.e. the theme) not know about the internal processing on the '$name' parameter as it considers the value of the '$setting' to be the setting name which is actually 'theme_afterburner/logo'. So, this requires careful documentation as might not be apparent to a developer with an OO brain. Just a thought , cheers, Gareth
          Hide
          Petr Škoda added a comment -

          Hehe, Moodle is unique any way you look at it. The 'theme_afterburner/logo' is my brutal hack that worked around the missing component parameter in settings constructor, previously plugins could not have settings at all.

          The serialize was moved around, there are no objects with references there, so it should be equivalent (I tested it and it works for me).

          It would be great if anybody written a detailed page explaining how to add stored files to themes.

          Show
          Petr Škoda added a comment - Hehe, Moodle is unique any way you look at it. The 'theme_afterburner/logo' is my brutal hack that worked around the missing component parameter in settings constructor, previously plugins could not have settings at all. The serialize was moved around, there are no objects with references there, so it should be equivalent (I tested it and it works for me). It would be great if anybody written a detailed page explaining how to add stored files to themes.
          Hide
          Gareth J Barnard added a comment -

          Dear Petr Škoda,

          I suppose it's all about creating clean usable code that fits in with the system it is for. So, I'd be willing to have a crack at such a detailed page having followed this development.

          Are we on track for a Friday integration?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Petr Škoda , I suppose it's all about creating clean usable code that fits in with the system it is for. So, I'd be willing to have a crack at such a detailed page having followed this development. Are we on track for a Friday integration? Cheers, Gareth
          Hide
          Mary Evans added a comment -

          Just saw your post in the forum.
          Sorry I have been busy with other things Moodle and missed the progress here.

          I've set this for integration review, so can you update your Github and rebase this branch?

          Well done and grateful thanks to Pau and Gareth for their part in this, and special thanks to Petr for making this possible.

          Show
          Mary Evans added a comment - Just saw your post in the forum. Sorry I have been busy with other things Moodle and missed the progress here. I've set this for integration review, so can you update your Github and rebase this branch? Well done and grateful thanks to Pau and Gareth for their part in this, and special thanks to Petr for making this possible.
          Hide
          Dan Poltawski added a comment -

          This new feature has arrived during the feature freeze, so I was am adding the integration_held tag.

          But, I see this issue has a lot of votes and watchers (and i'm a glory seeker ) so i've pinged Martin to see if he wants to give a special dispensation to consider this for 2.5. If so, please remove the integration_held label.

          Show
          Dan Poltawski added a comment - This new feature has arrived during the feature freeze, so I was am adding the integration_held tag. But, I see this issue has a lot of votes and watchers (and i'm a glory seeker ) so i've pinged Martin to see if he wants to give a special dispensation to consider this for 2.5. If so, please remove the integration_held label.
          Hide
          Martin Dougiamas added a comment -

          +10 I think we could squeeze this in because it helps all the new theme focus in 2.5.

          I'm just a little concerned about upgrading afterburner this way because it might surprise people who had set logos the old way. I would leave that bit out.

          Petr could you help by porting the conversion to the new simple theme? MDL-39021

          Another important question: will the pluginfile callback be inherited in child themes?

          Show
          Martin Dougiamas added a comment - +10 I think we could squeeze this in because it helps all the new theme focus in 2.5. I'm just a little concerned about upgrading afterburner this way because it might surprise people who had set logos the old way. I would leave that bit out. Petr could you help by porting the conversion to the new simple theme? MDL-39021 Another important question: will the pluginfile callback be inherited in child themes?
          Hide
          Petr Škoda added a comment -

          Hello, ooops I wanted to do more tweaks today before submitting it for integration.

          Theme settings are not inherited, file callbacks are not inherited either. Child themes need to add own settings and copy those few lines in lib.php for file handling.

          Show
          Petr Škoda added a comment - Hello, ooops I wanted to do more tweaks today before submitting it for integration. Theme settings are not inherited, file callbacks are not inherited either. Child themes need to add own settings and copy those few lines in lib.php for file handling.
          Hide
          Petr Škoda added a comment -

          done, upgrade implemented now

          Show
          Petr Škoda added a comment - done, upgrade implemented now
          Hide
          Mary Evans added a comment -

          Just correcting a typo in the description.
          I hope this get through to 2.5

          Show
          Mary Evans added a comment - Just correcting a typo in the description. I hope this get through to 2.5
          Hide
          Mary Evans added a comment -

          @Martin: I have added the this new logo setting to the Simple (bootstrap) theme, but commented it out as I was not sure what the state of play for this was. Removing the comments can be done easy enough during integration if this new feature is integrated too.

          Show
          Mary Evans added a comment - @Martin: I have added the this new logo setting to the Simple (bootstrap) theme, but commented it out as I was not sure what the state of play for this was. Removing the comments can be done easy enough during integration if this new feature is integrated too.
          Hide
          Mary Evans added a comment - - edited

          I'm just testing this and all seems to work OK adding/uploading logo.jpg, however, I'm having a problem getting it to show up in Simple theme's header. I'm using the following code to add the image in general.php.

          <a href="<?php $CFG->wwwroot ?>" title="<?php print_string('home'); ?>">
              <img src="<?php echo $PAGE->theme->settings->logo; ?>" alt="<?php print_string('logo', 'theme_simple'); ?>">
          </a>
          

          When looking at the page in Firebug, I can see this:

          <a id="yui_3_9_1_3_1365957544548_9" title="Home" href="">
          <img id="yui_3_9_1_3_1365957544548_8" alt="Logo" src="/logo.jpg">
          </a>
          

          So it looks like "/logo.php" seems to be missing the URL

          HELP!

          Show
          Mary Evans added a comment - - edited I'm just testing this and all seems to work OK adding/uploading logo.jpg, however, I'm having a problem getting it to show up in Simple theme's header. I'm using the following code to add the image in general.php. <a href= "<?php $CFG->wwwroot ?>" title= "<?php print_string('home'); ?>" > <img src= "<?php echo $PAGE->theme->settings->logo; ?>" alt= "<?php print_string('logo', 'theme_simple'); ?>" > </a> When looking at the page in Firebug, I can see this: <a id= "yui_3_9_1_3_1365957544548_9" title= "Home" href=""> <img id= "yui_3_9_1_3_1365957544548_8" alt= "Logo" src= "/logo.jpg" > </a> So it looks like "/logo.php" seems to be missing the URL HELP!
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Mary,
          Look at how it is done in theme/afterburner/lib.php
          you need to call $theme->setting_file_url('logo', 'logo'); to get the image url
          And don't forget to create a new function in theme/simple/lib.php

          function theme_simple_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options = array()) {
              if ($context->contextlevel == CONTEXT_SYSTEM and $filearea === 'logo') {
                  $theme = theme_config::load('simple');
                  return $theme->setting_file_serve('logo', $args, $forcedownload, $options);
              } else {
                  send_file_not_found();
              }
          }
          

          because if you don't the url will be correct but the file will not be served and you will get a broken image

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Mary, Look at how it is done in theme/afterburner/lib.php you need to call $theme->setting_file_url('logo', 'logo'); to get the image url And don't forget to create a new function in theme/simple/lib.php function theme_simple_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options = array()) { if ($context->contextlevel == CONTEXT_SYSTEM and $filearea === 'logo') { $theme = theme_config::load('simple'); return $theme->setting_file_serve('logo', $args, $forcedownload, $options); } else { send_file_not_found(); } } because if you don't the url will be correct but the file will not be served and you will get a broken image
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          As Jean-Michel says, you need the code in '/theme/simple/lib.php':

          function theme_simple_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options = array()) {
              if ($context->contextlevel == CONTEXT_SYSTEM and $filearea === 'logo') {
                  $theme = theme_config::load('simple');
                  return $theme->setting_file_serve('logo', $args, $forcedownload, $options);
              } else {
                  send_file_not_found();
              }
          }
          

          But that is only the code that runs on the server when the file is requested by the URL called by the css in the browser. The actual setting of the URL is in the css via the pre-processing mechanism, so in '/theme/simple/lib.php' you need:

          function simple_process_css($css, $theme) {
          
              // Set the background image for the logo
              $logo = $theme->setting_file_url('logo', 'logo');
              $css = simple_set_logo($css, $logo);
          
          ....
          
          function simple_set_logo($css, $logo) {
              global $OUTPUT;
              $tag = '[[setting:logo]]';
              $replacement = $logo;
              if (is_null($replacement)) {
                  $replacement = $OUTPUT->pix_url('images/logo','theme');
              }
          
              $css = str_replace($tag, $replacement, $css);
          
              return $css;
          }
          

          where in the css you would have (as an example taken from 'afterburner'):

          a.logo {
              background: url([[setting:logo]]) no-repeat 0 0;
              width: 320px;
              height: 75px;
              display: block;
              margin: 15px 10px 10px;
              float: left;
          }
          

          using the standard css url getting for the image.

          And assuming that in '/theme/simple/config.php' you have:

          $THEME->csspostprocess = 'simple_process_css';
          

          And where in '/theme/simple/lib.php' there is:

          $replacement = $OUTPUT->pix_url('images/logo','theme');
          

          is defined, that is the default logo in the images folder when no file is present.

          The code:

          $logo = $theme->setting_file_url('logo', 'logo');
          

          parameters (

          public function setting_file_url($setting, $filearea)

          ) are derived from the '/theme/simple/settings.php' file code (to add if not already):

              // Logo file setting
              $name = 'theme_simple/logo';
              $title = get_string('logo','theme_afterburner');
              $description = get_string('logodesc', 'theme_afterburner');
              $setting = new admin_setting_configstoredfile($name, $title, $description, 'logo');
              $setting->set_updatedcallback('theme_reset_all_caches');
              $settings->add($setting);
          

          Where the '$setting' is the second part after the forward slash of 'theme_simple/logo' - i.e. 'logo' and '$filearea' is taken from the last parameter on the 'admin_setting_configstoredfile' constructor, i.e. 'logo' in '$setting = new admin_setting_configstoredfile($name, $title, $description, 'logo');'.

          I hope this all makes sense.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , As Jean-Michel says, you need the code in '/theme/simple/lib.php': function theme_simple_pluginfile($course, $cm, $context, $filearea, $args, $forcedownload, array $options = array()) { if ($context->contextlevel == CONTEXT_SYSTEM and $filearea === 'logo') { $theme = theme_config::load('simple'); return $theme->setting_file_serve('logo', $args, $forcedownload, $options); } else { send_file_not_found(); } } But that is only the code that runs on the server when the file is requested by the URL called by the css in the browser. The actual setting of the URL is in the css via the pre-processing mechanism, so in '/theme/simple/lib.php' you need: function simple_process_css($css, $theme) { // Set the background image for the logo $logo = $theme->setting_file_url('logo', 'logo'); $css = simple_set_logo($css, $logo); .... function simple_set_logo($css, $logo) { global $OUTPUT; $tag = '[[setting:logo]]'; $replacement = $logo; if (is_null($replacement)) { $replacement = $OUTPUT->pix_url('images/logo','theme'); } $css = str_replace($tag, $replacement, $css); return $css; } where in the css you would have (as an example taken from 'afterburner'): a.logo { background: url([[setting:logo]]) no-repeat 0 0; width: 320px; height: 75px; display: block; margin: 15px 10px 10px; float : left; } using the standard css url getting for the image. And assuming that in '/theme/simple/config.php' you have: $THEME->csspostprocess = 'simple_process_css'; And where in '/theme/simple/lib.php' there is: $replacement = $OUTPUT->pix_url('images/logo','theme'); is defined, that is the default logo in the images folder when no file is present. The code: $logo = $theme->setting_file_url('logo', 'logo'); parameters ( public function setting_file_url($setting, $filearea) ) are derived from the '/theme/simple/settings.php' file code (to add if not already): // Logo file setting $name = 'theme_simple/logo'; $title = get_string('logo','theme_afterburner'); $description = get_string('logodesc', 'theme_afterburner'); $setting = new admin_setting_configstoredfile($name, $title, $description, 'logo'); $setting->set_updatedcallback('theme_reset_all_caches'); $settings->add($setting); Where the '$setting' is the second part after the forward slash of 'theme_simple/logo' - i.e. 'logo' and '$filearea' is taken from the last parameter on the 'admin_setting_configstoredfile' constructor, i.e. 'logo' in '$setting = new admin_setting_configstoredfile($name, $title, $description, 'logo');'. I hope this all makes sense. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          Dosey me missed off the plugins bit!

          It works great in the new bootstrap theme.

          Just about to upload it...if it's not too late!

          Thanks and double thanks to you both!

          Show
          Mary Evans added a comment - Dosey me missed off the plugins bit! It works great in the new bootstrap theme. Just about to upload it...if it's not too late! Thanks and double thanks to you both!
          Hide
          Damyon Wiese added a comment -

          Thanks everyone from working on this.

          I agree with Martins concern about changing the afterburner theme as part of this patch (and all themes inherited from afterburner). So - I have integrated this but have excluded the commit which updates the afterburner theme. So this patch on its own implements no ui changes - but makes it possible to integrate MDL-39021.

          Please open a new issue for updating afterburner and other themes if required.

          Show
          Damyon Wiese added a comment - Thanks everyone from working on this. I agree with Martins concern about changing the afterburner theme as part of this patch (and all themes inherited from afterburner). So - I have integrated this but have excluded the commit which updates the afterburner theme. So this patch on its own implements no ui changes - but makes it possible to integrate MDL-39021 . Please open a new issue for updating afterburner and other themes if required.
          Hide
          Mary Evans added a comment -

          @Damyon, if Simple theme is integrated and accessible for the tester, that theme could be tested instead of Afterburner thus not needing to cherry-pick the Afterburner related commit?

          Just a thought, of course this all depends on how integration works.

          Show
          Mary Evans added a comment - @Damyon, if Simple theme is integrated and accessible for the tester, that theme could be tested instead of Afterburner thus not needing to cherry-pick the Afterburner related commit? Just a thought, of course this all depends on how integration works.
          Hide
          Petr Škoda added a comment -

          Thanks, I have created MDL-39129 for the afterburner theme - ready for integration...

          Show
          Petr Škoda added a comment - Thanks, I have created MDL-39129 for the afterburner theme - ready for integration...
          Hide
          Mary Evans added a comment -

          Excellent!

          Show
          Mary Evans added a comment - Excellent!
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr, and everyone for working on this.

          Works as expected. I can upload a logo, delete it and changes were reflected in simple and after burner theme.

          Show
          Rajesh Taneja added a comment - Thanks Petr, and everyone for working on this. Works as expected. I can upload a logo, delete it and changes were reflected in simple and after burner theme.
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is documented in http://docs.moodle.org/25/en/Standard_themes

          Show
          Mary Cooch added a comment - Removing docs_required label as this is documented in http://docs.moodle.org/25/en/Standard_themes
          Hide
          Sergio Perez added a comment -

          Hi, i need help, how to add this feature to moodle version 2.2?

          Show
          Sergio Perez added a comment - Hi, i need help, how to add this feature to moodle version 2.2?

            People

            • Votes:
              41 Vote for this issue
              Watchers:
              32 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: