Skip to content

fix: catch and log exception from share events - #54419

Merged
AndyScherzinger merged 1 commit into
masterfrom
share-event-catch
Aug 19, 2025
Merged

fix: catch and log exception from share events#54419
AndyScherzinger merged 1 commit into
masterfrom
share-event-catch

Conversation

@icewind1991

Copy link
Copy Markdown
Member

Instead of failing the rest of the request, just log and move on.

Don't let something like nextcloud/activity#2103 stop people from leaving shares without seeing error messages.

@icewind1991icewind1991 added this to the Nextcloud 32 milestone Aug 14, 2025
@icewind1991
icewind1991 requested a review from a team as a code ownerAugust 14, 2025 09:13
@icewind1991
icewind1991 requested review from ArtificialOwl, CarlSchwan and nfebe and removed request for a teamAugust 14, 2025 09:13
@icewind1991icewind1991 added the 3. to review Waiting for reviews label Aug 14, 2025
Comment threadlib/private/Share20/Manager.php Outdated
Signed-off-by: Robin Appelman <robin@icewind.nl>

@come-nccome-nc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why have this specifically for share events?
This is the case for any event, any listener can crash the request.

So I’d say either we have that in the dispatcher or we do not have it at all?

@icewind1991

Copy link
Copy Markdown
MemberAuthor

"Exceptions from events" seem to be used in at least some places Share20\Manager::verifyPassword seems to use it to do the password verification. (and thus doesn't get the catching treatment here). Not sure about others.

@AndyScherzinger
AndyScherzinger merged commit c026c7c into masterAug 19, 2025
207 of 211 checks passed
@AndyScherzinger
AndyScherzinger deleted the share-event-catch branch August 19, 2025 05:21
@skjnldsvskjnldsv mentioned this pull request Aug 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@icewind1991@susnux@CarlSchwan@come-nc@AndyScherzinger@skjnldsv