[PR #4309] [CLOSED] Refactor Exception handling #6339

New Issue

OVERLORD · 2026-02-05T10:29:41+03:00

OVERLORD commented

2026-02-05 10:29:41 +03:00

📋 Pull Request Information

Original PR: https://github.com/BookStackApp/BookStack/pull/4309
Author: @devdot
Created: 6/15/2023
Status: ❌ Closed

Base: development ← Head: refactor-exceptions

📝 Commits (10+)

ecf99fa Add test showing the "HTTP 500 on not found" bug
9ba7d1e Fix "HTTP 500 on not found" bug #4290
321a459 Refactor exception handling by using interface
34d8268 Refactor notify exception to clean up api exception handling
9ddd81f Move handling of ApiAuthException to handler
34f24c2 Simplify exception handling in ApiTokenGuard
8265276 Modify HttpFetchException handle to log exception
d9cbc5e Make JsonDebugException to use proper interface
0214448 Add tests for Registration
3561b35 Partial cleanup of exception handling

📊 Changes

16 files changed (+273 additions, -113 deletions)

View changed files

📝 app/Access/Controllers/ConfirmEmailController.php (+4 -8)
📝 app/Access/Controllers/UserInviteController.php (+7 -14)
📝 app/Api/ApiTokenGuard.php (+9 -26)
📝 app/Exceptions/ApiAuthException.php (+34 -1)
📝 app/Exceptions/Handler.php (+2 -6)
📝 app/Exceptions/HttpFetchException.php (+17 -3)
📝 app/Exceptions/JsonDebugException.php (+5 -2)
📝 app/Exceptions/NotifyException.php (+42 -7)
📝 app/Exceptions/PrettyException.php (+37 -2)
➖ app/Exceptions/UnauthorizedException.php (+0 -16)
📝 app/Http/Middleware/ApiAuthenticate.php (+4 -20)
📝 app/Uploads/HttpFetcher.php (+2 -1)
📝 app/Uploads/UserAvatars.php (+6 -6)
📝 tests/Api/PagesApiTest.php (+21 -0)
📝 tests/Auth/RegistrationTest.php (+57 -0)
📝 tests/Uploads/AvatarTest.php (+26 -1)

📄 Description

While working on exceptions for #4290, I found exception handling to be quite fuzzy. Trying to untangle it all, I started working on this refactoring, but I'm not sure if this is really a helpful direction at all.

First of all, there are three major kinds of exceptions:

PrettyException, used for showing a good-looking exception to the user for those exceptions that cannot be reconciled from.
NotifyException, used for showing a notification (session flash) after a HTTP redirect, it is also JSON compatible (without a redirect in that case).
\Exception, the base exception mostly used for internal exceptions that ought to be caught in all cases (usually in controllers), therefore never exposed to user or API.

Now, I noticed the most common handling is with notifications of some kind:

by throwing exceptions that are never caught in controllers, like ConfirmationEmailException: https://github.com/devdot/BookStack/blob/development/app/Access/EmailConfirmationService.php#L23
by throwing internal exceptions and catching them in the controller, and then using Controller:showErrorNotification, like MoveOperationException https://github.com/devdot/BookStack/blob/development/app/Entities/Controllers/ChapterController.php#L188-L192
re-throwing exceptions because they should not show the technical message to the user, like here: https://github.com/devdot/BookStack/blob/development/app/Uploads/UserAvatars.php#L116-L118
re-throwing exceptions because they may need different messages for different controllers, like here: https://github.com/devdot/BookStack/blob/development/app/Access/Controllers/UserInviteController.php#L87-L100

So with all this observed, here is my question @ssddanbrown: Where are error notifications supposed to be handled? As I see it, there are these options:

Let exceptions bubble up from services, models, guards etc. into the exception handler. This would remove the exception handling from controllers entirely, making them cleaner and handling of these same types of exceptions would be the same (the NotifyException already creates proper HTTP responses/redirects, even for JSON). However, this would mean that services, models etc. determine how an exception should be handled, where it should be redirected to and what the message should be. That is a problem i.e. for the UserTokenService which currently has different error messages depending on the controller that accesses it.
Remove NotifyException altogether, make PrettyException the base class for all exceptions (perhaps rename to BaseException and let any notifying be handled by the controller, not the exception (and error handler). While this would potentially bloat controllers, the controllers get to decide how to present any error to the user. When the controllers do not catch exceptions, they would still be presentable (as extensions of PrettyException). However, this would eliminate the ease of throwing an error that will force a redirect with notification, which NotifyException currently does.
Leave it as it is.

To me, either 2 or 3 make to most sense.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/BookStackApp/BookStack/pull/4309 **Author:** [@devdot](https://github.com/devdot) **Created:** 6/15/2023 **Status:** ❌ Closed **Base:** `development` ← **Head:** `refactor-exceptions` --- ### 📝 Commits (10+) - [`ecf99fa`](https://github.com/BookStackApp/BookStack/commit/ecf99fa0edbddc5c47eb94b9cb3b5f22c65890ee) Add test showing the "HTTP 500 on not found" bug - [`9ba7d1e`](https://github.com/BookStackApp/BookStack/commit/9ba7d1e6c58c3730c343ea7730372f2890dbcfcc) Fix "HTTP 500 on not found" bug #4290 - [`321a459`](https://github.com/BookStackApp/BookStack/commit/321a4594219a39e9d10b601168e139c1b926e373) Refactor exception handling by using interface - [`34d8268`](https://github.com/BookStackApp/BookStack/commit/34d8268b2b20d62781b40274686b7492c890294a) Refactor notify exception to clean up api exception handling - [`9ddd81f`](https://github.com/BookStackApp/BookStack/commit/9ddd81fcf4895c1678690ac600e6c5b9c27951ca) Move handling of ApiAuthException to handler - [`34f24c2`](https://github.com/BookStackApp/BookStack/commit/34f24c273fb882124eb772f53f5808defd557ba0) Simplify exception handling in ApiTokenGuard - [`8265276`](https://github.com/BookStackApp/BookStack/commit/826527678bde09681817dfdc25cf67efbc311720) Modify HttpFetchException handle to log exception - [`d9cbc5e`](https://github.com/BookStackApp/BookStack/commit/d9cbc5e5cd79198fecb0da7363012a8e889d2be4) Make JsonDebugException to use proper interface - [`0214448`](https://github.com/BookStackApp/BookStack/commit/0214448979d646788337d4ee25f790e69e4a783d) Add tests for Registration - [`3561b35`](https://github.com/BookStackApp/BookStack/commit/3561b3568a845db8abccdc30a0a64fd48dc1880f) Partial cleanup of exception handling ### 📊 Changes **16 files changed** (+273 additions, -113 deletions) <details> <summary>View changed files</summary> 📝 `app/Access/Controllers/ConfirmEmailController.php` (+4 -8) 📝 `app/Access/Controllers/UserInviteController.php` (+7 -14) 📝 `app/Api/ApiTokenGuard.php` (+9 -26) 📝 `app/Exceptions/ApiAuthException.php` (+34 -1) 📝 `app/Exceptions/Handler.php` (+2 -6) 📝 `app/Exceptions/HttpFetchException.php` (+17 -3) 📝 `app/Exceptions/JsonDebugException.php` (+5 -2) 📝 `app/Exceptions/NotifyException.php` (+42 -7) 📝 `app/Exceptions/PrettyException.php` (+37 -2) ➖ `app/Exceptions/UnauthorizedException.php` (+0 -16) 📝 `app/Http/Middleware/ApiAuthenticate.php` (+4 -20) 📝 `app/Uploads/HttpFetcher.php` (+2 -1) 📝 `app/Uploads/UserAvatars.php` (+6 -6) 📝 `tests/Api/PagesApiTest.php` (+21 -0) 📝 `tests/Auth/RegistrationTest.php` (+57 -0) 📝 `tests/Uploads/AvatarTest.php` (+26 -1) </details> ### 📄 Description While working on exceptions for #4290, I found exception handling to be quite fuzzy. Trying to untangle it all, I started working on this refactoring, but I'm not sure if this is really a helpful direction at all. First of all, there are three major kinds of exceptions: - [PrettyException](https://github.com/BookStackApp/BookStack/blob/development/app/Exceptions/PrettyException.php), used for showing a good-looking exception to the user for those exceptions that cannot be reconciled from. - [NotifyException](https://github.com/BookStackApp/BookStack/blob/development/app/Exceptions/NotifyException.php), used for showing a notification (session flash) after a HTTP redirect, it is also JSON compatible (without a redirect in that case). - `\Exception`, the base exception mostly used for internal exceptions that ought to be caught in all cases (usually in controllers), therefore never exposed to user or API. Now, I noticed the most common handling is with notifications of some kind: - by throwing exceptions that are never caught in controllers, like `ConfirmationEmailException`: https://github.com/devdot/BookStack/blob/development/app/Access/EmailConfirmationService.php#L23 - by throwing internal exceptions and catching them in the controller, and then using `Controller:showErrorNotification`, like `MoveOperationException` https://github.com/devdot/BookStack/blob/development/app/Entities/Controllers/ChapterController.php#L188-L192 - re-throwing exceptions because they should not show the technical message to the user, like here: https://github.com/devdot/BookStack/blob/development/app/Uploads/UserAvatars.php#L116-L118 - re-throwing exceptions because they may need different messages for different controllers, like here: https://github.com/devdot/BookStack/blob/development/app/Access/Controllers/UserInviteController.php#L87-L100 So with all this observed, here is my question @ssddanbrown: Where are error notifications supposed to be handled? As I see it, there are these options: 1. Let exceptions bubble up from services, models, guards etc. into the exception handler. This would remove the exception handling from controllers entirely, making them cleaner and handling of these same types of exceptions would be the same (the `NotifyException` already creates proper HTTP responses/redirects, even for JSON). However, this would mean that services, models etc. determine how an exception should be handled, where it should be redirected to and what the message should be. That is a problem i.e. for the `UserTokenService` which currently has different error messages depending on the controller that accesses it. 2. Remove `NotifyException` altogether, make `PrettyException` the base class for all exceptions (perhaps rename to `BaseException` and let any notifying be handled by the controller, not the exception (and error handler). While this would potentially bloat controllers, the controllers get to decide how to present any error to the user. When the controllers do not catch exceptions, they would still be presentable (as extensions of `PrettyException`). However, this would eliminate the ease of throwing an error that will force a redirect with notification, which `NotifyException` currently does. 3. Leave it as it is. To me, either 2 or 3 make to most sense. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

OVERLORD added the pull-request label 2026-02-05 10:29:41 +03:00

OVERLORD closed this issue

2026-02-05 10:29:43 +03:00

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/BookStack#6339