Skip to content

Zipdownload: various improvements to this plugin - #10260

Open
gurnec wants to merge 4 commits into
roundcube:masterfrom
gurnec:zipdownload-updates
Open

Zipdownload: various improvements to this plugin#10260
gurnec wants to merge 4 commits into
roundcube:masterfrom
gurnec:zipdownload-updates

Conversation

@gurnec

@gurnec gurnec commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

(Extracted from #10175.)

This includes several improvements to zipdownload. Feel free to cherry-pick what looks useful, or ask me to split this into multiple PRs, or whatever makes most sense.

  • Currently the entire zip is written to the output buffer before sending which can easily cause an OOM. Instead, flush every 512 KiB.
  • Update the .htaccess, see 9bd686c for details.
  • For maildir exports, change the modified-time of each .eml file to the IMAP internal time to allow sorting extracted emails.
  • Change the default charset in the config to UTF-8, see a6b857f.

gurnec added 4 commits July 13, 2026 13:48
 * Apache doesn't trust Content-Length headers by default; it will strip
   them and, only if length is known (on disk/all buffered), add it back
 * Brotli already avoided compressing files already compressed,
   do the same with the deflate method
 * Don't recompress zips created by the zipdownload plugin
It's already the default if unspecified, and the zip format has
supported UTF-8 since 2006, so there's little reason to do otherwise.

https://www.loc.gov/preservation/digital/formats/digformatspecs/APPNOTE(20060929)_Version_6.3.0.txt

$rcmail->output->download_headers($filename, ['length' => filesize($tmpfname)]);

readfile($tmpfname);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is not readfile(), but output buffering. Can't we just disable output buffering before calling readfile()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a very slight preference to use fread() over readfile() with large files because the latter uses mmap() which means:

  • pages mapped by the kernel don't count against PHP's memory_limit
  • memory usage is difficult to predict, and can change between kernel versions
  • PHP doesn't use madvise(..., MADV_SEQUENTIAL) which would make the situation better

In practice, mmap() is not some new unoptimized kernel feature, and I doubt its use would cause problems, so I'm happy to move back to readfile() if you'd prefer, just let me know.

}

// open zip file
putenv('TZ=UTC'); // see _datetime_to_ziplocal() comments

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this still be needed if we use setMtimeName() on every file?

@gurnec gurnec Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes. libzip expects times in UTC and always converts them to a local time unless zip_file_set_dostime() is used which ZipArchive doesn't expose. libzip uses localtime() to do the conversion which can be controlled by the TZ environment variable on POSIX and Windows. Different libcs have different ways of setting a default, for example glibc uses /etc/localtime (man tzset). Overriding the default to UTC was the best way I could find to convert these localtime() calls to a no-op.

(Note that I've only tested this on glibc and Windows.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants