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

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

          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