search

jcrodriguez-dis / moodle-mod_vpl

Virtual Programming Lab for Moodle (Module)
GNU General Public License v3.0
98 stars 85 forks source link

Fixed submission download not working #99

Closed t-schroeder closed 3 years ago

t-schroeder commented 4 years ago

Fixes #98

jcrodriguez-dis commented 4 years ago

Dear t-shroeder, thanks for your report and fix. Please, indicate the conditions to replicate the problem. I have used ZipArchive::CREATE, ZipArchive::OVERWRITE, and both, class name uppercase but all versions work.

Best regards, Juan Carlos.

t-schroeder commented 4 years ago

Hi, I just tried to replicate this myself and strangely now it's working for me with and without the OVERWRITE. I even rolled back to an older VPL and PHP version but I no longer get the error that I described in #98. When I originally encountered the error I was running PHP 7.1 on CentOS 7.

I remember finding this post when I was encountering the error, which I tried out and saw that it fixed the problem for me: https://forums.phpfreaks.com/topic/140414-invalid-or-unitialized-zip-object/

But I suppose it can't hurt to use CREATE as well as OVERWRITE here. I'd also be fine with closing the issue and the pull requests. Up to you.

t-schroeder commented 4 years ago

I looked further into this. This was apparently causing the problem: https://bugs.php.net/bug.php?id=79296

A workaround has meanwhile been implemented in php-zip which explains why I can no longer replicate this error: https://github.com/php/php-src/pull/5281 But as remi mentions in the pull request future PHP versions will no longer contain that workaround. So I guess we should keep both the CREATE and OVERWRITE flag to hopefully keep the code compatible with future PHP versions.

jcrodriguez-dis commented 4 years ago

The ZipArchive documentation https://www.php.net/manual/en/ziparchive.open.php says "Opens a new or existing zip archive for reading, writing or modifying. Since libzip 1.6.0, a empty file is not a valid archive any longer. "

also

"ZipArchive::CREATE (integer) Create the archive if it does not exist."

and

"ZipArchive::OVERWRITE (integer) If archive exists, ignore its current contents. In other words, handle it the same way as an empty archive."

For me, the manual is not fully clear.

I think that two fixes may be applied: 1) Using only OVERWRITE, supposing that the sentence "an empty file is not a valid archive any longer" not apply here. I don't know if using OVERWRITE|CREATE is correct.

2) Using only CREATE, but adding a suffix to the given temp filename, g.e. "vpl.zip", to create a new zip archive and then removing the file created by tempname().

I guess that 1) is the best.

t-schroeder commented 3 years ago

I think this can be closed,