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

Remove hardcoded dataroot/temp and cache directory usage

    Details

    • Testing Instructions:
      Hide

      this is going to be tricky:
      1/ look at the patch and try all places that were affected
      2/ set custom dirs and repeat 1/ + verify the temp and cache is created in the custom dirs

      Show
      this is going to be tricky: 1/ look at the patch and try all places that were affected 2/ set custom dirs and repeat 1/ + verify the temp and cache is created in the custom dirs
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w36_MDL-28701_m22_tempcachedir

      Description

      Usage of $CFG->dataroot . "/temp" in various forms is strewn throughout Moodle.

      I propose creating a single $CFG->tempdir (which defaults to "$CFG->dataroot/temp" to maintain backwards compatibility) to allow a single point to change the location of the temp files if needed.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            The same goes for dataroot/cache I suppose.

            Show
            skodak Petr Skoda added a comment - The same goes for dataroot/cache I suppose.
            Hide
            tomlanyon Tom Lanyon added a comment -

            Added pull branches for dataroot/temp.

            Show
            tomlanyon Tom Lanyon added a comment - Added pull branches for dataroot/temp.
            Hide
            tomlanyon Tom Lanyon added a comment -

            Pull branches updated to fix a problem with setting the default value for $CFG->tempdir in lib/setup.php.

            Also I've added a $CFG->cachedir whilst I was at it, as suggested by Petr.

            Suggest this is ready for review / integration.

            Thanks,
            Tom

            Show
            tomlanyon Tom Lanyon added a comment - Pull branches updated to fix a problem with setting the default value for $CFG->tempdir in lib/setup.php. Also I've added a $CFG->cachedir whilst I was at it, as suggested by Petr. Suggest this is ready for review / integration. Thanks, Tom
            Hide
            skodak Petr Skoda added a comment -

            +1 for integration into master, thanks!

            Show
            skodak Petr Skoda added a comment - +1 for integration into master, thanks!
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this and providing a solution.

            Petr, could you take this through the peer review workflow, please?

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this and providing a solution. Petr, could you take this through the peer review workflow, please?
            Hide
            skodak Petr Skoda added a comment -

            There is a problem with make_upload_directory() because it does not know anything about the new cache or temp dirs.

            Possible solution is to create new functions make_temp_directory() and make_cache_directory() which return the temp/cache path. We could also hack the make_upload_directory() to look for "temp/" and "cache/" for more backwards compatibility...

            Any other ideas?

            I think this would be a too big change for stable branches, but my +10 for master once we resolve the creation of dirs.

            Show
            skodak Petr Skoda added a comment - There is a problem with make_upload_directory() because it does not know anything about the new cache or temp dirs. Possible solution is to create new functions make_temp_directory() and make_cache_directory() which return the temp/cache path. We could also hack the make_upload_directory() to look for "temp/" and "cache/" for more backwards compatibility... Any other ideas? I think this would be a too big change for stable branches, but my +10 for master once we resolve the creation of dirs.
            Hide
            skodak Petr Skoda added a comment -

            Are you going to work on this?

            Show
            skodak Petr Skoda added a comment - Are you going to work on this?
            Hide
            tomlanyon Tom Lanyon added a comment -

            How about we introduce new functions in MASTER; for 20_STABLE and 21_STABLE we could add the new functions and also have the existing make_upload_directory() match "temp" and "cache" for backwards compatibility?

            Show
            tomlanyon Tom Lanyon added a comment - How about we introduce new functions in MASTER; for 20_STABLE and 21_STABLE we could add the new functions and also have the existing make_upload_directory() match "temp" and "cache" for backwards compatibility?
            Hide
            skodak Petr Skoda added a comment -

            How important is it for you to have this in stable branches? In 2.1 we would have to create both dirs - new and old in make_upload_directory(). I personally think we should concentrate on 2.2dev only and get fixed as much as possible instead of tweaking and testing stable which is supposed to not have new features after all.

            Show
            skodak Petr Skoda added a comment - How important is it for you to have this in stable branches? In 2.1 we would have to create both dirs - new and old in make_upload_directory(). I personally think we should concentrate on 2.2dev only and get fixed as much as possible instead of tweaking and testing stable which is supposed to not have new features after all.
            Hide
            tomlanyon Tom Lanyon added a comment - - edited

            I'm not sure about getting it back into stable. I'd really like to see it get into 2.1 but I'll discuss this internally. In the mean time, I've removed the pull URLs for 20_STABLE and 21_STABLE.

            I've updated the master pull URL with new make_temp_directory() and make_cache_directory() functions: https://github.com/tomlanyon/moodle/compare/master...mdl28701-MASTER

            Probably the most interesting commit for you to review is https://github.com/tomlanyon/moodle/commit/7220fbd304dc765bf41d65fdd029ea549bf43ff4

            I've broken the old make_upload_directory down into two new functions: make_writable_directory and protect_directory and added three wrapper make_

            {upload,temp,cache}

            _directory functions around these.

            Show
            tomlanyon Tom Lanyon added a comment - - edited I'm not sure about getting it back into stable. I'd really like to see it get into 2.1 but I'll discuss this internally. In the mean time, I've removed the pull URLs for 20_STABLE and 21_STABLE. I've updated the master pull URL with new make_temp_directory() and make_cache_directory() functions: https://github.com/tomlanyon/moodle/compare/master...mdl28701-MASTER Probably the most interesting commit for you to review is https://github.com/tomlanyon/moodle/commit/7220fbd304dc765bf41d65fdd029ea549bf43ff4 I've broken the old make_upload_directory down into two new functions: make_writable_directory and protect_directory and added three wrapper make_ {upload,temp,cache} _directory functions around these.
            Hide
            skodak Petr Skoda added a comment -

            I have rebased the code and polished it a bit, thanks a lot for the report and patch.

            Show
            skodak Petr Skoda added a comment - I have rebased the code and polished it a bit, thanks a lot for the report and patch.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            This has been integrated now. Thank you both for the work!

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, This has been integrated now. Thank you both for the work! Cheers Sam
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Nice but... why? Uhm.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Nice but... why? Uhm.
            Hide
            skodak Petr Skoda added a comment - - edited

            The problem is that you may have "slow" disk and "fast" disk attached to your server, such as in the cloud or when setting up clusters or when using NFS for dataroot - you do not need to back up the temp and cache, you can even use ram disk for that.

            In the future each server in the cluster could use own temp dir and with a bit of hacking also the cache dir, our code base is not designed for this yet, but this is the first step.

            Show
            skodak Petr Skoda added a comment - - edited The problem is that you may have "slow" disk and "fast" disk attached to your server, such as in the cloud or when setting up clusters or when using NFS for dataroot - you do not need to back up the temp and cache, you can even use ram disk for that. In the future each server in the cluster could use own temp dir and with a bit of hacking also the cache dir, our code base is not designed for this yet, but this is the first step.
            Hide
            skodak Petr Skoda added a comment -

            I was planning this for a long time already, I thought it would be a bit more difficult and more BC problems, but the patch seems relatively simple now.

            Show
            skodak Petr Skoda added a comment - I was planning this for a long time already, I thought it would be a bit more difficult and more BC problems, but the patch seems relatively simple now.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Works Great.
            Thanks for fixing this Tom + Petr

            Show
            rajeshtaneja Rajesh Taneja added a comment - Works Great. Thanks for fixing this Tom + Petr
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Sorry Guys,

            I just found one issue, while testing another bug...

            PHP Notice:  Undefined property: stdClass::$cachedir in /usr/local/www/moodle/lib/moodlelib.php on line 5688, referer: http://rajesh.moodle.local/moodle/install.php
            PHP Stack trace:, referer: http://rajesh.moodle.local/moodle/install.php
            PHP   1. {main}() /usr/local/www/moodle/install.php:0, referer: http://rajesh.moodle.local/moodle/install.php
            PHP   2. get_string() /usr/local/www/moodle/install.php:456, referer: http://rajesh.moodle.local/moodle/install.php
            PHP   3. get_string_manager() /usr/local/www/moodle/lib/moodlelib.php:6673, referer: http://rajesh.moodle.local/moodle/install.php
            [error] [client 127.0.1.1] PHP Notice:  Undefined property: stdClass::$cachedir in /usr/local/www/moodle/lib/moodlelib.php on line 5700, referer: http://rajesh.moodle.local/moodle/install.php
            [error] [client 127.0.1.1] PHP Stack trace:, referer: http://rajesh.moodle.local/moodle/install.php
            [error] [client 127.0.1.1] PHP   1. {main}() /usr/local/www/moodle/install.php:0, referer: http://rajesh.moodle.local/moodle/install.php
            [error] [client 127.0.1.1] PHP   2. get_string() /usr/local/www/moodle/install.php:456, referer: http://rajesh.moodle.local/moodle/install.php
            [error] [client 127.0.1.1] PHP   3. get_string_manager() /usr/local/www/moodle/lib/moodlelib.php:6673, referer: http://rajesh.moodle.local/moodle/install.php

            This happened when I tried to install moodle, at 3rd screen and onwards (Choose database driver)
            Can you please reopen this or let me know and I will open another bug.

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Sorry Guys, I just found one issue, while testing another bug... PHP Notice: Undefined property: stdClass::$cachedir in /usr/local/www/moodle/lib/moodlelib.php on line 5688, referer: http://rajesh.moodle.local/moodle/install.php PHP Stack trace:, referer: http://rajesh.moodle.local/moodle/install.php PHP 1. {main}() /usr/local/www/moodle/install.php:0, referer: http://rajesh.moodle.local/moodle/install.php PHP 2. get_string() /usr/local/www/moodle/install.php:456, referer: http://rajesh.moodle.local/moodle/install.php PHP 3. get_string_manager() /usr/local/www/moodle/lib/moodlelib.php:6673, referer: http://rajesh.moodle.local/moodle/install.php [error] [client 127.0.1.1] PHP Notice: Undefined property: stdClass::$cachedir in /usr/local/www/moodle/lib/moodlelib.php on line 5700, referer: http://rajesh.moodle.local/moodle/install.php [error] [client 127.0.1.1] PHP Stack trace:, referer: http://rajesh.moodle.local/moodle/install.php [error] [client 127.0.1.1] PHP 1. {main}() /usr/local/www/moodle/install.php:0, referer: http://rajesh.moodle.local/moodle/install.php [error] [client 127.0.1.1] PHP 2. get_string() /usr/local/www/moodle/install.php:456, referer: http://rajesh.moodle.local/moodle/install.php [error] [client 127.0.1.1] PHP 3. get_string_manager() /usr/local/www/moodle/lib/moodlelib.php:6673, referer: http://rajesh.moodle.local/moodle/install.php This happened when I tried to install moodle, at 3rd screen and onwards (Choose database driver) Can you please reopen this or let me know and I will open another bug.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            yeah, the point is that install.php uses lang-string services that requires cachedir to be available, but does not include setup.php, so it has not been set.

            So both $CFG->tempdir and $CFG->cachedir should be defined @ install.php.

            Problem is they will default to dataroot/temp and dataroot/cache so will create some rubbish there only for installation (assuming the user changes that to other dirs later in config.php). No big problem, anyway.

            +1 for "caused a regression" linked new issue, for Petr. And keep this passed. (it's master problem only and not blocker one).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - yeah, the point is that install.php uses lang-string services that requires cachedir to be available, but does not include setup.php, so it has not been set. So both $CFG->tempdir and $CFG->cachedir should be defined @ install.php. Problem is they will default to dataroot/temp and dataroot/cache so will create some rubbish there only for installation (assuming the user changes that to other dirs later in config.php). No big problem, anyway. +1 for "caused a regression" linked new issue, for Petr. And keep this passed. (it's master problem only and not blocker one). Ciao
            Hide
            skodak Petr Skoda added a comment -

            working on a fix now

            Show
            skodak Petr Skoda added a comment - working on a fix now
            Hide
            skodak Petr Skoda added a comment - - edited

            patch created, thanks a lot!

            Edited: MDL-29351

            Show
            skodak Petr Skoda added a comment - - edited patch created, thanks a lot! Edited: MDL-29351
            Hide
            tomlanyon Tom Lanyon added a comment -

            Thanks Petr. Is it worthwhile also removing the $

            {dataroot}

            /temp that's created by the installer after it's finished?

            Show
            tomlanyon Tom Lanyon added a comment - Thanks Petr. Is it worthwhile also removing the $ {dataroot} /temp that's created by the installer after it's finished?
            Hide
            skodak Petr Skoda added a comment -

            I think we can keep the temp there, in most cases you would not change the dirs when using web installer. In advanced setups you would create your own custom config.php manually, right?

            Show
            skodak Petr Skoda added a comment - I think we can keep the temp there, in most cases you would not change the dirs when using web installer. In advanced setups you would create your own custom config.php manually, right?
            Hide
            tomlanyon Tom Lanyon added a comment -

            Sure, but often not until after the install process, then you'd end up with a 'temp' directory under dataroot no longer used and it'd be nice if that was cleaned up.

            I was also just thinking of future changes to the web and/or cli installers which could allow you to specify cachedir and tempdir during install, so everything after the installer would the custom dirs but the installer would still temporarily need dataroot/temp.

            I can't think of any downside to cleaning up after install.php runs, the install temp dir is not needed after the install is complete and it'd be recreated on first request anyway, so figured it'd be worthwhile cleaning up anyway.

            Show
            tomlanyon Tom Lanyon added a comment - Sure, but often not until after the install process, then you'd end up with a 'temp' directory under dataroot no longer used and it'd be nice if that was cleaned up. I was also just thinking of future changes to the web and/or cli installers which could allow you to specify cachedir and tempdir during install, so everything after the installer would the custom dirs but the installer would still temporarily need dataroot/temp. I can't think of any downside to cleaning up after install.php runs, the install temp dir is not needed after the install is complete and it'd be recreated on first request anyway, so figured it'd be worthwhile cleaning up anyway.
            Hide
            skodak Petr Skoda added a comment -

            I do not understand your arguments - if somebody decides to change the temp dir they are responsible for cleaning up the dataroot or keeping it as-is. I do not think people should stop the web installer in the middle and hack config.php right before the db installation starts.

            The only recommended ways are imho:
            1/ use the cli/web installer and modify everything (including temp/cache dirs) after the installation completes
            2/ create custom config.php before starting install

            Show
            skodak Petr Skoda added a comment - I do not understand your arguments - if somebody decides to change the temp dir they are responsible for cleaning up the dataroot or keeping it as-is. I do not think people should stop the web installer in the middle and hack config.php right before the db installation starts. The only recommended ways are imho: 1/ use the cli/web installer and modify everything (including temp/cache dirs) after the installation completes 2/ create custom config.php before starting install
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            YTC !

            (aka, yay, thanks and ciao ) Closing.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.
            Hide
            tlock Tim Lock added a comment -

            Any chance this could be backported to 2.1.4?

            Show
            tlock Tim Lock added a comment - Any chance this could be backported to 2.1.4?
            Hide
            skodak Petr Skoda added a comment -

            Hello, I am sorry, new features are not usually backported to older stable branches, please consider upgrading to 2.2. Petr

            Show
            skodak Petr Skoda added a comment - Hello, I am sorry, new features are not usually backported to older stable branches, please consider upgrading to 2.2. Petr
            Hide
            tlock Tim Lock added a comment -

            Thanks anyways Petr.

            We support lots clients on 2.0 & 2.1, so upgrading within the stable releases cycle is what we normally do.

            Almost all the patches cherry-pick nicely on 2.1.3 so wouldn't be alot of effect.

            Regards,
            Tim

            Show
            tlock Tim Lock added a comment - Thanks anyways Petr. We support lots clients on 2.0 & 2.1, so upgrading within the stable releases cycle is what we normally do. Almost all the patches cherry-pick nicely on 2.1.3 so wouldn't be alot of effect. Regards, Tim

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/Dec/11