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
    • Rank:
      18360

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          The same goes for dataroot/cache I suppose.

          Show
          Petr Škoda added a comment - The same goes for dataroot/cache I suppose.
          Hide
          Tom Lanyon added a comment -

          Added pull branches for dataroot/temp.

          Show
          Tom Lanyon added a comment - Added pull branches for dataroot/temp.
          Hide
          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
          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
          Petr Škoda added a comment -

          +1 for integration into master, thanks!

          Show
          Petr Škoda added a comment - +1 for integration into master, thanks!
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

          Are you going to work on this?

          Show
          Petr Škoda added a comment - Are you going to work on this?
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda added a comment -

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

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

          Hi guys,

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

          Cheers
          Sam

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

          Nice but... why? Uhm.

          Show
          Eloy Lafuente (stronk7) added a comment - Nice but... why? Uhm.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Rajesh Taneja added a comment -

          Works Great.
          Thanks for fixing this Tom + Petr

          Show
          Rajesh Taneja added a comment - Works Great. Thanks for fixing this Tom + Petr
          Hide
          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
          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
          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
          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
          Petr Škoda added a comment -

          working on a fix now

          Show
          Petr Škoda added a comment - working on a fix now
          Hide
          Petr Škoda added a comment - - edited

          patch created, thanks a lot!

          Edited: MDL-29351

          Show
          Petr Škoda added a comment - - edited patch created, thanks a lot! Edited: MDL-29351
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

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

          Any chance this could be backported to 2.1.4?

          Show
          Tim Lock added a comment - Any chance this could be backported to 2.1.4?
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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: