Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      28064

      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.

      1. moodle-1.7-themedir.patch
        20 kB
        Dan Poltawski
      2. themedir4.patch
        27 kB
        Petr Škoda

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          forgot to attach the patch

          Show
          Dan Poltawski added a comment - forgot to attach the patch
          Hide
          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
          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
          Martin Dougiamas added a comment -

          Thanks, Dan! Makes sense to me!

          Petr can you review and check in?

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

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

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

          I am stuck in the blog code, sorry

          Show
          Petr Škoda added a comment - I am stuck in the blog code, sorry
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda added a comment -

          thanks! I will review it once more tomorrow

          Show
          Petr Škoda added a comment - thanks! I will review it once more tomorrow
          Hide
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Martin Dougiamas added a comment -

          Yep +10 to fix the trailing slash!

          Show
          Martin Dougiamas added a comment - Yep +10 to fix the trailing slash!
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Martin Dougiamas added a comment -

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

          Show
          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: