Moodle
  1. Moodle
  2. MDL-38570

Delete tmp files and directories older than one week in cron

    Details

    • Testing Instructions:
      Hide

      This issue needs to be tested on windows and linux and osx.

      These testing instructions verify that temporary files and empty directories older that are one week old (or older) are deleted.

      1. Under the moodledata/temp directory creating files and directories for testing. You can use the “touch” command to create files and directories with a specific time:

      touch -d "25 hours ago" testdir1
      touch -d "1 month ago" testdir2
      touch -d "3 days ago" testfile1
      touch -d "2 weeks ago" testfile2

      2. Run the moodle cron

      Note: that the cleanup is not triggered during every cron run but only approximately 20% of the time. You may need to run the cron more than once.

      3. Verify that files and directories that were given a time of more than one week are deleted from moodledata/temp

      4. Run phpunit tests on linux, windows and macosx

      Show
      This issue needs to be tested on windows and linux and osx. These testing instructions verify that temporary files and empty directories older that are one week old (or older) are deleted. 1. Under the moodledata/temp directory creating files and directories for testing. You can use the “touch” command to create files and directories with a specific time: touch -d "25 hours ago" testdir1 touch -d "1 month ago" testdir2 touch -d "3 days ago" testfile1 touch -d "2 weeks ago" testfile2 2. Run the moodle cron Note: that the cleanup is not triggered during every cron run but only approximately 20% of the time. You may need to run the cron more than once. 3. Verify that files and directories that were given a time of more than one week are deleted from moodledata/temp 4. Run phpunit tests on linux, windows and macosx
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38570-master
    • Rank:
      48595

      Description

      After 1 year of running Moodle, there are a large number of files stored in the /temp folder of Moodle data directory, which occupied huge disk space Eg. there are files in the subfolders "backup", "csvimport","gradeexport","gradeimport",etc. Are all these files only used temporarily, and safe to be deleted?

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          Yes, dataroot/temp can be deleted when nobody accesses your server.

          Show
          Petr Škoda added a comment - Yes, dataroot/temp can be deleted when nobody accesses your server.
          Hide
          Martín Langhoff added a comment -

          AFAICS, there is no reason for moodle cron not to delete old files, say 1 week old.

          @Petr Škoda?

          Show
          Martín Langhoff added a comment - AFAICS, there is no reason for moodle cron not to delete old files, say 1 week old. @ Petr Škoda ?
          Hide
          Petr Škoda added a comment -

          Right, any temp files older than cca 1 day should be ok to delete - it has to be more than oldest running PHP script (such as stats or upgrade).

          Show
          Petr Škoda added a comment - Right, any temp files older than cca 1 day should be ok to delete - it has to be more than oldest running PHP script (such as stats or upgrade).
          Hide
          Tim Gus added a comment -

          Please consider my fix in the branches provided on my github account.

          Show
          Tim Gus added a comment - Please consider my fix in the branches provided on my github account.
          Hide
          Petr Škoda added a comment -

          Hello, why is there the $tempdir parameter? Why would you change it to anything else?

          Show
          Petr Škoda added a comment - Hello, why is there the $tempdir parameter? Why would you change it to anything else?
          Hide
          Tim Gus added a comment -

          The tempdir param was initially added to support unit testing through vfsStream so that you could pass in something like vfsStream::url('path/path') as an argument instead of a "real" path. The same is true for the timestamp parameter. Unfortunately, adding vfsStream to Moodle through Composer didn't work out too well.

          Show
          Tim Gus added a comment - The tempdir param was initially added to support unit testing through vfsStream so that you could pass in something like vfsStream::url('path/path') as an argument instead of a "real" path. The same is true for the timestamp parameter. Unfortunately, adding vfsStream to Moodle through Composer didn't work out too well.
          Hide
          Petr Škoda added a comment - - edited

          you can unit test this by overriding the $CFG->tempdir before calling this new method, or just use the current temp dir - in moodle tests you can modify global state and dataroot freely

          Show
          Petr Škoda added a comment - - edited you can unit test this by overriding the $CFG->tempdir before calling this new method, or just use the current temp dir - in moodle tests you can modify global state and dataroot freely
          Hide
          Tim Gus added a comment -

          I've updated the code since unit tests aren't added anyways; however, I'm unable to edit this issue with the new diff URL's.

          Show
          Tim Gus added a comment - I've updated the code since unit tests aren't added anyways; however, I'm unable to edit this issue with the new diff URL's.
          Hide
          Petr Škoda added a comment -

          1/ cron_delete_from_temp() - maybe the method name could be improved, if I saw only the name I would expect it does something else
          2/ I am not sure the file_storage class is the best place, because it deals with stored files only, temporary files are not controlled by that class
          3/ we require unit tests for all new features now, this looks like a new feature to me
          4/ ideally the temp files should be removed after use, did anybody verify the places that create temp files and made sure there are appropriate unlink()s? Ideally the leftovers in temdir should be caused by request aborts.
          5/ I am not sure the timeout should be method parameter, what code would change it?

          Show
          Petr Škoda added a comment - 1/ cron_delete_from_temp() - maybe the method name could be improved, if I saw only the name I would expect it does something else 2/ I am not sure the file_storage class is the best place, because it deals with stored files only, temporary files are not controlled by that class 3/ we require unit tests for all new features now, this looks like a new feature to me 4/ ideally the temp files should be removed after use, did anybody verify the places that create temp files and made sure there are appropriate unlink()s? Ideally the leftovers in temdir should be caused by request aborts. 5/ I am not sure the timeout should be method parameter, what code would change it?
          Hide
          Toni Ginard added a comment -

          Just in case it helps, we're using a local cron to daily remove all the content of moodledata/temp/. The code is as follows:

          echo "Deleting files from temp directory...\n";

          $tempdir = $CFG->tempdir;

          define('SECONDS_IN_A_DAY', 86400);
          define('NOW', mktime());

          if (is_dir($tempdir)) {

          // Using PHP object for iteration
          $dir = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($tempdir), RecursiveIteratorIterator::CHILD_FIRST);

          for ($dir->rewind(); $dir->valid(); $dir->next()) {
          if (is_file($dir->getPathname()) && (NOW - filemtime($dir->getPathname()) > SECONDS_IN_A_DAY))

          { unlink($dir->getPathname()); echo 'File ' . $dir->getPathname() . " deleted\n"; }


          // Conditions are tested from left to right and execution stops when any of them is false
          elseif (is_dir($dir->getPathname()) && is_writable($dir->getPathname()) && rmdir($dir->getPathname()))

          { // rmdir only removes empty directories. Upon failure, an E_WARNING is emitted. echo 'Empty directory ' . $dir->getPathname() . " deleted\n"; }

          }
          }

          Show
          Toni Ginard added a comment - Just in case it helps, we're using a local cron to daily remove all the content of moodledata/temp/. The code is as follows: echo "Deleting files from temp directory...\n"; $tempdir = $CFG->tempdir; define('SECONDS_IN_A_DAY', 86400); define('NOW', mktime()); if (is_dir($tempdir)) { // Using PHP object for iteration $dir = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($tempdir), RecursiveIteratorIterator::CHILD_FIRST); for ($dir->rewind(); $dir->valid(); $dir->next()) { if (is_file($dir->getPathname()) && (NOW - filemtime($dir->getPathname()) > SECONDS_IN_A_DAY)) { unlink($dir->getPathname()); echo 'File ' . $dir->getPathname() . " deleted\n"; } // Conditions are tested from left to right and execution stops when any of them is false elseif (is_dir($dir->getPathname()) && is_writable($dir->getPathname()) && rmdir($dir->getPathname())) { // rmdir only removes empty directories. Upon failure, an E_WARNING is emitted. echo 'Empty directory ' . $dir->getPathname() . " deleted\n"; } } }
          Hide
          Petr Škoda added a comment -

          The general rule is that the code heading to integration must not be confusing - I am personally confused by the method location, method name and its parameters, that is not a good sign. In general I do like the idea of cleaning up temporary files that somehow did not get deleted, however our other priority should be to delete the files if possible after they are used - implementing automatic cleanup first complicates the discovered of problematic places a bit.

          Show
          Petr Škoda added a comment - The general rule is that the code heading to integration must not be confusing - I am personally confused by the method location, method name and its parameters, that is not a good sign. In general I do like the idea of cleaning up temporary files that somehow did not get deleted, however our other priority should be to delete the files if possible after they are used - implementing automatic cleanup first complicates the discovered of problematic places a bit.
          Hide
          Tim Gus added a comment - - edited

          @skodak
          "I am not sure the timeout should be method parameter, what code would change it?"

          • I think you mean the "timestamp" parameter. That would be needed for unit tests unless a global CFG is used, or it's set a class property.

          "We require unit tests for all new features now, this looks like a new feature to me"

          • The unit test would have to deal with file timestamps which are volatile. Something like vfsStream would allow us to perform a "touch" on the file to change the file modification time but would you allow integration of vfsStream into Moodle through Composer?

          "ideally the temp files should be removed after use, did anybody verify the places that create temp files and made sure there are appropriate unlink()s? Ideally the leftovers in temdir should be caused by request aborts"

          • I not sure what you mean by that. This issue requests deletion of files under moodledata/temp after a certain period of time.

          "I am not sure the file_storage class is the best place, because it deals with stored files only, temporary files are not controlled by that class"

          • The function make_temp_directory is in lib/setuplib.php but that's probably not the right place for this function either. I'm not sure what would be an ideal location for you.
          Show
          Tim Gus added a comment - - edited @skodak "I am not sure the timeout should be method parameter, what code would change it?" I think you mean the "timestamp" parameter. That would be needed for unit tests unless a global CFG is used, or it's set a class property. "We require unit tests for all new features now, this looks like a new feature to me" The unit test would have to deal with file timestamps which are volatile. Something like vfsStream would allow us to perform a "touch" on the file to change the file modification time but would you allow integration of vfsStream into Moodle through Composer? "ideally the temp files should be removed after use, did anybody verify the places that create temp files and made sure there are appropriate unlink()s? Ideally the leftovers in temdir should be caused by request aborts" I not sure what you mean by that. This issue requests deletion of files under moodledata/temp after a certain period of time. "I am not sure the file_storage class is the best place, because it deals with stored files only, temporary files are not controlled by that class" The function make_temp_directory is in lib/setuplib.php but that's probably not the right place for this function either. I'm not sure what would be an ideal location for you.
          Hide
          Petr Škoda added a comment -

          1/ it is not a timestamp, it is the max lifetime of a temp file - I still do not see a reason to make it configurable, you may shoot your foot easily if you use small value there

          2/ tests are required - I would use touch() to simulate outdated temp files in real temdir and verify results

          3/ file_storage is not good place for this in my opinion, I do not know where to put it

          Show
          Petr Škoda added a comment - 1/ it is not a timestamp, it is the max lifetime of a temp file - I still do not see a reason to make it configurable, you may shoot your foot easily if you use small value there 2/ tests are required - I would use touch() to simulate outdated temp files in real temdir and verify results 3/ file_storage is not good place for this in my opinion, I do not know where to put it
          Hide
          Tim Gus added a comment -

          @skodak
          "I would use touch() to simulate outdated temp files in real temdir and verify results"

          • Would the phpunit data directory be a better idea to store the temp files for testing as defined in config.php? $CFG->phpunit_dataroot = '/home/moodle/phpu_moodledata';
          Show
          Tim Gus added a comment - @skodak "I would use touch() to simulate outdated temp files in real temdir and verify results" Would the phpunit data directory be a better idea to store the temp files for testing as defined in config.php? $CFG->phpunit_dataroot = '/home/moodle/phpu_moodledata';
          Hide
          Petr Škoda added a comment -

          I do not understand, when running phpunit tests in moodle you are free to modify anything in dataroot (or database), it all gets discarded at the end of the test.

          Show
          Petr Škoda added a comment - I do not understand, when running phpunit tests in moodle you are free to modify anything in dataroot (or database), it all gets discarded at the end of the test.
          Hide
          Tim Gus added a comment -

          @skodak
          How about we create a new file lib/filestorage/temp_file.php?
          You can checkout my new master branch (MDL-38570-master_v2) with the unit test in lib/filestorage/tests/temp_file_test.php:
          https://github.com/timgus/moodle/commit/6f12c54717cdbd83763e944bd5c5b46e8eaf40bc

          I've added a new cleanup call for the temp files in lib/cronlib.php. It will run once a day.
          I'm using Toni's example of the RecursiveIteratorIterator that will iterate through a tree structures rather than using get_directory_list which has to put everything in memory first and then iterate.

          If that solution is acceptable I will update the rest of the Moodle branch versions. If it's not then we can always change things.

          Show
          Tim Gus added a comment - @skodak How about we create a new file lib/filestorage/temp_file.php? You can checkout my new master branch ( MDL-38570 -master_v2) with the unit test in lib/filestorage/tests/temp_file_test.php: https://github.com/timgus/moodle/commit/6f12c54717cdbd83763e944bd5c5b46e8eaf40bc I've added a new cleanup call for the temp files in lib/cronlib.php. It will run once a day. I'm using Toni's example of the RecursiveIteratorIterator that will iterate through a tree structures rather than using get_directory_list which has to put everything in memory first and then iterate. If that solution is acceptable I will update the rest of the Moodle branch versions. If it's not then we can always change things.
          Hide
          Petr Škoda added a comment -

          I do not understand where you see any problems, just delete all parameters from cron_delete_from_temp() and write simple unit tests that works on actual dirs that are present in our phpunit test environment.

          Show
          Petr Škoda added a comment - I do not understand where you see any problems, just delete all parameters from cron_delete_from_temp() and write simple unit tests that works on actual dirs that are present in our phpunit test environment.
          Hide
          Tim Gus added a comment -

          I'm now confused too. In your earlier comments you gave the opinion that the class file_storage was not a good place to add the new changes because "temporary files are not controlled by that class" and that you don't know where to put the new changes. That's why my new changes create the file lib/filestorage/temp_file.php.
          Please take a look at my new changes: https://github.com/timgus/moodle/commit/6f12c54717cdbd83763e944bd5c5b46e8eaf40bc

          Show
          Tim Gus added a comment - I'm now confused too. In your earlier comments you gave the opinion that the class file_storage was not a good place to add the new changes because "temporary files are not controlled by that class" and that you don't know where to put the new changes. That's why my new changes create the file lib/filestorage/temp_file.php. Please take a look at my new changes: https://github.com/timgus/moodle/commit/6f12c54717cdbd83763e944bd5c5b46e8eaf40bc
          Hide
          Petr Škoda added a comment -

          yes, temp files have nothing to do with /lib/filestorage/*, you can put it to the lib/cronlib.php directly, if necessary we could move it later - there is a plan to abstract cron tasks to individual classes with new scheduling mechanism

          Show
          Petr Škoda added a comment - yes, temp files have nothing to do with /lib/filestorage/*, you can put it to the lib/cronlib.php directly, if necessary we could move it later - there is a plan to abstract cron tasks to individual classes with new scheduling mechanism
          Hide
          Tim Gus added a comment -

          My new changes for the master branch are found here:
          https://github.com/timgus/moodle/commit/897d3e65b50e95b12ca6e6b4cbe8c09f2ee8759a

          If that's acceptable I will update the rest of the branches and provide links. I do not appear to have edit permissions for this issue to update the branch and diff URLs.

          Note that I've added the file and directory deletion code to the "occasional clean-up tasks" section of the cron.

          Show
          Tim Gus added a comment - My new changes for the master branch are found here: https://github.com/timgus/moodle/commit/897d3e65b50e95b12ca6e6b4cbe8c09f2ee8759a If that's acceptable I will update the rest of the branches and provide links. I do not appear to have edit permissions for this issue to update the branch and diff URLs. Note that I've added the file and directory deletion code to the "occasional clean-up tasks" section of the cron.
          Hide
          Petr Škoda added a comment -

          Thanks! I think this fits current coding style much better, but I just realised there is a potential race condition when deleting the directories - if anything is reusing already created dir it could theoretically fail badly, is dir timestamp updated when you add or delete files in all supported OS and filesystems?

          This brings us back to the beginning - should we really delete dirs at all in cron? our top priority should be imo to delete the temp files and dirs after use instead of doing cron cleanup (it would probably require more ignore_user_aborts() to prevent interruptions by users). Maybe a longer $time such as one week could significantly lower the risk of directory race conditions.

          Show
          Petr Škoda added a comment - Thanks! I think this fits current coding style much better, but I just realised there is a potential race condition when deleting the directories - if anything is reusing already created dir it could theoretically fail badly, is dir timestamp updated when you add or delete files in all supported OS and filesystems? This brings us back to the beginning - should we really delete dirs at all in cron? our top priority should be imo to delete the temp files and dirs after use instead of doing cron cleanup (it would probably require more ignore_user_aborts() to prevent interruptions by users). Maybe a longer $time such as one week could significantly lower the risk of directory race conditions.
          Hide
          Tim Gus added a comment -

          PHP has a function called "flock" to prevent race conditions by locking files. That function however is not well supported by all filesystems apparently but it's used in Moodle (see: cache/stores/file/lib.php). When unlink fails due to a race condition it will issue an E_WARNING that can be suppressed with @. Also using rmdir on a non-empty directory will issue an E_WARNING that can be suppressed with the silence operator.
          I'm not exactly sure what you mean when you say temp files and dirs should be cleared after use. Are you saying that it should be the responsibility of who creates the temp files to also delete them? If so, then we can't enforce or guarantee that everyone will do that, obviously.
          I've updated the code to clean files and directories older than one week, removed scandir and added @ suppression to unlink and rmdir:
          https://github.com/timgus/moodle/compare/MDL-38570-master

          Show
          Tim Gus added a comment - PHP has a function called "flock" to prevent race conditions by locking files. That function however is not well supported by all filesystems apparently but it's used in Moodle (see: cache/stores/file/lib.php). When unlink fails due to a race condition it will issue an E_WARNING that can be suppressed with @. Also using rmdir on a non-empty directory will issue an E_WARNING that can be suppressed with the silence operator. I'm not exactly sure what you mean when you say temp files and dirs should be cleared after use. Are you saying that it should be the responsibility of who creates the temp files to also delete them? If so, then we can't enforce or guarantee that everyone will do that, obviously. I've updated the code to clean files and directories older than one week, removed scandir and added @ suppression to unlink and rmdir: https://github.com/timgus/moodle/compare/MDL-38570-master
          Hide
          Petr Škoda added a comment -

          flock does not solve the race condition if you delete dir after something else checks its existence and creates a new file in it, in anycase flock would cause performance problems and is not recommended at all especially in filedir. And yes, whoever creates temp files is supposed to delete them afterwards.

          Show
          Petr Škoda added a comment - flock does not solve the race condition if you delete dir after something else checks its existence and creates a new file in it, in anycase flock would cause performance problems and is not recommended at all especially in filedir. And yes, whoever creates temp files is supposed to delete them afterwards.
          Hide
          Tim Gus added a comment -

          There are many third-party plugins that we use that will write to tempdir and likely never delete them. That means we would have to either modify the source code of each of those plugins (which could be many plugins in all and that's unrealistic to do) to delete those files afterwards or use an external script, maybe something like a custom cron script that will delete from tempdir and risk running into potential race conditions.

          Do you think there is still a way for a solution within Moodle? Perhaps I can modify my code to only delete files and not directories.

          I appreciate your comments so far.

          Show
          Tim Gus added a comment - There are many third-party plugins that we use that will write to tempdir and likely never delete them. That means we would have to either modify the source code of each of those plugins (which could be many plugins in all and that's unrealistic to do) to delete those files afterwards or use an external script, maybe something like a custom cron script that will delete from tempdir and risk running into potential race conditions. Do you think there is still a way for a solution within Moodle? Perhaps I can modify my code to only delete files and not directories. I appreciate your comments so far.
          Hide
          Martín Langhoff added a comment -

          Petr Škoda: at some point we have to declare very old tmpfiles and tmpdirs as abandoned, no? One month?

          Any plugin instance needs to cope with temp being, well, temporary. I more than agree with not racing with users of temp, but a week or a month are far more reasonable than "never".

          Show
          Martín Langhoff added a comment - Petr Škoda : at some point we have to declare very old tmpfiles and tmpdirs as abandoned, no? One month? Any plugin instance needs to cope with temp being, well, temporary. I more than agree with not racing with users of temp, but a week or a month are far more reasonable than "never".
          Hide
          Petr Škoda added a comment - - edited

          One week for files is reasonable, but for directories there is a very small chance of race condition, but I think it would be ok to risk it. I suppose the top level dirs in tempdir should be kept and we could safely delete all old subdirs. In any case I am going to audit all temp file operations in master and fix any problem I see there.

          Show
          Petr Škoda added a comment - - edited One week for files is reasonable, but for directories there is a very small chance of race condition, but I think it would be ok to risk it. I suppose the top level dirs in tempdir should be kept and we could safely delete all old subdirs. In any case I am going to audit all temp file operations in master and fix any problem I see there.
          Hide
          Martín Langhoff added a comment -

          TBH, we aren't worried so much about core moodle, but non-core plugins.

          We do review plugins before they are installed on our hosted Moodle setup (at Remote Learner) but it is not feasible to sort out the cleanup strategies of every one of them.

          Show
          Martín Langhoff added a comment - TBH, we aren't worried so much about core moodle, but non-core plugins. We do review plugins before they are installed on our hosted Moodle setup (at Remote Learner) but it is not feasible to sort out the cleanup strategies of every one of them.
          Hide
          Tim Gus added a comment -

          Petr Škoda Martin and I already raised the issue with non-core plugins: will you handle those in addition to the core plugins?

          Show
          Tim Gus added a comment - Petr Škoda Martin and I already raised the issue with non-core plugins: will you handle those in addition to the core plugins?
          Hide
          Petr Škoda added a comment -

          Add-ons are not maintained by the HQ team, we can only encourage authors to fix them.

          Show
          Petr Škoda added a comment - Add-ons are not maintained by the HQ team, we can only encourage authors to fix them.
          Hide
          Tim Gus added a comment -

          If we can still get the one week clean-up going I have the patches attached to this issue.

          Show
          Tim Gus added a comment - If we can still get the one week clean-up going I have the patches attached to this issue.
          Hide
          Tyler Bannister added a comment -

          Petr Škoda I think Martin and Tim are suggesting that Moodle may want to do some minimal clean up for poorly behaving plugins.

          Show
          Tyler Bannister added a comment - Petr Škoda I think Martin and Tim are suggesting that Moodle may want to do some minimal clean up for poorly behaving plugins.
          Hide
          Tyler Bannister added a comment -

          Fixed pull branches.

          Show
          Tyler Bannister added a comment - Fixed pull branches.
          Hide
          Tyler Bannister added a comment -

          Added testing instructions.

          Show
          Tyler Bannister added a comment - Added testing instructions.
          Hide
          Tyler Bannister added a comment -

          Rajesh Taneja, this issues is ready for integration.

          [Y] Syntax
          [Y] Whitespace
          [-] Output
          [-] Language
          [-] Databases
          [Y] Testing (instructions and automated tests)
          [-] Security
          [Y] Documentation
          [Y] Git
          [Y] Sanity check
          
          Show
          Tyler Bannister added a comment - Rajesh Taneja , this issues is ready for integration. [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [Y] Documentation [Y] Git [Y] Sanity check
          Hide
          Petr Škoda added a comment -

          Hi, please do not hardcode directory permissions 0777, instead use $CFG->directorypermissions or use standard moodle functions for creating of temp dirs.

          Show
          Petr Škoda added a comment - Hi, please do not hardcode directory permissions 0777, instead use $CFG->directorypermissions or use standard moodle functions for creating of temp dirs.
          Hide
          Tim Gus added a comment -

          Updated from default function permissions to permissions from $CFG->directorypermissions.

          Show
          Tim Gus added a comment - Updated from default function permissions to permissions from $CFG->directorypermissions.
          Hide
          Petr Škoda added a comment -

          thanks!

          Show
          Petr Škoda added a comment - thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks Tim, Tyler and Petr for working on this.

          Tyler Bannister I am pushing this for integration, but I should not be assignee for this issue, as I haven't worked on it.
          Will check with Michael and see how we can make Tim as assignee.

          Show
          Rajesh Taneja added a comment - Thanks Tim, Tyler and Petr for working on this. Tyler Bannister I am pushing this for integration, but I should not be assignee for this issue, as I haven't worked on it. Will check with Michael and see how we can make Tim as assignee.
          Hide
          Michael de Raadt added a comment -

          Tim: I've added you to the developers group and assigned this issue to you. Thanks for your work.

          Your email address in the tracker seems incomplete. You are now able to edit issues, assign issues to yourself and submit issues for peer review. For more details, please see http://docs.moodle.org/dev/Tracker_guide#Tracker_groups_and_permissions

          Show
          Michael de Raadt added a comment - Tim: I've added you to the developers group and assigned this issue to you. Thanks for your work. Your email address in the tracker seems incomplete. You are now able to edit issues, assign issues to yourself and submit issues for peer review. For more details, please see http://docs.moodle.org/dev/Tracker_guide#Tracker_groups_and_permissions
          Hide
          Dan Poltawski added a comment -

          Thanks Tim. I like this change and i've integrated this to master

          Show
          Dan Poltawski added a comment - Thanks Tim. I like this change and i've integrated this to master
          Hide
          Petr Škoda added a comment -

          tests fail on Windows:

          1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass
          , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass
          , stdClass), array('C:\\server\\phpu_moodledata/temp/dir1', 'C:\\server\\phpu_mo
          odledata/temp/dir1/dir1_1', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_1/dir1_2
          ', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_1/dir1_2/file2', 'C:\\server\\php
          u_moodledata/temp/dir1/dir1_4', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_4/fi
          le2', 'C:\\server\\phpu_moodledata/temp/dir2', 'C:\\server\\phpu_moodledata/temp
          /file1'))
          Failed asserting that two arrays are equal.
          --- Expected
          +++ Actual
          @@ @@
           Array (
          -    0 => 'C:\server\phpu_moodledata/temp/dir1'
          -    1 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1'
          -    2 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1/dir1_2'
          -    3 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1/dir1_2/file2'
          -    4 => 'C:\server\phpu_moodledata/temp/dir1/dir1_4'
          -    5 => 'C:\server\phpu_moodledata/temp/dir1/dir1_4/file2'
          -    6 => 'C:\server\phpu_moodledata/temp/dir2'
          -    7 => 'C:\server\phpu_moodledata/temp/file1'
          +    0 => 'C:\server\phpu_moodledata/temp\dir1'
          +    1 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1'
          +    2 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1\dir1_2'
          +    3 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1\dir1_2\file2'
          +    4 => 'C:\server\phpu_moodledata/temp\dir1\dir1_4'
          +    5 => 'C:\server\phpu_moodledata/temp\dir1\dir1_4\file2'
          +    6 => 'C:\server\phpu_moodledata/temp\dir2'
          +    7 => 'C:\server\phpu_moodledata/temp\file1'
           )
          
          C:\server\workspace\moodle24\lib\tests\cronlib_test.php:153
          C:\server\workspace\moodle24\lib\phpunit\classes\basic_testcase.php:64
          
          To re-run:
           C:\server\workspace\moodle24\vendor\phpunit\phpunit\composer\bin\phpunit cronlib_testcase lib\tests\cronlib_test.php
          
          Show
          Petr Škoda added a comment - tests fail on Windows: 1) cronlib_testcase::test_cron_delete_from_temp with data set #0 (array(stdClass , stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass, stdClass , stdClass), array('C:\\server\\phpu_moodledata/temp/dir1', 'C:\\server\\phpu_mo odledata/temp/dir1/dir1_1', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_1/dir1_2 ', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_1/dir1_2/file2', 'C:\\server\\php u_moodledata/temp/dir1/dir1_4', 'C:\\server\\phpu_moodledata/temp/dir1/dir1_4/fi le2', 'C:\\server\\phpu_moodledata/temp/dir2', 'C:\\server\\phpu_moodledata/temp /file1')) Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( - 0 => 'C:\server\phpu_moodledata/temp/dir1' - 1 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1' - 2 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1/dir1_2' - 3 => 'C:\server\phpu_moodledata/temp/dir1/dir1_1/dir1_2/file2' - 4 => 'C:\server\phpu_moodledata/temp/dir1/dir1_4' - 5 => 'C:\server\phpu_moodledata/temp/dir1/dir1_4/file2' - 6 => 'C:\server\phpu_moodledata/temp/dir2' - 7 => 'C:\server\phpu_moodledata/temp/file1' + 0 => 'C:\server\phpu_moodledata/temp\dir1' + 1 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1' + 2 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1\dir1_2' + 3 => 'C:\server\phpu_moodledata/temp\dir1\dir1_1\dir1_2\file2' + 4 => 'C:\server\phpu_moodledata/temp\dir1\dir1_4' + 5 => 'C:\server\phpu_moodledata/temp\dir1\dir1_4\file2' + 6 => 'C:\server\phpu_moodledata/temp\dir2' + 7 => 'C:\server\phpu_moodledata/temp\file1' ) C:\server\workspace\moodle24\lib\tests\cronlib_test.php:153 C:\server\workspace\moodle24\lib\phpunit\classes\basic_testcase.php:64 To re-run: C:\server\workspace\moodle24\vendor\phpunit\phpunit\composer\bin\phpunit cronlib_testcase lib\tests\cronlib_test.php
          Hide
          Tim Gus added a comment -

          I've never tested this on Windows. The testing issue is with the function getPathname which mixes slashes as noted here: http://php.net/manual/en/splfileinfo.getpathname.php
          I've replaced getPathname in two places in my code to use getRealPath and updated the master branch. Testing still passes on my Linux machine.

          Show
          Tim Gus added a comment - I've never tested this on Windows. The testing issue is with the function getPathname which mixes slashes as noted here: http://php.net/manual/en/splfileinfo.getpathname.php I've replaced getPathname in two places in my code to use getRealPath and updated the master branch. Testing still passes on my Linux machine.
          Hide
          Tim Gus added a comment -

          I can now verify that the unit test passes on Windows too. I've made changes using the realpath function and the DIRECTORY_SEPARATOR constant.

          Show
          Tim Gus added a comment - I can now verify that the unit test passes on Windows too. I've made changes using the realpath function and the DIRECTORY_SEPARATOR constant.
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          Is there any chance you could provide your change as a patch on top of your previous patch? As we've already integrated that code I can't pull in the modified commit.

          Show
          Dan Poltawski added a comment - Hi Tim, Is there any chance you could provide your change as a patch on top of your previous patch? As we've already integrated that code I can't pull in the modified commit.
          Hide
          Dan Poltawski added a comment -
          Show
          Dan Poltawski added a comment - I constructed the diff myself as we're pressed for time: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=c4012e7e82186e082fff23d25be2c7d323be1ec9;hp=ef9ca1069921604be53a3f2561bed1f43eb64f54 I'm sending this back to testing.
          Hide
          Petr Škoda added a comment -

          works now, thanks

          Show
          Petr Škoda added a comment - works now, thanks
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.
          Hide
          Bei Chapman added a comment -

          This issue exists in 2.4.4 as well, and the solution works.

          Show
          Bei Chapman added a comment - This issue exists in 2.4.4 as well, and the solution works.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: