Exception when rendering breadcrumbs on Laravel 404 error page

[lewislarsen]

When overriding the default 404 template with a template that uses breadcrumbs in the view, and navigating to a implictly bound route that 404's you encounter the following error:

Diglactic\Breadcrumbs\ServiceProvider::{closure}(): Argument #2 ($directory) must be of type App\Models\Directory, string given

Here's the relevant breadcrumbs

Breadcrumbs::for('directory.show', function (BreadcrumbTrail $trail, Directory $directory) {
    $trail->parent('directory.index');
    $trail->push($directory->title, route('directory.show', ['directory' => $directory]));
});
// 404
Breadcrumbs::for('errors.404', function (BreadcrumbTrail $trail) {
    $trail->push('Page Not Found');
});

Would you happen to know the best way to resolve this? Going to look into it myself too. :)

[shengslogar]

Hmm... it's not immediately clear why this would be happening, I'll need to try and recreate this.

The obvious issue is somehow the breadcrumb for directory.show is getting called when only the errors.404 breadcrumb should be, hence the type exception. You might try replacing the Directory $directory type with an anonymous $directory to see what's being passed through.

Can you show me how you're calling Breadcrumbs::render in your Blade templates?

[shengslogar]

I'm not able to repro this. You'll need to post additional source code.

[lewislarsen]

Here's some more, hope this helps!

app.blade.php (which is extended on the show blade template & 404 template)

<!-- Breadcrumb -->
{{ Breadcrumbs::render() }}

web.php section

Route::namespace('directory')->name('directory.')->prefix('directory')->group(function () {
    Route::get('/', [IndexController::class, '__invoke'])->name('index');
    Route::get('/view/{directory}', [ShowDirectoryController::class, '__invoke'])->name('show');
    });

The model's default route key name is a slug, it probably doesn't matter but I wanted to mention it anyway!

Using Laravel version 8.7.5 & breadcrumbs version 7.0.1. Let me know if there's anything specific you want to see, it is doing this on every page that has route parameters that 404s looking at my failing tests. (I don't have any routes with parameters that don't use implicit binding)

[shengslogar]

Thanks. Was able to repro. Will need to look into this.

Edit:

Issue specifically relates to 404s on named routes.

https://github.com/diglactic/laravel-breadcrumbs/blob/0a14e8785fa9423c878edd3975af2a92dfcdaf42/src/Manager.php#L241-L243

[shengslogar]

Ok, after thinking over this for awhile, I've decided to close out this issue without implementing a fix. Technically, the package is doing the correct behavior. If your controller were to fire with the same parameters being passed breadcrumbs, it, too, would throw a similar exception.

// breadcrumbs.php
Breadcrumbs::for('users.show', function(BreadcrumbTrail $trail, User $user){
   // ...
});

// UserController.php

public function show (User $user) { // This never fires on 404; if it did, there would be a type mismatch
   // ...
}

While messy, it seems to me that this variability should be handled at the breadcrumb definition level.

Breadcrumbs::for('users.show', function(BreadcrumbsTrail $trail, $user) {
    if ($user instanceof User) {
          $trail->push($user->name);
    } else {
          $trail->push('User not found');
    }
});

There's a reason why the default Laravel error pages don't have any app-related data on them because that just introduces additional points of failure.

There is a workaround that could help you avoid this. Whenever Laravel renders an error page, it passes an $exception variable to your Blade templates. That allows you to write:

@unless (isset($exception))
     {{ Breadcrumbs::render() }}
@endunless

Now, breadcrumbs will be entirely missing whenever an error is rendered. This makes sense to me, since breadcrumbs often pull data from models and the database which could cause a stack overflow.

Please voice any counterarguments, but for now I'm considering this an anti-pattern to the package.

[Tofandel]

I'm also running into the same problem, but the issue doesn't come from being a 404 otherwise the Breadcrumbs::for('errors.404') would be running and this issue wouldn't happen, the problem comes from the fact that laravel still gives the matched route in $router->current() even though it didn't find a matching model, this results in the string being passed instead of the $model

I'm using Breadcrumbs::generate() in Inertia and also Breadcrumbs::current() to retrieve the title of the page so the workaround doesn't really work for that, and a try catch doesn't work as well

Adding mixed parameters with cases to each breadcrumb route would be really really bad, first because it's cumbersome and long to write and second because we loose autocompletion (it could be typed | string though which would be correct but not really better)

Where it is inadequate, is that laravel doesn't run the controller if there is a model required to run the controller that is missing

What could be done is maybe some Reflection on the closure to fail to errors.404 in case of missing required param

[Tofandel]

I have found a more reliable workaround

Adding this in the top of the ExceptionHandler's render method

Breadcrumbs::setCurrentRoute('error', []);

To bypass the breadcrumbs with whatever you want in case of an error

This might be worth documenting