Themes

Thank you Mary, it does make sense.  But at the same time seems to break the general rules of assignment operators on global variables!  Or indeed any assignment operator when applied to the attribute of an object.

Thanks all,

It seems that $THEME is not the same thing all the same time.  Ok, with the following at the bottom of the Clean, Bootstrapbase and Base theme config.php files:

error_log(print_r(debug_backtrace(), true));

It produces the attached log when using the URL to set the theme and loading the front page.  It appears that:

setup.php checks the theme is valid in the URL by loading it to the point of creating a theme_config object by using  'theme_config::load($themename)' in 'outputlib.php'.  This is then placed in $SESSION->theme.  But then is not used by pagelib.php 'initialise_theme_and_output()' when it repeats the entire process with the call to 'theme_config::load($themename)' in 'outputlib.php'.  setup.php is called from config.php which is called from index.php.  This appears to be inefficient.

'theme_config::load($themename)' in 'outputlib.php' has in it two lines:

if ($config = theme_config::find_theme_config($themename, $settings)) {
return new theme_config($config);

The first one just calls 'find_theme_config' but that method also loads the parents to check they are ok, but then ditches the parent theme data loaded:

        // verify the theme configuration is OK
        if (!is_array($THEME->parents)) {
            // parents option is mandatory now
            return null;
        } else {
            // We use $parentscheck to only check the direct parents (avoid infinite loop).
            if ($parentscheck) {
                // Find all parent theme configs.
                foreach ($THEME->parents as $parent) {
                    $parentconfig = theme_config::find_theme_config($parent, $settings, false);
                    if (empty($parentconfig)) {
                        return null;
                    }
                }
            }
        }

 As $parentconfig is lost.  The second line 'return new theme_config($config);' calls '__construct($config)' in 'outputlib.php' which calls 'find_theme_config' in 'outputlib.php' again for the parent themes and performs a second valid parent check:

 
        // verify all parents and load configs and renderers
        foreach ($this->parents as $parent) {
            if ($parent == 'base') {
                $parent_config = $baseconfig;
            } else if (!$parent_config = theme_config::find_theme_config($parent, $this->settings)) {
                // this is not good - better exclude faulty parents
                continue;
            }

 But this time decides to keep $parent_config if it is valid.  Now the parent in a parent check would be fine in 'find_theme_config' if we had grandparent - parent - child themes, but the code in '__construct($config)' appears to only go as far as processing the parents of the theme selected and no further.  So therefore the parent in a parent check in the line:

} else if (!$parent_config = theme_config::find_theme_config($parent, $this->settings))

is faulty because the third parameter is not 'false' which tells the method not to check parents.

The upshot of all of this is that I believe there is credible evidence for performance improvements that reduce file loads and memory allocate / deallocate of data which has been carefully processed and then lost.  Also I believe it demonstrates that we cannot have grandparent - parent - child themes or at least if that is the intent, then the data stored in 'config.php' of the theme is not processed correctly.  It also demonstrates that the Clean theme which uses Bootstrapbase as a parent also relies on the contents of the Base theme's config.php:

        if ($this->name != 'base') {
            $baseconfig = theme_config::find_theme_config('base', $this->settings);
        } else {
            $baseconfig = $config;
        }
 

....

        // cascade all layouts properly
        foreach ($baseconfig->layouts as $layout=>$value) {
            if (!isset($this->layouts[$layout])) {
                foreach ($this->parent_configs as $parent_config) {
                    if (isset($parent_config->layouts[$layout])) {
                        $this->layouts[$layout] = $parent_config->layouts[$layout];
                        continue 2;
                    }
                }
                $this->layouts[$layout] = $value;
            }
        }

 When perhaps it should only rely on the '$THEME->layouts' of Bootstrapbase being the primary parent theme.  Therefore conceptually, Base's config.php 'layouts' MUST be accurate for the Bootstrapbase -> Clean pair to work properly.

Ok, this is all complicated, but I think I have it described fairly accurately.  I welcome comments, testing suggestions and indeed possible solutions that would demonstrate a performance gain by optimising the code in a tracker improvement.  I know it may only shave a fraction of a second off the page load time, but what if you were to multiply that by the number of Moodle page loads across all the installations on the planet?  Could we actually save a bit of carbon footprint?

Cheers,

Gareth

P.S. The stack trace line numbers in the log file refer to files in Moodle version 2013111801.01 - release 2.6.1+ (Build: 20140117) if you wish to cross correlate them.