Moodle
  1. Moodle
  2. MDL-27280

csspostprocess does not accept arrays

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0
    • Fix Version/s: None
    • Component/s: Themes
    • Labels:
      None
    • Environment:
      2.0.2+
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      16970

      Description

      This issue is related to MDL-25856 - it seems that in that ticket it was decided that a theme creator should call the parent csspostprocess function if they wanted it, rather than Moodle doing it manually. The issue is that csspostprocess doesn't accept an array.

      When creating a child theme that has its own postprocessing based off of a parent that also has postprocessing, only one can be called, not both.

      Here's what happens:
      $THEME->csspostprocess = array('parent_process_css', 'child_process_css');
      will cause Moodle to throw errors about not being able to parse $csspostprocess.

      Expected result:
      Each value in the array would be parsed and the process_css function would be called for each theme and then properly rendered in that order.

      Usage scenario:
      Theme Parent has a single setting that defines the URL for the site logo
      Themes Child1, Child2, and Child3 have a variety of different settings including colors, etc...

      Ideally the admin would be able to change the logo url in the Parent settings page and it would propagate to Child1, Child2, and Child3 automatically.

      As it is, the settings, lib functions, and strings must first be removed from the parent theme, and then replicated in Child1, Child2, and Child3 - then the admin must go to the settings page for Child1, Child2, and Child3 to change the logo url.

      Obviously the more settings that the parent has the more trouble the current model becomes to maintain (e.g. logo url, footer information, font size options, etc...) and the more child themes that are created off of the parent theme cause the amount of extra work to increase exponentially.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          @Daniel

          Have you had any luck trying to find a way of doing this by hacking Moodle code?

          Just curious

          Mary

          Show
          Mary Evans added a comment - @Daniel Have you had any luck trying to find a way of doing this by hacking Moodle code? Just curious Mary
          Hide
          Daniel Wahl added a comment -

          I had looked at it a while back but couldn't figure it out - I can revisit it though.

          Show
          Daniel Wahl added a comment - I had looked at it a while back but couldn't figure it out - I can revisit it though.
          Hide
          Mary Evans added a comment -

          Hi,
          I was just thinking that if it was possible, and a patch could be found, then I could ask Sam to 'Peer Review' it and it might have a chance of getting integrated into Moodle 2.3

          Just thinking outside the box.
          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi, I was just thinking that if it was possible, and a patch could be found, then I could ask Sam to 'Peer Review' it and it might have a chance of getting integrated into Moodle 2.3 Just thinking outside the box. Cheers Mary
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've been having a think about this, there are several different ways in which a theme can deal with settings from the parent.

          1. The theme_config class could call required parents post process functions if as you say they are named in an array.
          2. The current theme's post process function looks for and calls the parents post process method.
          3. The current theme's post process function does ALL of the processing itself, including replicating any required processing the parent would have done that is still required.

          All three are possible, currently options 2 and 3 are possible. Option 3 is going to be easiest for most people as it just required duplicating what the parent theme does (as required) its always going to work and the theme designer is going to understand everything thats going on. It's not however going to be any fun to maintain.
          Option 2 is pretty easy as well and will certainly be easier to maintain, however this is the first point at which the question of ordering is raised, does parent theme processing happen before or after the current theme processing? The good thing about this method is that you can decide the ordering as you have to explicitly call things yourself.
          Another downside to this approach is that things are liable to go wrong as theme settings get more complex.
          In order for this method to work a parent theme's post process method would need to written to not assume it was being called for just the current theme. It couldn't in fact rely on anything in the itself being present as when it is called it may be called from another theme.
          The following code outlines method 2:

          // config.php
          $THEME->csspostprocess = 'my_post_process_function';
          
          // lib.php
          function my_post_process_function($css, $theme) {
              global $CFG;
              // Do my processing first
              $css = my_crazy_css_changes($css);
          
              // Now do the parent settings
              if (function_exists('parent_post_process_function')) {
                  $css = parent_post_process_function($css, $theme);
              }
          }
          

          Now the first option the; idea you are exploring here.
          It is automated and ordering would be achieved through the order of elements in the array.
          It's going to be the easiest, however it is going to suffer from the same problems as option 2.
          If you did want to explore this option the following patch would either do it or at least get you close (untested):

          diff --git a/lib/outputlib.php b/lib/outputlib.php
          index 8feae4c..07efd6d 100644
          --- a/lib/outputlib.php
          +++ b/lib/outputlib.php
          @@ -883,9 +883,16 @@ class theme_config {
                   }
           
                   // now resolve all theme settings or do any other postprocessing
          -        $csspostprocess = $this->csspostprocess;
          -        if (function_exists($csspostprocess)) {
          -            $css = $csspostprocess($css, $this);
          +        if (!empty($this->csspostprocess)) {
          +            $csspostprocess = $this->csspostprocess;
          +            if (!is_array($csspostprocess)) {
          +                $csspostprocess = array($csspostprocess);
          +            }
          +            foreach ($csspostprocess as $function) {
          +                if (function_exists($function)) {
          +                    $css = $function($css, $this);
          +                }
          +            }
                   }
           
                   return $css;
          

          This code simply adds the functionality of processing an array of post process methods, they could be parent methods but they don't have to be (you could have several post process methods of your own). To be truthful I'm not sure this would get into core as it has the potential to cause harm and essentially it is only a matter of convenience.
          Just noting as well if you do decide to go that way it would be master only as it would be a new feature/improvement.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've been having a think about this, there are several different ways in which a theme can deal with settings from the parent. The theme_config class could call required parents post process functions if as you say they are named in an array. The current theme's post process function looks for and calls the parents post process method. The current theme's post process function does ALL of the processing itself, including replicating any required processing the parent would have done that is still required. All three are possible, currently options 2 and 3 are possible. Option 3 is going to be easiest for most people as it just required duplicating what the parent theme does (as required) its always going to work and the theme designer is going to understand everything thats going on. It's not however going to be any fun to maintain. Option 2 is pretty easy as well and will certainly be easier to maintain, however this is the first point at which the question of ordering is raised, does parent theme processing happen before or after the current theme processing? The good thing about this method is that you can decide the ordering as you have to explicitly call things yourself. Another downside to this approach is that things are liable to go wrong as theme settings get more complex. In order for this method to work a parent theme's post process method would need to written to not assume it was being called for just the current theme. It couldn't in fact rely on anything in the itself being present as when it is called it may be called from another theme. The following code outlines method 2: // config.php $THEME->csspostprocess = 'my_post_process_function'; // lib.php function my_post_process_function($css, $theme) { global $CFG; // Do my processing first $css = my_crazy_css_changes($css); // Now do the parent settings if (function_exists('parent_post_process_function')) { $css = parent_post_process_function($css, $theme); } } Now the first option the; idea you are exploring here. It is automated and ordering would be achieved through the order of elements in the array. It's going to be the easiest, however it is going to suffer from the same problems as option 2. If you did want to explore this option the following patch would either do it or at least get you close (untested): diff --git a/lib/outputlib.php b/lib/outputlib.php index 8feae4c..07efd6d 100644 --- a/lib/outputlib.php +++ b/lib/outputlib.php @@ -883,9 +883,16 @@ class theme_config { } // now resolve all theme settings or do any other postprocessing - $csspostprocess = $ this ->csspostprocess; - if (function_exists($csspostprocess)) { - $css = $csspostprocess($css, $ this ); + if (!empty($ this ->csspostprocess)) { + $csspostprocess = $ this ->csspostprocess; + if (!is_array($csspostprocess)) { + $csspostprocess = array($csspostprocess); + } + foreach ($csspostprocess as $function) { + if (function_exists($function)) { + $css = $function($css, $ this ); + } + } } return $css; This code simply adds the functionality of processing an array of post process methods, they could be parent methods but they don't have to be (you could have several post process methods of your own). To be truthful I'm not sure this would get into core as it has the potential to cause harm and essentially it is only a matter of convenience. Just noting as well if you do decide to go that way it would be master only as it would be a new feature/improvement. Cheers Sam
          Hide
          Mary Evans added a comment -

          Hi Sam,

          This is going to be a great help!
          Many thanks.
          Mary

          Show
          Mary Evans added a comment - Hi Sam, This is going to be a great help! Many thanks. Mary
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d.

          TW9vZGxlDQo=

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year. It was reported as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.5 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d. TW9vZGxlDQo=
          Hide
          Daniel Wahl added a comment -

          this issue is still relevant.

          Show
          Daniel Wahl added a comment - this issue is still relevant.

            People

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

              Dates

              • Created:
                Updated: