Moodle
  1. Moodle
  2. MDL-32639

Filemanager download button problems

    Details

    • Testing Instructions:
      Hide

      Test pre-requisites

      • A few level of folders and files in your private files
      • Some empty directories in there

      Test steps

      1. Go to your private files
      2. Make sure you can "Download all" at any level
      3. Make sure the content of the "Download all" zip file is accurate (and does not start with a folder named 0)
      4. Make sure you stay in the same directory
      5. Make sure you CANNOT "Download all" when the directory is empty
      1. Go to your private files
      2. Click on any folder to zip it
      3. Make sure the content of the zip is correct
      4. Make sure you can zip a folder at any level
      5. Make sure the zip created does not overwrite any existing file (if the file already exist, _2 will be appended to the zip file name)

      Repeat with JavaScript turned off

      Master only test

      1. Make sure you can "Download all" when there are NO files (but folders) throughout the whole area
      Show
      Test pre-requisites A few level of folders and files in your private files Some empty directories in there Test steps Go to your private files Make sure you can "Download all" at any level Make sure the content of the "Download all" zip file is accurate (and does not start with a folder named 0) Make sure you stay in the same directory Make sure you CANNOT "Download all" when the directory is empty Go to your private files Click on any folder to zip it Make sure the content of the zip is correct Make sure you can zip a folder at any level Make sure the zip created does not overwrite any existing file (if the file already exist, _2 will be appended to the zip file name) Repeat with JavaScript turned off Master only test Make sure you can "Download all" when there are NO files (but folders) throughout the whole area
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-32639-master
    • Rank:
      39573

      Description

      There are several problems with 'Download dir' button in filemanager form element that I found:

      1) If there are files in filemanager but there are no files in the current folder, the button is still displayed.
      This happens because the obj.filescount contains total count of files and not count of files in the current subfolder. Since this variable is also used in dndupload, we need to introduce another counter

      2) After downloading folder on the second level, filemanager jumps to the root level. When trying to download folder on the third level, error is thrown.
      There is probably error in repository/draffiles_ajax.php

      3) In the downloaded zip archive the empty folders are skipped and the extra root folder with name "0" is created
      The same happens if I select 'Zip' in folder context menu in filemanager

        Issue Links

          Activity

          Hide
          Vadim Dvorovenko added a comment - - edited

          Preparation:
          Get Latest 2.4 version
          Create file resourse
          Create folder named "Folder" incide it
          Create subfolder named "Subfolder" inside it
          Create subfolder named "Subsubfolder" inside it
          Crete file "File.txt" inside it.
          Every time download created zip archive and look at structure in it.
          Test 1. Go to Files (root) folder. Click download All
          Expected: Folder\Subfolder\Subsubfolder\File.txt Get: 0\Folder\Subfolder\Subsubfolder\File.txt and two unnamed epmty folders.
          Test 2. Click right bottom corner of Folder, Zip it.
          Expected: Folder\Subfolder\Subsubfolder\File.txt Get: 0\Subfolder\Subsubfolder\File.txt and two unnamed epmty folders.
          Test 3. Go to Folder. Click Download All
          Expected: Subfolder\Subsubfolder\File.txt Get: 0\Subfolder\Subsubfolder\File.txt and unnamed epmty folder.
          Test 4. Click right bottom corner of Subfolder, Zip it.
          Expected: Subfolder\Subsubfolder\File.txt, archivename Subfolder.zip Get: 0\Subsubfolder\File.txt, unnamed epmty folder and archivename FolderSubfolder.zip (archivename bad only with ajax filemanager).
          I can see unnamed folder with Windows Winrar 4.11
          Fixed version here
          https://github.com/vadimonus/moodle.git branch MDL-32639-24
          diff: https://github.com/vadimonus/moodle/commit/58377282391731ab77581d6a350c80650bbe2130

          Show
          Vadim Dvorovenko added a comment - - edited Preparation: Get Latest 2.4 version Create file resourse Create folder named "Folder" incide it Create subfolder named "Subfolder" inside it Create subfolder named "Subsubfolder" inside it Crete file "File.txt" inside it. Every time download created zip archive and look at structure in it. Test 1. Go to Files (root) folder. Click download All Expected: Folder\Subfolder\Subsubfolder\File.txt Get: 0\Folder\Subfolder\Subsubfolder\File.txt and two unnamed epmty folders. Test 2. Click right bottom corner of Folder, Zip it. Expected: Folder\Subfolder\Subsubfolder\File.txt Get: 0\Subfolder\Subsubfolder\File.txt and two unnamed epmty folders. Test 3. Go to Folder. Click Download All Expected: Subfolder\Subsubfolder\File.txt Get: 0\Subfolder\Subsubfolder\File.txt and unnamed epmty folder. Test 4. Click right bottom corner of Subfolder, Zip it. Expected: Subfolder\Subsubfolder\File.txt, archivename Subfolder.zip Get: 0\Subsubfolder\File.txt, unnamed epmty folder and archivename FolderSubfolder.zip (archivename bad only with ajax filemanager). I can see unnamed folder with Windows Winrar 4.11 Fixed version here https://github.com/vadimonus/moodle.git branch MDL-32639 -24 diff: https://github.com/vadimonus/moodle/commit/58377282391731ab77581d6a350c80650bbe2130
          Hide
          Vadim Dvorovenko added a comment - - edited

          also 2.2 and 2.3 fixes
          https://github.com/vadimonus/moodle.git branches MDL-32639-22 & MDL-32639-23

          Show
          Vadim Dvorovenko added a comment - - edited also 2.2 and 2.3 fixes https://github.com/vadimonus/moodle.git branches MDL-32639 -22 & MDL-32639 -23
          Hide
          Sam Marshall added a comment -

          I happened to come across this issue and noticed nobody has looked at the proposed patches yet (I'm not sure if they are correct or not, I'm not familiar with this code). The issue isn't marked 'waiting for peer review' so perhaps it has fallen off people's radar - Vadim, you might want to try assigning it to yourself and then clicking 'submit for peer review' or 'request peer review' or whatever that button's called (assuming you have tracker permissions for that).

          Show
          Sam Marshall added a comment - I happened to come across this issue and noticed nobody has looked at the proposed patches yet (I'm not sure if they are correct or not, I'm not familiar with this code). The issue isn't marked 'waiting for peer review' so perhaps it has fallen off people's radar - Vadim, you might want to try assigning it to yourself and then clicking 'submit for peer review' or 'request peer review' or whatever that button's called (assuming you have tracker permissions for that).
          Hide
          Vadim Dvorovenko added a comment -

          Hi Sam. The problem that i do not have rights to assign issue to me, and i don't see 'request peer review' button. This button disappered from all issues more than half a year ago. I think 'request peer review' currently is disabled for ordinal users.

          Show
          Vadim Dvorovenko added a comment - Hi Sam. The problem that i do not have rights to assign issue to me, and i don't see 'request peer review' button. This button disappered from all issues more than half a year ago. I think 'request peer review' currently is disabled for ordinal users.
          Hide
          Michael de Raadt added a comment -

          It would be good if we could review the patch provided by Vadim.

          Show
          Michael de Raadt added a comment - It would be good if we could review the patch provided by Vadim.
          Hide
          Vadim Dvorovenko added a comment -

          I've created new issue only because i cannot request peer review here and no one from moodle HQ wtches this issue. Is requesting peer review disabled?

          Show
          Vadim Dvorovenko added a comment - I've created new issue only because i cannot request peer review here and no one from moodle HQ wtches this issue. Is requesting peer review disabled?
          Hide
          Marina Glancy added a comment -

          I am still listed as assignee, therefore only i can request peer review.
          But I don't know how to make you an assignee, you are not in the list

          For me your patch looks very good.

          Show
          Marina Glancy added a comment - I am still listed as assignee, therefore only i can request peer review. But I don't know how to make you an assignee, you are not in the list For me your patch looks very good.
          Hide
          Frédéric Massart added a comment -

          Thank you for your patch Vadim. I have pulled it into my branches. I have added a commit on top of it to take care of the two other issues mentioned in the description.

          • I removed the "Download all" button (JS and non-JS) when there is no file to download in the current folder. This does not work with tree view.
          • I fixed the refresh in the wrong directory when using "Download all".
          • I fixed the error while trying to download a sub-sub directory.

          You will noticed that I added a debug message in the zip_archive class when the zip file does not contain anything. In such case the zip is corrupted and we should probably not return it as a "successed zipping". Although, I did not change anything in the stable branches about that.

          Cheers!

          Show
          Frédéric Massart added a comment - Thank you for your patch Vadim. I have pulled it into my branches. I have added a commit on top of it to take care of the two other issues mentioned in the description. I removed the "Download all" button (JS and non-JS) when there is no file to download in the current folder. This does not work with tree view. I fixed the refresh in the wrong directory when using "Download all". I fixed the error while trying to download a sub-sub directory. You will noticed that I added a debug message in the zip_archive class when the zip file does not contain anything. In such case the zip is corrupted and we should probably not return it as a "successed zipping". Although, I did not change anything in the stable branches about that. Cheers!
          Hide
          Vadim Dvorovenko added a comment -

          Cheched on 2.3 verison. As for zip content - behaviour is always as expected.
          Just one strange behaviour with JS download button.
          Create folder inside root folder.
          Create subfolder in folder.
          Go to folder. Download All button is not visible in JS mode. That's abnormal behaviour, the folder contains subfolder. In non-js mode you can download well-formed zip file, wich contain empty subfolder. You can add many folders and subfolders, but download button will not be visible, until you upload any file.
          Go to root folder, upload any file. Download All will appear in any folder that contains subfolders, even if they're empty. That's normal behaviour.

          Show
          Vadim Dvorovenko added a comment - Cheched on 2.3 verison. As for zip content - behaviour is always as expected. Just one strange behaviour with JS download button. Create folder inside root folder. Create subfolder in folder. Go to folder. Download All button is not visible in JS mode. That's abnormal behaviour, the folder contains subfolder. In non-js mode you can download well-formed zip file, wich contain empty subfolder. You can add many folders and subfolders, but download button will not be visible, until you upload any file. Go to root folder, upload any file. Download All will appear in any folder that contains subfolders, even if they're empty. That's normal behaviour.
          Hide
          Vadim Dvorovenko added a comment -

          Frederic, please take a look at my patches on MDL-29749. It's other issue with filemanager/zipping. Testing instruction there.

          Show
          Vadim Dvorovenko added a comment - Frederic, please take a look at my patches on MDL-29749 . It's other issue with filemanager/zipping. Testing instruction there.
          Hide
          Frédéric Massart added a comment -

          Thanks a lot for testing the patch Vadim. Your feedback was really useful as I could simplify the logic in filemanager.js by only using existing CSS.

          The downside is that I could not properly fix the behaviour you mentioned for 2.3. As you said above, the filecount only contains the whole file count for the draft area. Which means that the download button will disappear if there is no files at all. I kept this for 2.3 as the logic in draftfiles_ajax will generate an error if no files are present in the area. I thought that improving that would be too much of an overkill. Although, I have fixed this in master only by extending the API. (Those comments did not apply for 2.2)

          Thank you!

          (I will try to have a look at MDL-29749 shortly.)

          Show
          Frédéric Massart added a comment - Thanks a lot for testing the patch Vadim. Your feedback was really useful as I could simplify the logic in filemanager.js by only using existing CSS. The downside is that I could not properly fix the behaviour you mentioned for 2.3. As you said above, the filecount only contains the whole file count for the draft area. Which means that the download button will disappear if there is no files at all. I kept this for 2.3 as the logic in draftfiles_ajax will generate an error if no files are present in the area. I thought that improving that would be too much of an overkill. Although, I have fixed this in master only by extending the API. (Those comments did not apply for 2.2) Thank you! (I will try to have a look at MDL-29749 shortly.)
          Hide
          Marina Glancy added a comment -

          Vadim, Fred, fix looks really great.

          There could be a performance improvement, actually not related to the issue itself. While doing review I noticed that function file_get_draft_area_info() is terribly inefficient. Basically all we want is to check if there are any files/subfolders in the area (or path). But instead of doing one simple count() query we retrieve all files and folders and create an instance of stored_file for each of them. There is a function file_storage::is_area_empty() but it does not really do what we need.
          Also file_get_draft_area_info() gets the total filesize regardless of whether file is reference or not. I would make another attribute in return value to show filesize of not-reference files.

          And couple of clarifications:

          Is there a check on filesize/areasize limit for the created zip file?

          Did you check how zip works if at least one of the files is a reference which content is not hashed? To be honest I don't know how to deal with that the best.

          Show
          Marina Glancy added a comment - Vadim, Fred, fix looks really great. There could be a performance improvement, actually not related to the issue itself. While doing review I noticed that function file_get_draft_area_info() is terribly inefficient. Basically all we want is to check if there are any files/subfolders in the area (or path). But instead of doing one simple count() query we retrieve all files and folders and create an instance of stored_file for each of them. There is a function file_storage::is_area_empty() but it does not really do what we need. Also file_get_draft_area_info() gets the total filesize regardless of whether file is reference or not. I would make another attribute in return value to show filesize of not-reference files. And couple of clarifications: Is there a check on filesize/areasize limit for the created zip file? Did you check how zip works if at least one of the files is a reference which content is not hashed? To be honest I don't know how to deal with that the best.
          Hide
          Vadim Dvorovenko added a comment -

          Hello, Marina. I was thinking a lot about your last question. That's what i think:
          Though Moodle file structure is very complex and has little common with ordinary file systems, when creating user interface for file management, we have to make it look like filemanager in modern operation system. That's why we want that zip behaviour be very simular to zip in windows and linux.
          So i think we should replicate behaviour of operating system, when we work with links.
          If look at Windows, there are .lnk files. We can represent file references with some files in archive, and put there information needed to restore reference after unzipping. Maybe such files should be placed in separate hidden directory. This is bad solution - after zipping it's difficult to distinguish such files from usual files.
          If look at linux, there are symbolic links in filesystem. When we're zipping files, they are zipped too. We can reproduce such behaviour by creating zip archive comment, containing service information about references (WinRar stores SFX params whis way). Moodle will use this comment to create references, while ordinal unzip will just ignore it.
          Regardless of OS links can cause problem, when unzipping to other place. That's why i don't like references ability in moodle. It makes moodle courses immovable between sites, when sites have different repository plugins configuration. That's why i think, that currently we have to disable zipping of references at all, and alert user on zipping and backuping course that references are not zipped.

          Show
          Vadim Dvorovenko added a comment - Hello, Marina. I was thinking a lot about your last question. That's what i think: Though Moodle file structure is very complex and has little common with ordinary file systems, when creating user interface for file management, we have to make it look like filemanager in modern operation system. That's why we want that zip behaviour be very simular to zip in windows and linux. So i think we should replicate behaviour of operating system, when we work with links. If look at Windows, there are .lnk files. We can represent file references with some files in archive, and put there information needed to restore reference after unzipping. Maybe such files should be placed in separate hidden directory. This is bad solution - after zipping it's difficult to distinguish such files from usual files. If look at linux, there are symbolic links in filesystem. When we're zipping files, they are zipped too. We can reproduce such behaviour by creating zip archive comment, containing service information about references (WinRar stores SFX params whis way). Moodle will use this comment to create references, while ordinal unzip will just ignore it. Regardless of OS links can cause problem, when unzipping to other place. That's why i don't like references ability in moodle. It makes moodle courses immovable between sites, when sites have different repository plugins configuration. That's why i think, that currently we have to disable zipping of references at all, and alert user on zipping and backuping course that references are not zipped.
          Hide
          Marina Glancy added a comment -

          Hi Vadim,
          Yes some warning messages would be nice. And absence of fatal errors too

          Show
          Marina Glancy added a comment - Hi Vadim, Yes some warning messages would be nice. And absence of fatal errors too
          Hide
          Frédéric Massart added a comment -

          Hi guys,

          I think, for now, the file references are added like other files if the contenthash is cached, and if not they're not. As easy as that. We could probably display a message to the user when a reference as not been included in the zip, I'll have a look at that.

          Regarding the questions raised during your peer review Marina, no there is no check on the file/area size when zipping a folder. But while thinking of it, I don't think it would be a great improvement to add one. Again, this security would rely on the parameters passed via Ajax (maxbytes, areamaxbytes) which can very easily be altered to allow the naughty user to fill up the draft area. Also, when validating the form, an error would prevent them from saving from the draft area to the 'real' area.

          I thought that there could be a problem when the Zip file already exists as we're overwriting it without prompting the user. I haven't pushed my updated branches yet but I used the repository::get_unused_filename() to prevent that. We could probably ask the user what they want to do, or how they'd like to rename the zip file, but I didn't look at it as it seemed to be more complicated and a bit out of the scope of this issue.

          Cheers,

          Fred

          Show
          Frédéric Massart added a comment - Hi guys, I think, for now, the file references are added like other files if the contenthash is cached, and if not they're not. As easy as that. We could probably display a message to the user when a reference as not been included in the zip, I'll have a look at that. Regarding the questions raised during your peer review Marina, no there is no check on the file/area size when zipping a folder. But while thinking of it, I don't think it would be a great improvement to add one. Again, this security would rely on the parameters passed via Ajax (maxbytes, areamaxbytes) which can very easily be altered to allow the naughty user to fill up the draft area. Also, when validating the form, an error would prevent them from saving from the draft area to the 'real' area. I thought that there could be a problem when the Zip file already exists as we're overwriting it without prompting the user. I haven't pushed my updated branches yet but I used the repository::get_unused_filename() to prevent that. We could probably ask the user what they want to do, or how they'd like to rename the zip file, but I didn't look at it as it seemed to be more complicated and a bit out of the scope of this issue. Cheers, Fred
          Hide
          Frédéric Massart added a comment -

          Requesting a peer review again. The only thing that I added is a commit to prevent the folder zipping to overwrite any existing file (which could be quite annoying for the user).

          I raised MDL-37123 to take care of warning the user when some references were not zipped. Also MDL-36708 to improve file_get_draft_area_info().

          Thanks guys!

          Show
          Frédéric Massart added a comment - Requesting a peer review again. The only thing that I added is a commit to prevent the folder zipping to overwrite any existing file (which could be quite annoying for the user). I raised MDL-37123 to take care of warning the user when some references were not zipped. Also MDL-36708 to improve file_get_draft_area_info(). Thanks guys!
          Hide
          Marina Glancy added a comment -

          looks good to me

          Show
          Marina Glancy added a comment - looks good to me
          Hide
          Frédéric Massart added a comment -

          Thanks Marina. Pushing for integration with label integration_help as master differs from 2.4. Cheers!

          Show
          Frédéric Massart added a comment - Thanks Marina. Pushing for integration with label integration_help as master differs from 2.4. Cheers!
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Vadim and Fred.

          I've not pulled the fix into 2.2 as this is now only in security support.

          Show
          Dan Poltawski added a comment - Integrated, thanks Vadim and Fred. I've not pulled the fix into 2.2 as this is now only in security support.
          Hide
          Dan Poltawski added a comment -

          Hi fred, looks like this is breaking phpunit

          Show
          Dan Poltawski added a comment - Hi fred, looks like this is breaking phpunit
          Hide
          Dan Poltawski added a comment -

          ebugging: Can not zip 'xtest.txt' file

          • line 120 of /lib/filestorage/zip_packer.php: call to debugging()
          • line 150 of /lib/filestorage/tests/zip_packer_test.php: call to zip_packer->archive_to_pathname()
          • line ? of unknownfile: call to zip_packer_testcase->test_archive_to_pathname()
          • line 967 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs()
          • line 825 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest()
          • line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare()
          • line 649 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare()
          • line 770 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run()
          • line 776 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run()
          • line 746 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest()
          • line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          • line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          • line 325 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run()
          • line 177 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun()
          • line 130 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run()
          • line 46 of /usr/local/Cellar/php53/5.3.16/bin/phpunit: call to PHPUnit_TextUI_Command::main()
            Debugging: Nothing was added to the zip file
          • line 143 of /lib/filestorage/zip_packer.php: call to debugging()
          • line 150 of /lib/filestorage/tests/zip_packer_test.php: call to zip_packer->archive_to_pathname()
          • line ? of unknownfile: call to zip_packer_testcase->test_archive_to_pathname()
          • line 967 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs()
          • line 825 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest()
          • line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare()
          • line 649 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare()
          • line 770 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run()
          • line 776 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run()
          • line 746 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest()
          • line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          • line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run()
          • line 325 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run()
          • line 177 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun()
          • line 130 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run()
          • line 46 of /usr/local/Cellar/php53/5.3.16/bin/phpunit: call to PHPUnit_TextUI_Command::main()
            FSSSS............................................... 732 / 1518 ( 48%)
          Show
          Dan Poltawski added a comment - ebugging: Can not zip 'xtest.txt' file line 120 of /lib/filestorage/zip_packer.php: call to debugging() line 150 of /lib/filestorage/tests/zip_packer_test.php: call to zip_packer->archive_to_pathname() line ? of unknownfile: call to zip_packer_testcase->test_archive_to_pathname() line 967 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs() line 825 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest() line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare() line 649 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare() line 770 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run() line 776 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run() line 746 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest() line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() line 325 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run() line 177 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun() line 130 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run() line 46 of /usr/local/Cellar/php53/5.3.16/bin/phpunit: call to PHPUnit_TextUI_Command::main() Debugging: Nothing was added to the zip file line 143 of /lib/filestorage/zip_packer.php: call to debugging() line 150 of /lib/filestorage/tests/zip_packer_test.php: call to zip_packer->archive_to_pathname() line ? of unknownfile: call to zip_packer_testcase->test_archive_to_pathname() line 967 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to ReflectionMethod->invokeArgs() line 825 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestCase->runTest() line 76 of /lib/phpunit/classes/advanced_testcase.php: call to PHPUnit_Framework_TestCase->runBare() line 649 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestResult.php: call to advanced_testcase->runBare() line 770 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestCase.php: call to PHPUnit_Framework_TestResult->run() line 776 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestCase->run() line 746 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->runTest() line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() line 706 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/Framework/TestSuite.php: call to PHPUnit_Framework_TestSuite->run() line 325 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/TestRunner.php: call to PHPUnit_Framework_TestSuite->run() line 177 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_TestRunner->doRun() line 130 of /usr/local/Cellar/php53/5.3.16/lib/php/PHPUnit/TextUI/Command.php: call to PHPUnit_TextUI_Command->run() line 46 of /usr/local/Cellar/php53/5.3.16/bin/phpunit: call to PHPUnit_TextUI_Command::main() FSSSS............................................... 732 / 1518 ( 48%)
          Hide
          Dan Poltawski added a comment -

          There was 1 failure:

          1) zip_packer_testcase::test_archive_to_pathname
          Expectation failed, debugging() triggered 2 times.

          /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:280
          /Users/danp/git/integration/lib/filestorage/tests/zip_packer_test.php:152
          /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76

          To re-run:
          /usr/local/Cellar/php53/5.3.16/bin/phpunit zip_packer_testcase lib/filestorage/tests/zip_packer_test.php

          Show
          Dan Poltawski added a comment - There was 1 failure: 1) zip_packer_testcase::test_archive_to_pathname Expectation failed, debugging() triggered 2 times. /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:280 /Users/danp/git/integration/lib/filestorage/tests/zip_packer_test.php:152 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76 To re-run: /usr/local/Cellar/php53/5.3.16/bin/phpunit zip_packer_testcase lib/filestorage/tests/zip_packer_test.php
          Hide
          Frédéric Massart added a comment -

          Sorry about that guys. The error is coming from a new debugging message which I have introduced in master only. The function that assert the debugging messages does not allow for more than one, hence the failing test. I have added a commit to hack the test so that it does not fail and raised MDL-37429 to properly fix this issue.

          Show
          Frédéric Massart added a comment - Sorry about that guys. The error is coming from a new debugging message which I have introduced in master only. The function that assert the debugging messages does not allow for more than one, hence the failing test. I have added a commit to hack the test so that it does not fail and raised MDL-37429 to properly fix this issue.
          Hide
          Dan Poltawski added a comment -

          Fred and I discussed this and agreed to revert back to the old output buffering behaviour in order to deal with the multiple debug messages, creating a new issue to work out how to properly solve this.

          Show
          Dan Poltawski added a comment - Fred and I discussed this and agreed to revert back to the old output buffering behaviour in order to deal with the multiple debug messages, creating a new issue to work out how to properly solve this.
          Hide
          Michael de Raadt added a comment -

          I found a discrepancy in 2.4. I was able to download a zip when the folder was empty.

          All other parts of the test were successful.

          Show
          Michael de Raadt added a comment - I found a discrepancy in 2.4. I was able to download a zip when the folder was empty. All other parts of the test were successful.
          Hide
          Frédéric Massart added a comment -

          Thanks for testing this Michael. Could you please try again making sure that you flushed your cache first (and browser cache as well)? There is a CSS rule that should prevent the button from being displayed when the current folder is empty. I have checked on my 2.4 and ended up having the same wrong behaviour, but after flushing the cache it appeared to work as expected. Thanks!

          Show
          Frédéric Massart added a comment - Thanks for testing this Michael. Could you please try again making sure that you flushed your cache first (and browser cache as well)? There is a CSS rule that should prevent the button from being displayed when the current folder is empty. I have checked on my 2.4 and ended up having the same wrong behaviour, but after flushing the cache it appeared to work as expected. Thanks!
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          After flushing caches, that functioned correctly. It's odd that it happened only in that one version.

          Can I also say that I'm really impressed with the current functionality. I think we have finally achieved a great file manager. Thanks to everyone involved.

          Show
          Michael de Raadt added a comment - Test result: Success! After flushing caches, that functioned correctly. It's odd that it happened only in that one version. Can I also say that I'm really impressed with the current functionality. I think we have finally achieved a great file manager. Thanks to everyone involved.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: