From 0fca80e7e4af59a2346fcc116ac00c8cd5709a29 Mon Sep 17 00:00:00 2001
From: Jonny Barnes <jonny@jonnybarnes.uk>
Date: Sun, 9 Aug 2020 15:54:10 +0100
Subject: [PATCH] Improve exception handling and model binding

---
 app/Exceptions/Handler.php                    | 56 +++++++++----------
 app/Http/Controllers/ArticlesController.php   | 18 ++++--
 app/Http/Controllers/AuthController.php       |  9 +--
 app/Http/Controllers/BookmarksController.php  |  6 +-
 app/Http/Controllers/ContactsController.php   | 15 +++--
 app/Http/Controllers/FeedsController.php      |  6 +-
 app/Http/Controllers/FrontPageController.php  |  6 +-
 app/Http/Controllers/LikesController.php      |  6 +-
 app/Http/Controllers/MicropubController.php   |  8 +--
 app/Http/Controllers/NotesController.php      | 10 +++-
 app/Http/Controllers/PlacesController.php     | 10 ++--
 app/Http/Controllers/SearchController.php     |  2 +-
 .../Controllers/SessionStoreController.php    |  4 +-
 app/Http/Controllers/ShortURLsController.php  |  8 +--
 .../Controllers/TokenEndpointController.php   | 10 ++--
 app/Models/Place.php                          | 10 ++++
 routes/web.php                                |  4 +-
 tests/Feature/ArticlesTest.php                | 14 +++++
 tests/Feature/ContactsTest.php                |  7 +++
 tests/Feature/LikesTest.php                   |  7 +++
 tests/Feature/NotesControllerTest.php         |  7 +++
 tests/Feature/PlacesTest.php                  |  7 +++
 22 files changed, 148 insertions(+), 82 deletions(-)

diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index 3b156bc4..e81af7e0 100644
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -4,6 +4,7 @@ namespace App\Exceptions;
 
 use Exception;
 use GuzzleHttp\Client;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
@@ -23,7 +24,8 @@ class Handler extends ExceptionHandler
      * @var array
      */
     protected $dontReport = [
-        \Symfony\Component\HttpKernel\Exception\NotFoundHttpException::class,
+        NotFoundHttpException::class,
+        ModelNotFoundException::class,
     ];
 
     /**
@@ -49,35 +51,33 @@ class Handler extends ExceptionHandler
     {
         parent::report($throwable);
 
-        if ($throwable instanceof NotFoundHttpException) {
-            return;
-        }
+        if ($this->shouldReport($throwable)) {
+            $guzzle = new Client([
+                'headers' => [
+                    'Content-Type' => 'application/json',
+                ],
+            ]);
 
-        $guzzle = new Client([
-            'headers' => [
-                'Content-Type' => 'application/json',
-            ],
-        ]);
-
-        $guzzle->post(
-            config('logging.slack'),
-            [
-                'body' => json_encode([
-                    'attachments' => [[
-                        'fallback' => 'There was an exception.',
-                        'pretext' => 'There was an exception.',
-                        'color' => '#d00000',
-                        'author_name' => app()->environment(),
-                        'author_link' => config('app.url'),
-                        'fields' => [[
-                            'title' => get_class($throwable) ?? 'Unknown Exception',
-                            'value' => $throwable->getTraceAsString() ?? '',
+            $guzzle->post(
+                config('logging.slack'),
+                [
+                    'body' => json_encode([
+                        'attachments' => [[
+                            'fallback' => 'There was an exception.',
+                            'pretext' => 'There was an exception.',
+                            'color' => '#d00000',
+                            'author_name' => app()->environment(),
+                            'author_link' => config('app.url'),
+                            'fields' => [[
+                                'title' => get_class($throwable) ?? 'Unknown Exception',
+                                'value' => $throwable->getTraceAsString() ?? '',
+                            ]],
+                            'ts' => time(),
                         ]],
-                        'ts' => time(),
-                    ]],
-                ]),
-            ]
-        );
+                    ]),
+                ]
+            );
+        }
     }
 
     /**
diff --git a/app/Http/Controllers/ArticlesController.php b/app/Http/Controllers/ArticlesController.php
index c8e13a77..5b483474 100644
--- a/app/Http/Controllers/ArticlesController.php
+++ b/app/Http/Controllers/ArticlesController.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace App\Http\Controllers;
 
 use App\Models\Article;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\View\View;
 use Jonnybarnes\IndieWeb\Numbers;
@@ -14,8 +15,8 @@ class ArticlesController extends Controller
     /**
      * Show all articles (with pagination).
      *
-     * @param int $year
-     * @param int $month
+     * @param int|null $year
+     * @param int|null $month
      * @return View
      */
     public function index(int $year = null, int $month = null): View
@@ -38,7 +39,12 @@ class ArticlesController extends Controller
      */
     public function show(int $year, int $month, string $slug)
     {
-        $article = Article::where('titleurl', $slug)->firstOrFail();
+        try {
+            $article = Article::where('titleurl', $slug)->firstOrFail();
+        } catch (ModelNotFoundException $exception) {
+            abort(404);
+        }
+
         if ($article->updated_at->year != $year || $article->updated_at->month != $month) {
             return redirect('/blog/'
                             . $article->updated_at->year
@@ -59,7 +65,11 @@ class ArticlesController extends Controller
     public function onlyIdInUrl(int $idFromUrl): RedirectResponse
     {
         $realId = resolve(Numbers::class)->b60tonum($idFromUrl);
-        $article = Article::findOrFail($realId);
+        try {
+            $article = Article::findOrFail($realId);
+        } catch (ModelNotFoundException $exception) {
+            abort(404);
+        }
 
         return redirect($article->link);
     }
diff --git a/app/Http/Controllers/AuthController.php b/app/Http/Controllers/AuthController.php
index 67aa1f27..6efd237e 100644
--- a/app/Http/Controllers/AuthController.php
+++ b/app/Http/Controllers/AuthController.php
@@ -6,13 +6,14 @@ namespace App\Http\Controllers;
 
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Support\Facades\Auth;
+use Illuminate\View\View;
 
 class AuthController extends Controller
 {
     /**
      * Show the login form.
      *
-     * @return \Illuminate\View\View|\Illuminate\Http\RedirectResponse
+     * @return View|RedirectResponse
      */
     public function showLogin()
     {
@@ -27,7 +28,7 @@ class AuthController extends Controller
      * Log in a user, set a session variable, check credentials against
      * the .env file.
      *
-     * @return \Illuminate\Http\RedirectResponse
+     * @return RedirectResponse
      */
     public function login(): RedirectResponse
     {
@@ -43,7 +44,7 @@ class AuthController extends Controller
     /**
      * Show the form to logout a user.
      *
-     * @return \Illuminate\View\View|\Illuminate\Http\RedirectResponse
+     * @return View|RedirectResponse
      */
     public function showLogout()
     {
@@ -58,7 +59,7 @@ class AuthController extends Controller
     /**
      * Log the user out from their current session.
      *
-     * @return \Illuminate\Http\RedirectResponse;
+     * @return RedirectResponse;
      */
     public function logout(): RedirectResponse
     {
diff --git a/app/Http/Controllers/BookmarksController.php b/app/Http/Controllers/BookmarksController.php
index 8bd95d96..9b73e989 100644
--- a/app/Http/Controllers/BookmarksController.php
+++ b/app/Http/Controllers/BookmarksController.php
@@ -12,7 +12,7 @@ class BookmarksController extends Controller
     /**
      * Show the most recent bookmarks.
      *
-     * @return \Illuminate\View\View
+     * @return View
      */
     public function index(): View
     {
@@ -24,8 +24,8 @@ class BookmarksController extends Controller
     /**
      * Show a single bookmark.
      *
-     * @param  \App\Models\Bookmark  $bookmark
-     * @return \Illuminate\View\View
+     * @param Bookmark $bookmark
+     * @return View
      */
     public function show(Bookmark $bookmark): View
     {
diff --git a/app/Http/Controllers/ContactsController.php b/app/Http/Controllers/ContactsController.php
index 8550a79b..cd1272b5 100644
--- a/app/Http/Controllers/ContactsController.php
+++ b/app/Http/Controllers/ContactsController.php
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace App\Http\Controllers;
 
 use App\Models\Contact;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Filesystem\Filesystem;
 use Illuminate\View\View;
 
@@ -13,7 +14,7 @@ class ContactsController extends Controller
     /**
      * Show all the contacts.
      *
-     * @return \Illuminate\View\View
+     * @return View
      */
     public function index(): View
     {
@@ -34,17 +35,15 @@ class ContactsController extends Controller
     /**
      * Show a single contact.
      *
-     * @todo Use implicit model binding.
-     *
-     * @param  string  $nick The nickname associated with contact
-     * @return \Illuminate\View\View
+     * @param Contact $contact
+     * @return View
      */
-    public function show(string $nick): View
+    public function show(Contact $contact): View
     {
-        $filesystem = new Filesystem();
-        $contact = Contact::where('nick', '=', $nick)->firstOrFail();
         $contact->homepageHost = parse_url($contact->homepage, PHP_URL_HOST);
         $file = public_path() . '/assets/profile-images/' . $contact->homepageHost . '/image';
+
+        $filesystem = new Filesystem();
         $image = ($filesystem->exists($file)) ?
             '/assets/profile-images/' . $contact->homepageHost . '/image'
         :
diff --git a/app/Http/Controllers/FeedsController.php b/app/Http/Controllers/FeedsController.php
index 945609fc..b544d242 100644
--- a/app/Http/Controllers/FeedsController.php
+++ b/app/Http/Controllers/FeedsController.php
@@ -73,9 +73,9 @@ class FeedsController extends Controller
     /**
      * Returns the blog JSON feed.
      *
-     * @return \Illuminate\Http\JsonResponse
+     * @return array
      */
-    public function blogJson()
+    public function blogJson(): array
     {
         $articles = Article::where('published', '1')->latest('updated_at')->take(20)->get();
         $data = [
@@ -106,7 +106,7 @@ class FeedsController extends Controller
     /**
      * Returns the notes JSON feed.
      *
-     * @return \Illuminate\Http\JsonResponse
+     * @return array
      */
     public function notesJson()
     {
diff --git a/app/Http/Controllers/FrontPageController.php b/app/Http/Controllers/FrontPageController.php
index a5d86a9d..05731663 100644
--- a/app/Http/Controllers/FrontPageController.php
+++ b/app/Http/Controllers/FrontPageController.php
@@ -7,11 +7,15 @@ use App\Models\Bookmark;
 use App\Models\Like;
 use App\Models\Note;
 use App\Services\ActivityStreamsService;
+use Illuminate\Http\Response;
+use Illuminate\View\View;
 
 class FrontPageController extends Controller
 {
     /**
      * Show all the recent activity.
+     *
+     * @return Response|View
      */
     public function index()
     {
@@ -19,8 +23,6 @@ class FrontPageController extends Controller
             return (new ActivityStreamsService())->siteOwnerResponse();
         }
 
-        $pageNumber = request()->query('page') ?? 1;
-
         $notes = Note::latest()->get();
         $articles = Article::latest()->get();
         $bookmarks = Bookmark::latest()->get();
diff --git a/app/Http/Controllers/LikesController.php b/app/Http/Controllers/LikesController.php
index dbc92ec6..3bf5a1dd 100644
--- a/app/Http/Controllers/LikesController.php
+++ b/app/Http/Controllers/LikesController.php
@@ -12,7 +12,7 @@ class LikesController extends Controller
     /**
      * Show the latest likes.
      *
-     * @return \Illuminate\View\View
+     * @return View
      */
     public function index(): View
     {
@@ -24,8 +24,8 @@ class LikesController extends Controller
     /**
      * Show a single like.
      *
-     * @param  \App\Models\Like  $like
-     * @return \Illuminate\View\View
+     * @param Like $like
+     * @return View
      */
     public function show(Like $like): View
     {
diff --git a/app/Http/Controllers/MicropubController.php b/app/Http/Controllers/MicropubController.php
index fcccaa18..b5eff2d1 100644
--- a/app/Http/Controllers/MicropubController.php
+++ b/app/Http/Controllers/MicropubController.php
@@ -16,10 +16,10 @@ use MStaack\LaravelPostgis\Geometries\Point;
 
 class MicropubController extends Controller
 {
-    protected $tokenService;
-    protected $hentryService;
-    protected $hcardService;
-    protected $updateService;
+    protected TokenService $tokenService;
+    protected HEntryService $hentryService;
+    protected HCardService $hcardService;
+    protected UpdateService $updateService;
 
     public function __construct(
         TokenService $tokenService,
diff --git a/app/Http/Controllers/NotesController.php b/app/Http/Controllers/NotesController.php
index 72994314..4b60c256 100644
--- a/app/Http/Controllers/NotesController.php
+++ b/app/Http/Controllers/NotesController.php
@@ -6,7 +6,7 @@ namespace App\Http\Controllers;
 
 use App\Models\Note;
 use App\Services\ActivityStreamsService;
-use Illuminate\Contracts\View\Factory as ViewFactory;
+use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Response;
@@ -20,7 +20,7 @@ class NotesController extends Controller
     /**
      * Show all the notes. This is also the homepage.
      *
-     * @return ViewFactory|View|Response
+     * @return View|Response
      */
     public function index()
     {
@@ -45,7 +45,11 @@ class NotesController extends Controller
      */
     public function show(string $urlId)
     {
-        $note = Note::nb60($urlId)->with('webmentions')->firstOrFail();
+        try {
+            $note = Note::nb60($urlId)->with('webmentions')->firstOrFail();
+        } catch (ModelNotFoundException $exception) {
+            abort(404);
+        }
 
         if (request()->wantsActivityStream()) {
             return (new ActivityStreamsService())->singleNoteResponse($note);
diff --git a/app/Http/Controllers/PlacesController.php b/app/Http/Controllers/PlacesController.php
index f93ab259..7f6ea928 100644
--- a/app/Http/Controllers/PlacesController.php
+++ b/app/Http/Controllers/PlacesController.php
@@ -12,7 +12,7 @@ class PlacesController extends Controller
     /**
      * Show all the places.
      *
-     * @return \Illuminate\View\View
+     * @return View
      */
     public function index(): View
     {
@@ -24,13 +24,11 @@ class PlacesController extends Controller
     /**
      * Show a specific place.
      *
-     * @param  string  $slug
-     * @return \Illuminate\View\View
+     * @param Place $place
+     * @return View
      */
-    public function show(string $slug): View
+    public function show(Place $place): View
     {
-        $place = Place::where('slug', '=', $slug)->firstOrFail();
-
         return view('singleplace', ['place' => $place]);
     }
 }
diff --git a/app/Http/Controllers/SearchController.php b/app/Http/Controllers/SearchController.php
index b03c16d0..773f270c 100644
--- a/app/Http/Controllers/SearchController.php
+++ b/app/Http/Controllers/SearchController.php
@@ -12,7 +12,7 @@ class SearchController extends Controller
     /**
      * Display search results.
      *
-     * @return \Illuminate\View\View
+     * @return View
      */
     public function search(): View
     {
diff --git a/app/Http/Controllers/SessionStoreController.php b/app/Http/Controllers/SessionStoreController.php
index 042aa958..24665e1f 100644
--- a/app/Http/Controllers/SessionStoreController.php
+++ b/app/Http/Controllers/SessionStoreController.php
@@ -9,9 +9,9 @@ class SessionStoreController extends Controller
     /**
      * Save the selected colour scheme in the session.
      *
-     * @return \Illuminate\Http\JsonResponse
+     * @return string[]
      */
-    public function saveColour()
+    public function saveColour(): array
     {
         $css = request()->input('css');
 
diff --git a/app/Http/Controllers/ShortURLsController.php b/app/Http/Controllers/ShortURLsController.php
index 2510a7b0..941086f4 100644
--- a/app/Http/Controllers/ShortURLsController.php
+++ b/app/Http/Controllers/ShortURLsController.php
@@ -20,7 +20,7 @@ class ShortURLsController extends Controller
     /**
      * Redirect from '/' to the long url.
      *
-     * @return \Illuminate\Http\RedirectResponse
+     * @return RedirectResponse
      */
     public function baseURL(): RedirectResponse
     {
@@ -30,7 +30,7 @@ class ShortURLsController extends Controller
     /**
      * Redirect from '/@' to a twitter profile.
      *
-     * @return \Illuminate\Http\RedirectResponse
+     * @return RedirectResponse
      */
     public function twitter(): RedirectResponse
     {
@@ -40,7 +40,7 @@ class ShortURLsController extends Controller
     /**
      * Redirect from '/+' to a Google+ profile.
      *
-     * @return \Illuminate\Http\RedirectResponse
+     * @return RedirectResponse
      */
     public function googlePlus(): RedirectResponse
     {
@@ -53,7 +53,7 @@ class ShortURLsController extends Controller
      *
      * @param  string  Post type
      * @param  string  Post ID
-     * @return \Illuminate\Http\RedirectResponse
+     * @return RedirectResponse
      */
     public function expandType(string $type, string $postId): RedirectResponse
     {
diff --git a/app/Http/Controllers/TokenEndpointController.php b/app/Http/Controllers/TokenEndpointController.php
index 5a32aac4..a6a17311 100644
--- a/app/Http/Controllers/TokenEndpointController.php
+++ b/app/Http/Controllers/TokenEndpointController.php
@@ -13,18 +13,18 @@ class TokenEndpointController extends Controller
     /**
      * The IndieAuth Client.
      */
-    protected $client;
+    protected Client $client;
 
     /**
      * The Token handling service.
      */
-    protected $tokenService;
+    protected TokenService $tokenService;
 
     /**
      * Inject the dependencies.
      *
-     * @param  \IndieAuth\Client  $client
-     * @param  \App\Services\TokenService  $tokenService
+     * @param Client $client
+     * @param TokenService $tokenService
      */
     public function __construct(
         Client $client,
@@ -37,7 +37,7 @@ class TokenEndpointController extends Controller
     /**
      * If the user has auth’d via the IndieAuth protocol, issue a valid token.
      *
-     * @return \Illuminate\Http\JsonResponse
+     * @return JsonResponse
      */
     public function create(): JsonResponse
     {
diff --git a/app/Models/Place.php b/app/Models/Place.php
index e6e278bf..552225d1 100644
--- a/app/Models/Place.php
+++ b/app/Models/Place.php
@@ -60,6 +60,16 @@ class Place extends Model
     use Sluggable;
     use PostgisTrait;
 
+    /**
+     * Get the route key for the model.
+     *
+     * @return string
+     */
+    public function getRouteKeyName()
+    {
+        return 'slug';
+    }
+
     /**
      * The attributes that are mass assignable.
      *
diff --git a/routes/web.php b/routes/web.php
index 0551b4c6..0e4f2a73 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -156,11 +156,11 @@ Route::group(['domain' => config('url.longurl')], function () {
 
     // Contacts
     Route::get('contacts', 'ContactsController@index');
-    Route::get('contacts/{nick}', 'ContactsController@show');
+    Route::get('contacts/{contact:nick}', 'ContactsController@show');
 
     // Places
     Route::get('places', 'PlacesController@index');
-    Route::get('places/{slug}', 'PlacesController@show');
+    Route::get('places/{place}', 'PlacesController@show');
 
     Route::get('search', 'SearchController@search');
 
diff --git a/tests/Feature/ArticlesTest.php b/tests/Feature/ArticlesTest.php
index 9b584f40..18ed7212 100644
--- a/tests/Feature/ArticlesTest.php
+++ b/tests/Feature/ArticlesTest.php
@@ -30,4 +30,18 @@ class ArticlesTest extends TestCase
         $response = $this->get('/blog/s/2');
         $response->assertRedirect('/blog/' . date('Y') . '/' . date('m') . '/some-code-i-did');
     }
+
+    /** @test  */
+    public function unknownSlugGives404()
+    {
+        $response = $this->get('/blog/' . date('Y') . '/' . date('m') . '/unknown-slug');
+        $response->assertNotFound();
+    }
+
+    /** @test */
+    public function unknownArticleIdGives404()
+    {
+        $response = $this->get('/blog/s/22');
+        $response->assertNotFound();
+    }
 }
diff --git a/tests/Feature/ContactsTest.php b/tests/Feature/ContactsTest.php
index 8980c266..b9d99af6 100644
--- a/tests/Feature/ContactsTest.php
+++ b/tests/Feature/ContactsTest.php
@@ -38,4 +38,11 @@ class ContactsTest extends TestCase
         $response = $this->get('/contacts/aaron');
         $response->assertViewHas('image', '/assets/profile-images/aaronparecki.com/image');
     }
+
+    /** @test */
+    public function unknownContactGives404()
+    {
+        $response = $this->get('/contacts/unknown');
+        $response->assertNotFound();
+    }
 }
diff --git a/tests/Feature/LikesTest.php b/tests/Feature/LikesTest.php
index 0ad53ac8..a9da9f0b 100644
--- a/tests/Feature/LikesTest.php
+++ b/tests/Feature/LikesTest.php
@@ -211,4 +211,11 @@ END;
 
         $this->assertEquals('Jonny Barnes', Like::find($id)->author_name);
     }
+
+    /** @test */
+    public function unknownLikeGives404()
+    {
+        $response = $this->get('/likes/202');
+        $response->assertNotFound();
+    }
 }
diff --git a/tests/Feature/NotesControllerTest.php b/tests/Feature/NotesControllerTest.php
index 38585638..bf3bd79a 100644
--- a/tests/Feature/NotesControllerTest.php
+++ b/tests/Feature/NotesControllerTest.php
@@ -57,4 +57,11 @@ class NotesControllerTest extends TestCase
         $response = $this->get('/notes/tagged/beer');
         $response->assertViewHas('tag', 'beer');
     }
+
+    /** @test */
+    public function unknownNoteGives404()
+    {
+        $response = $this->get('/notes/112233');
+        $response->assertNotFound();
+    }
 }
diff --git a/tests/Feature/PlacesTest.php b/tests/Feature/PlacesTest.php
index 64b34fd5..0940d40e 100644
--- a/tests/Feature/PlacesTest.php
+++ b/tests/Feature/PlacesTest.php
@@ -29,4 +29,11 @@ class PlacesTest extends TestCase
         $response = $this->get('/places/the-bridgewater-pub');
         $response->assertViewHas('place', $place);
     }
+
+    /** @test */
+    public function unknownPlaceGives404()
+    {
+        $response = $this->get('/places/unknown');
+        $response->assertNotFound();
+    }
 }