Updated not-found image path handling to have better ux

Added test to cover.
Started refactoring some of the app error handling in
the process of this.

Fixes #2696

diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index 19689716431bcaa2780a79ba089fab2f508e17a7..8d3a743faf939bd18b101c8e982a0d30afc4ad4e 100644 (file)
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -68,19 +68,6 @@ class Handler extends ExceptionHandler
             return redirect($e->redirectLocation);
         }
 
-        // Handle pretty exceptions which will show a friendly application-fitting page
-        // Which will include the basic message to point the user roughly to the cause.
-        if ($this->isExceptionType($e, PrettyException::class)  && !config('app.debug')) {
-            $message = $this->getOriginalMessage($e);
-            $code = ($e->getCode() === 0) ? 500 : $e->getCode();
-            return response()->view('errors/' . $code, ['message' => $message], $code);
-        }
-
-        // Handle 404 errors with a loaded session to enable showing user-specific information
-        if ($this->isExceptionType($e, NotFoundHttpException::class)) {
-            return \Route::respondWithRoute('fallback');
-        }
-
         return parent::render($request, $e);
     }
 

diff --git a/app/Exceptions/PrettyException.php b/app/Exceptions/PrettyException.php
index 7fad7df45812e1fc2e75881e50af063b07c23ff5..af60c3d06c93c2919275b1c9c11655a278a780cd 100644 (file)
--- a/app/Exceptions/PrettyException.php
+++ b/app/Exceptions/PrettyException.php
@@ -1,6 +1,44 @@
 <?php namespace BookStack\Exceptions;
 
-class PrettyException extends \Exception
+use Exception;
+use Illuminate\Contracts\Support\Responsable;
+use Illuminate\Http\Request;
+
+class PrettyException extends Exception implements Responsable
 {
+    /**
+     * @var ?string
+     */
+    protected $subtitle = null;
+
+    /**
+     * @var ?string
+     */
+    protected $details = null;
+
+    /**
+     * Render a response for when this exception occurs.
+     * @param Request $request
+     */
+    public function toResponse($request)
+    {
+        $code = ($this->getCode() === 0) ? 500 : $this->getCode();
+        return response()->view('errors.' . $code, [
+            'message' => $this->getMessage(),
+            'subtitle' => $this->subtitle,
+            'details' => $this->details,
+        ], $code);
+    }
+
+    public function setSubtitle(string $subtitle): self
+    {
+        $this->subtitle = $subtitle;
+        return $this;
+    }
 
+    public function setDetails(string $details): self
+    {
+        $this->details = $details;
+        return $this;
+    }
 }

diff --git a/app/Http/Controllers/Images/ImageController.php b/app/Http/Controllers/Images/ImageController.php
index ecc36bf67e24ad531f83326ed32d22bf4f97f63d..1eb8917b360edb2fb90ddaff26eff1e2028d137a 100644 (file)
--- a/app/Http/Controllers/Images/ImageController.php
+++ b/app/Http/Controllers/Images/ImageController.php
@@ -1,6 +1,7 @@
 <?php namespace BookStack\Http\Controllers\Images;
 
 use BookStack\Exceptions\ImageUploadException;
+use BookStack\Exceptions\NotFoundException;
 use BookStack\Http\Controllers\Controller;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageRepo;
@@ -27,12 +28,15 @@ class ImageController extends Controller
 
     /**
      * Provide an image file from storage.
+     * @throws NotFoundException
      */
     public function showImage(string $path)
     {
         $path = storage_path('uploads/images/' . $path);
         if (!file_exists($path)) {
-            abort(404);
+            throw (new NotFoundException(trans('errors.image_not_found')))
+                ->setSubtitle(trans('errors.image_not_found_subtitle'))
+                ->setDetails(trans('errors.image_not_found_details'));
         }
 
         return response()->file($path);

diff --git a/resources/lang/en/errors.php b/resources/lang/en/errors.php
index 79024e482ed69efa633116592f9b7c83a0bcc93a..eb8ba54ea81506519a4958769d54086cc3b3d7be 100644 (file)
--- a/resources/lang/en/errors.php
+++ b/resources/lang/en/errors.php
@@ -83,6 +83,9 @@ return [
     '404_page_not_found' => 'Page Not Found',
     'sorry_page_not_found' => 'Sorry, The page you were looking for could not be found.',
     'sorry_page_not_found_permission_warning' => 'If you expected this page to exist, you might not have permission to view it.',
+    'image_not_found' => 'Image Not Found',
+    'image_not_found_subtitle' => 'Sorry, The image file you were looking for could not be found.',
+    'image_not_found_details' => 'If you expected this image to exist it might have been deleted.',
     'return_home' => 'Return to home',
     'error_occurred' => 'An Error Occurred',
     'app_down' => ':appName is down right now',

diff --git a/resources/views/errors/404.blade.php b/resources/views/errors/404.blade.php
index 02f97fc546fcbc195ec04421731fba44b15cfb1d..b3325ba828805da62ae78a7d3ca0b0fe8cd81bff 100644 (file)
--- a/resources/views/errors/404.blade.php
+++ b/resources/views/errors/404.blade.php
@@ -7,8 +7,8 @@
         <div class="grid half v-center">
             <div>
                 <h1 class="list-heading">{{ $message ?? trans('errors.404_page_not_found') }}</h1>
-                <h5>{{ trans('errors.sorry_page_not_found') }}</h5>
-                <p>{{ trans('errors.sorry_page_not_found_permission_warning') }}</p>
+                <h5>{{ $subtitle ?? trans('errors.sorry_page_not_found') }}</h5>
+                <p>{{ $details ?? trans('errors.sorry_page_not_found_permission_warning') }}</p>
             </div>
             <div class="text-right">
                 @if(!signedInUser())

diff --git a/routes/web.php b/routes/web.php
index 9d482dc41a8e52bfc842eeca7a5cc364b759d44e..730c795f8a06857bc95cb24ecde483f8bf29a991 100644 (file)
--- a/routes/web.php
+++ b/routes/web.php
@@ -254,4 +254,4 @@ Route::post('/password/email', 'Auth\ForgotPasswordController@sendResetLinkEmail
 Route::get('/password/reset/{token}', 'Auth\ResetPasswordController@showResetForm');
 Route::post('/password/reset', 'Auth\ResetPasswordController@reset');
 
-Route::fallback('HomeController@getNotFound');
\ No newline at end of file
+Route::fallback('HomeController@getNotFound')->name('fallback');
\ No newline at end of file

diff --git a/tests/ErrorTest.php b/tests/ErrorTest.php
index 1558df78d1c200d59b29d559ceef38f687b81e4f..6b69355fcd3a8cc218469557e0b45b48258e190c 100644 (file)
--- a/tests/ErrorTest.php
+++ b/tests/ErrorTest.php
@@ -38,4 +38,11 @@ class ErrorTest extends TestCase
 
         $this->assertCount(1, $handler->getRecords());
     }
+
+    public function test_access_to_non_existing_image_location_provides_404_response()
+    {
+        $resp = $this->actingAs($this->getViewer())->get('/uploads/images/gallery/2021-05/anonexistingimage.png');
+        $resp->assertStatus(404);
+        $resp->assertSeeText('Image Not Found');
+    }
 }
\ No newline at end of file