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

Delete tmp files and directories older than one week in cron

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.3
    • Fix Version/s: 2.6
    • Component/s: Administration
    • Labels:
    • 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

      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?

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

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

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

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

            @Petr Skoda?

            Show
            martinlanghoff Martín Langhoff added a comment - AFAICS, there is no reason for moodle cron not to delete old files, say 1 week old. @ Petr Skoda ?
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus Tim Gus added a comment -

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

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

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

            Show
            skodak Petr Skoda added a comment - Hello, why is there the $tempdir parameter? Why would you change it to anything else?
            Hide
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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 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 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            martinlanghoff Martín Langhoff added a comment -

            Petr Skoda: 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
            martinlanghoff Martín Langhoff added a comment - Petr Skoda : 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            martinlanghoff 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
            martinlanghoff 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
            tgus Tim Gus added a comment -

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

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

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

            Show
            skodak Petr Skoda added a comment - Add-ons are not maintained by the HQ team, we can only encourage authors to fix them.
            Hide
            tgus 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
            tgus 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
            tbannister Tyler Bannister added a comment -

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

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

            Fixed pull branches.

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

            Added testing instructions.

            Show
            tbannister Tyler Bannister added a comment - Added testing instructions.
            Hide
            tbannister 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
            tbannister 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus Tim Gus added a comment -

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

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

            thanks!

            Show
            skodak Petr Skoda added a comment - thanks!
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            salvetore 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
            salvetore 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Thanks Tim. I like this change and i've integrated this to master
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            tgus 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
            tgus 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
            tgus 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
            tgus 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
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -
            Show
            poltawski 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
            skodak Petr Skoda added a comment -

            works now, thanks

            Show
            skodak Petr Skoda added a comment - works now, thanks
            Hide
            damyon 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 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
            beichapman Bei Chapman added a comment -

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

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

            Removing the docs_required label as there doesn't seem to be anything needing documenting. If this is wrong, please describe what needs doing in a comment and re-add the docs_required label.

            Show
            tsala Helen Foster added a comment - Removing the docs_required label as there doesn't seem to be anything needing documenting. If this is wrong, please describe what needs doing in a comment and re-add the docs_required label.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13