Improve user experience when downloading archives by salmart-dev · Pull Request #59049 · nextcloud/server

salmart-dev · 2026-03-18T17:25:39Z

Resolves: [Enhancement] ZipFolderPlugin fails silently on errors #58825

Took over some of the changes from #57335, thanks @Koc

Summary

This PR adds an alternative working mode to the ZipFolderPlugin.

By setting the new archive_report_missing_files to true (default false) in config.php administrators can allow users to download partial archives when those contain a file that is blocked or missing/corrupted files.

The users will be informed of this via the addition of a missing_files.json inside the archive itself, giving the user some minimal information about why the files are missing.

Why is this needed

The ZipFolderPlugin currently halts the streaming of an archive for download due to various factors, such as storage issues with a file, temporary database problems, or a lost network connection. Because the plugin streams without a content-length header, the user receives an incomplete archive without any warning. In some cases, this issue only becomes apparent when attempting to open a file in the archive and discovering that the entire archive is corrupted.

What changes

Behaviour	Before	After
Downloading a directory containing a blocked file	Whole download denied	Download possible with `missing_files.json` containing an entry for the blocked file
Downloading a directory with an unstable file backend	Corrupted archive	Valid archive with partial files and entry in `missing_files.json` reporting the issue
Downloading a directory with files that cannot be opened because of X	Corrupted archive	Valid archive with missing file and entry in `missing_files.json` reporting the reason

Note: this will need to be backported down to stable29

TODO

Write tests
Translations
Update documentation where needed
May need a patch to deepdiver/zipstreamer to avoid slow tests due to loss of precision

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

The content of this PR was partly or fully generated using AI

susnux · 2026-03-31T09:42:52Z

so only admin configured, no user option if first failed?

salmart-dev · 2026-04-02T11:18:12Z

so only admin configured, no user option if first failed?

We could add this option later, right? We need this to work on 29 too 🙈

icewind1991 · 2026-04-02T14:02:50Z

+				}
+
+				$byteCounter = new StreamByteCounter();
+				$wrapped = stream_filter_append($stream, 'count.bytes', STREAM_FILTER_READ, ['counter' => $byteCounter]);


There is already the CountWrapper that can be used to track how many bytes get read from a stream.

Dropped the ByteCountFilter and switched to CountWrapper 👍

icewind1991 · 2026-04-02T15:30:11Z

It might make the code clearer if the error reporting code in the inner logic was always active, and only have the outer logic conditionally throw/abort or generate the error report.

icewind1991 · 2026-04-02T15:30:32Z

The overall logic looks good

joshtrichards

Embedding missing_files.json is a meaningful improvement over silently producing corrupted/incomplete archives (never good).

The UX concern that a “successful” download (archive) with missing files is still too easy to misunderstand/overlook remains. missing_files.json is better than nothing, but many users will not inspect it. I don't have a good answer here.
The opt-in flag seems like a good idea, but less certain upon reflection. If we need a toggle for straightforward (from the end-user perspective) functionality like this it may be a sign the implementation and design still needs further refinement before we commit (or, alternatively,we should drop the flag and accept it as the new default behavior!)
Admittedly, sometimes the best we can do is improve the UX until the next "go around". ;-)

For cases known ahead of time (e.g. filtered/blocked files), it may still be worth considering an additional visible signal that the archive is incomplete.

joshtrichards · 2026-04-13T01:21:17Z

+		}
+
+		if (!($streamMetadata['eof'] ?? true) || $readFileSize != $expectedFileSize) {
+			return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$expectedFileSize, $readFileSize]);


Suggested change

return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$expectedFileSize, $readFileSize]);

return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$readFileSize, $expectedFileSize]);

nice catch! addressed 👍

joshtrichards · 2026-04-13T01:35:01Z

+				// opening failed, log the failure as reason for the missing file
+				if ($this->reportMissingFiles) {
+					$exceptionClass = get_class($e);
+					return $this->l10n->t('Error while opening the file: %s', [$exceptionClass]);


Not sure about reporting the class name here. May be useful to admin/support (but then it should be logged not shown), but it is not end-user friendly. Maybe even just use the below (from stream === false check) for now?

This code has been changed, now the error is logged when the exception is not re-thrown.

joshtrichards · 2026-04-13T01:42:33Z

+		$this->reportMissingFiles = $this->config->getSystemValueBool('archive_report_missing_files', false);
+
+		if ($this->reportMissingFiles) {
+			stream_filter_register('count.bytes', ByteCounterFilter::class);


guard? / return value is currently ignored

Also this has changed, now using another wrapper and the return value is checked not to be false 👍

…wnload folder with non-downloadable files Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

salmart-dev · 2026-04-29T09:13:46Z

It might make the code clearer if the error reporting code in the inner logic was always active, and only have the outer logic conditionally throw/abort or generate the error report.

Makes sense: I removed all ifs inside the inner logic and just returning a reason in case of failure. Then the outer code decides what to do.

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

salmart-dev changed the title ~~Improve user feedback for downloaded archives~~ Improve user experience when downloading archives Mar 18, 2026

salmart-dev mentioned this pull request Mar 18, 2026

Inform users of missing files in archive #58895

Closed

10 tasks

salmart-dev self-assigned this Mar 18, 2026

salmart-dev added the 2. developing Work in progress label Mar 18, 2026

salmart-dev added this to the Nextcloud 34 milestone Mar 18, 2026

salmart-dev force-pushed the feat/zipfolderplugin-error-handling branch from a9a469d to 6e8ee13 Compare March 18, 2026 17:48

salmart-dev added the enhancement label Mar 18, 2026

salmart-dev marked this pull request as ready for review March 18, 2026 17:49

salmart-dev requested a review from a team as a code owner March 18, 2026 17:49

salmart-dev requested review from Altahrim, ArtificialOwl, icewind1991 and leftybournes and removed request for a team March 18, 2026 17:49

salmart-dev added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 18, 2026

salmart-dev force-pushed the feat/zipfolderplugin-error-handling branch 2 times, most recently from b8e07ba to fbdef2a Compare March 19, 2026 13:01

salmart-dev added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 27, 2026

salmart-dev marked this pull request as draft March 27, 2026 13:50

salmart-dev force-pushed the feat/zipfolderplugin-error-handling branch 3 times, most recently from d90be1a to ccc00aa Compare March 30, 2026 13:39

salmart-dev marked this pull request as ready for review March 30, 2026 19:16

salmart-dev added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 30, 2026

susnux approved these changes Apr 2, 2026

View reviewed changes

icewind1991 reviewed Apr 2, 2026

View reviewed changes

joshtrichards reviewed Apr 13, 2026

View reviewed changes

Koc and others added 6 commits April 29, 2026 10:52

feat(files_sharing): Improve user experience when user attempts to do…

fa065eb

…wnload folder with non-downloadable files Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

feat: improve ZipFolderPlugin behaviour for different cases

49034df

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

fixup! feat: improve ZipFolderPlugin behaviour for different cases

c6e68f1

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

fixup! feat: improve ZipFolderPlugin behaviour for different cases

3d681ea

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

fixup! feat: improve ZipFolderPlugin behaviour for different cases

e0b0810

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

fixup! feat: improve ZipFolderPlugin behaviour for different cases

df9c1be

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

salmart-dev force-pushed the feat/zipfolderplugin-error-handling branch from e5b982a to df9c1be Compare April 29, 2026 09:12

fixup! feat: improve ZipFolderPlugin behaviour for different cases

468aab2

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

	return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$expectedFileSize, $readFileSize]);
	return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$readFileSize, $expectedFileSize]);

Uh oh!

Conversation

salmart-dev commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why is this needed

What changes

TODO

Checklist

AI (if applicable)

Uh oh!

susnux commented Mar 31, 2026

Uh oh!

salmart-dev commented Apr 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icewind1991 commented Apr 2, 2026

Uh oh!

icewind1991 commented Apr 2, 2026

Uh oh!

joshtrichards left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salmart-dev commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

salmart-dev commented Mar 18, 2026 •

edited

Loading

joshtrichards left a comment •

edited

Loading