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

Fixing Theme Config Variables

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.7
    • Fix Version/s: 1.7
    • Component/s: Libraries
    • Labels:
      None
    • Database:
      Any
    • Affected Branches:
      MOODLE_17_STABLE
    • Fixed Branches:
      MOODLE_17_STABLE

      Description

      Throughout the moodle core there is use of $CFG->themedir and $CFG->themewww, but these variables are not used in some places, thus making them useful.

      I've produced a patch which fixes the few places where these variables aren't used (and copious bits in the standard themes).

      Mostly its just replacing $CFG->dirroot . 'theme/' with the themedir variable.

      In order to keep previous behaviour I added $CFG->httpsthemewww to keep places happy which used httpswwwroot.

      Also in get_list_of_plugins I added a case statement which allows the themedir to be taken notice of.

      These changes mean that a completely different theme directory can be used and is taken notice of - the theme selector works too.

      Obviously theme authors would need to use these variables - and i've patched the stock themes, but this does mean that the variables are useable.

      We wish to use this to allow the core and themes to be kept seperate on a shared hosting solution.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              poltawski Dan Poltawski added a comment -

              forgot to attach the patch

              Show
              poltawski Dan Poltawski added a comment - forgot to attach the patch
              Hide
              poltawski Dan Poltawski added a comment -

              The patch is rather big - this is due to the many themes i made the small variable changes to - please don't be put off by this! I can create a 'core patch' and then one for each theme if this is helpful

              Show
              poltawski Dan Poltawski added a comment - The patch is rather big - this is due to the many themes i made the small variable changes to - please don't be put off by this! I can create a 'core patch' and then one for each theme if this is helpful
              Hide
              dougiamas Martin Dougiamas added a comment -

              Thanks, Dan! Makes sense to me!

              Petr can you review and check in?

              Show
              dougiamas Martin Dougiamas added a comment - Thanks, Dan! Makes sense to me! Petr can you review and check in?
              Hide
              skodak Petr Skoda added a comment -

              Sounds reasonable to me too, going to review/commit it tomorrow

              Show
              skodak Petr Skoda added a comment - Sounds reasonable to me too, going to review/commit it tomorrow
              Hide
              skodak Petr Skoda added a comment -

              I am stuck in the blog code, sorry

              Show
              skodak Petr Skoda added a comment - I am stuck in the blog code, sorry
              Hide
              skodak Petr Skoda added a comment -

              I do not like the trailing slash in themedir and wwwdir, because all other config.php paths forbid it. The main problem is that if we do not fix it now, we will not be able to do it later because it would break BC in 3rd party themes.

              I have prepared a new patch that fixes following:

              • trailing slash removed + fixed current use of themedir and themewww - fortunately no theme uses it now
              • fixed initilisation of httpswwwroot in setup.php
              • removed obsoleted detect_munged_arguments() in setup.php

              If you can, please test it - I will ask MD tomorrow to confirm the removal of the trailing slashes

              In any case we need to update theme docs in the docs wiki. Any volunteers?

              Show
              skodak Petr Skoda added a comment - I do not like the trailing slash in themedir and wwwdir, because all other config.php paths forbid it. The main problem is that if we do not fix it now, we will not be able to do it later because it would break BC in 3rd party themes. I have prepared a new patch that fixes following: trailing slash removed + fixed current use of themedir and themewww - fortunately no theme uses it now fixed initilisation of httpswwwroot in setup.php removed obsoleted detect_munged_arguments() in setup.php If you can, please test it - I will ask MD tomorrow to confirm the removal of the trailing slashes In any case we need to update theme docs in the docs wiki. Any volunteers?
              Hide
              poltawski Dan Poltawski added a comment -

              Going to give a bash in sec - although i've just noticed an oversight in your patch on line 127,136, you've not put the additional slash in:

              $CFG->httpsthemewww . current_theme()

              Show
              poltawski Dan Poltawski added a comment - Going to give a bash in sec - although i've just noticed an oversight in your patch on line 127,136, you've not put the additional slash in: $CFG->httpsthemewww . current_theme()
              Hide
              skodak Petr Skoda added a comment -

              thanks! I will review it once more tomorrow

              Show
              skodak Petr Skoda added a comment - thanks! I will review it once more tomorrow
              Hide
              poltawski Dan Poltawski added a comment -

              I've had a good play with it and it seems to look good as far as I can see.

              I'll have a go at documenting it on the wiki tommorow if I get a chance.

              Show
              poltawski Dan Poltawski added a comment - I've had a good play with it and it seems to look good as far as I can see. I'll have a go at documenting it on the wiki tommorow if I get a chance.
              Show
              poltawski Dan Poltawski added a comment - Tried to start documentation: http://docs.moodle.org/en/Development:Theme_directory_guide http://docs.moodle.org/en/Theme_directory http://docs.moodle.org/en/1.7_theme_upgrade
              Hide
              skodak Petr Skoda added a comment -

              Thanks you Dan!
              I have added Urs to the watch list. I was unable to contact Martin Dougiamas today, I will ask for the confirmation of the trailing slash removal tomorrow and commit ASAP if ok

              Show
              skodak Petr Skoda added a comment - Thanks you Dan! I have added Urs to the watch list. I was unable to contact Martin Dougiamas today, I will ask for the confirmation of the trailing slash removal tomorrow and commit ASAP if ok
              Hide
              urshunkler Urs Hunkler added a comment -

              Dan, I skimmed the docu and the oportunity to have a variable theme dir sounds great.

              Will be intersting to give some real time examples where this feature has it's advantages over the existing theme directory. The articles in Moodle docs are a good place.

              Petr, would it be save to place the theme dir into the moodledata area to give teachers the chance to upload their themes. - Writing this I suppose it's no good idea because there will be too many security issues.

              Do we also get the chance to handle themes simmilar to languages? To get and install them when you need them?

              Show
              urshunkler Urs Hunkler added a comment - Dan, I skimmed the docu and the oportunity to have a variable theme dir sounds great. Will be intersting to give some real time examples where this feature has it's advantages over the existing theme directory. The articles in Moodle docs are a good place. Petr, would it be save to place the theme dir into the moodledata area to give teachers the chance to upload their themes. - Writing this I suppose it's no good idea because there will be too many security issues. Do we also get the chance to handle themes simmilar to languages? To get and install them when you need them?
              Hide
              skodak Petr Skoda added a comment -

              The problem with themes in moodledata is that files can not be served directly by the apache and style.php needs to know where is the main config.php; it will be hard to solve and I guess it would break backwards compatibility

              I guess we would have to use some new themefile.php to allow fetching of theme files from arbitrary location. We can not work around the security problems with execution of php scripts, the upload of new themes should be definitely reserved for administrators only and it should be disabled by default. The download&installation from web should not be a big problem, it would be as secure as current language import.

              Show
              skodak Petr Skoda added a comment - The problem with themes in moodledata is that files can not be served directly by the apache and style.php needs to know where is the main config.php; it will be hard to solve and I guess it would break backwards compatibility I guess we would have to use some new themefile.php to allow fetching of theme files from arbitrary location. We can not work around the security problems with execution of php scripts, the upload of new themes should be definitely reserved for administrators only and it should be disabled by default. The download&installation from web should not be a big problem, it would be as secure as current language import.
              Hide
              dougiamas Martin Dougiamas added a comment -

              Yep +10 to fix the trailing slash!

              Show
              dougiamas Martin Dougiamas added a comment - Yep +10 to fix the trailing slash!
              Hide
              skodak Petr Skoda added a comment -

              Committed into cvs, I am going to close it later and file a new feature request for the enhancement discussed above...

              Show
              skodak Petr Skoda added a comment - Committed into cvs, I am going to close it later and file a new feature request for the enhancement discussed above...
              Hide
              dougiamas Martin Dougiamas added a comment -

              I guess this is resolved, please re-open if not.

              Show
              dougiamas Martin Dougiamas added a comment - I guess this is resolved, please re-open if not.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    7/Nov/06