Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Add exception as parameter to the missing() callbacks #38289

Merged
merged 4 commits into from
Aug 9, 2021
Merged

[8.x] Add exception as parameter to the missing() callbacks #38289

merged 4 commits into from
Aug 9, 2021

Conversation

bilfeldt
Copy link
Contributor

@bilfeldt bilfeldt commented Aug 9, 2021

This PR change the logic for the missing() method handling any implicitly bound route models not found - changes are:

  1. Passing the ModelNotFoundException exception to the callback registered with the routes missing()
  2. And only return the result of the callback if this callback does actually return a non-null value.

Note that the missing() method was created in #36035 and made available in Laravel v8.26.0.

Motivation

The missing() method (documented here) offers a great way to handle any missing implicitly bound models, but there might be cases where one would like to only handle some missing models differently - and that is now possible i with this PR.

Expanding on the original example from PR #36035:

Route::get('/locations/{location:slug}/photos/{photo}', [LocationsController::class, 'show'])
    ->name('locations.view')
    ->missing(function ($request, $exception) {
        if ($exception->getModel() == 'App\Models\Location') {
            return Redirect::route('locations.index', null, 301); // Only redirect user if the location is not found
        }

        // For all other models simply render the ModelNotFoundException as we would normally do
    );

The example above will only redirect the user if the slug provided for the Location model is not found. If an invalid photo is provided a normal ModelNotFound exception would be thrown and handled normally.

A real world example could be when using teams and wanting a separate logic for dealing with an invalid team while still keeping the normal 404 for other parameters.

Alternatives

An alternative approach could be to allow a second parameter for the missing() method which specifies exactly what route parameter to apply the closure for:

Route::get('/locations/{location:slug}', [LocationsController::class, 'show'])
    ->name('locations.view')
    ->missing(fn($request) => Redirect::route('locations.index', null, 301), 'location'); // This callable will only be applied to the location parameter

I personally think this syntax is better, but unfortunately an approach like this would require the method getMissing() to return an array of callables which would be a breaking change.

Note that this PR does not stand in the way of implementing the alternative syntax above in a later version of Laravel.

Add  `ModelNotFoundException` exception to the missing callback and only return the result of the callback if this is not present.
@bilfeldt bilfeldt changed the title Feature/route missing callback Add exception as parameter to the missing() callbacks Aug 9, 2021
@taylorotwell
Copy link
Member

Will pass the exception but can't make the other breaking change on a patch release.

@taylorotwell taylorotwell merged commit 744ce4c into laravel:8.x Aug 9, 2021
@GrahamCampbell GrahamCampbell changed the title Add exception as parameter to the missing() callbacks [8.x] Add exception as parameter to the missing() callbacks Aug 9, 2021
@bilfeldt
Copy link
Contributor Author

bilfeldt commented Aug 9, 2021

Thanks for merging @taylorotwell

Do note that the proposed implementation was not a breaking change as far as I can see because it would always return the output of the callable (example fn ($request) => Redirect::route('locations.index', null, 301)), so only in this case of fn ($request) => null would the default ModelNotFound exception be thrown.

That being said, for anyone reading this in the future, with the recent merge this would now be possible:

Route::get('/locations/{location:slug}/photos/{photo}', [LocationsController::class, 'show'])
    ->name('locations.view')
    ->missing(function ($request, $exception) {
        if ($exception->getModel() == 'App\Models\Location') {
            return Redirect::route('locations.index', null, 301); // Only redirect user if the location is not found
        }

        throw $exception; // This will cause the normal "ModelNotFoundException" to be rendered
    );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants