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 Route::missing() #36035

Merged
merged 7 commits into from
Jan 27, 2021
Merged

[8.x] Add Route::missing() #36035

merged 7 commits into from
Jan 27, 2021

Conversation

hotmeteor
Copy link
Contributor

@hotmeteor hotmeteor commented Jan 25, 2021

This PR introduces a new ->missing() method to Route.

Here's the intention...

Let's say you have a route that's using implicit route-model binding (in this case, bound to a particular attribute):

Route::get('/locations/{location:slug}', [LocationsController::class, 'show'])
    ->name('locations.view');

Now, if that record doesn't exist, or has been removed, it'll throw a 404 if someone tries to access it. Currently to mitigate that you'd need to customize the controller method to check for it's existence and then handle it accordingly.

Instead, it would be great if there was an ->missing() method that accepted a callback and handled the situation, ie:

Route::get('/locations/{location:slug}', [LocationsController::class, 'show'])
    ->name('locations.view')
    ->missing(fn($request) => Redirect::route('locations.index', null, 301));

To do this we'd now be catching a ModelNotFoundException in the SubstituteBindings middleware, but this seems acceptable since it'll only be doing this if it detects an missing fallback.

Open to suggestions!

@hotmeteor hotmeteor changed the title First pass at else [8.x] Add Route::else() Jan 25, 2021
@reinink
Copy link
Contributor

reinink commented Jan 25, 2021

A couple ideas here Adam.

One, would missing be a better name than else?

It might be smart to also accept a callback here, instead of providing the response straight up. That would allow you to lazily create the response, and potentially even get access to the request.

Something like this:

Route::get('/locations/{location:slug}', [LocationsController::class, 'show'])
    ->name('locations.view')
    ->missing(fn () => Redirect::route('locations.index', null, 301));

@GrahamCampbell
Copy link
Member

Missing, fallback, default?

@hotmeteor
Copy link
Contributor Author

@reinink @GrahamCampbell

Thanks. Updated method to be called missing(), and it now accepts a callback.

@hotmeteor hotmeteor changed the title [8.x] Add Route::else() [8.x] Add Route::missing() Jan 25, 2021
@taylorotwell
Copy link
Member

@hotmeteor did you test this with Route caching? Is your "missing" attribute persisted in that case? If so, how so when it is using a Closure?

@hotmeteor
Copy link
Contributor Author

@taylorotwell

Thanks for pointing that out, I hadn't. I've updated this to handle caching, and tested it.

As part of this modified the SubstituteBindings middleware instead of the Router, which I felt made it a clearer entrypoint for handling this but possibly less backwards compatible (?) Happy to take suggestions here.

@browner12
Copy link
Contributor

This reminds me of my "Internal Redirects" PR from a year ago.

#30806

I still think it would be great if we could handle this through internal routing, rather than making a separate HTTP request.

@hotmeteor
Copy link
Contributor Author

hotmeteor commented Jan 25, 2021

@browner12 My intent with this was that there specifically would be a separate HTTP request, because I want to report to Google that a page is no longer available.

But if that could all be handled internally, then awesome.

@browner12
Copy link
Contributor

My original intent with the PR is that you could return a 404 status with a customized view that is still run through a controller.

If you're sending a 404 a search engine should pick that up as a dead page, doesn't need to be a 3xx.

@GrahamCampbell
Copy link
Member

You could just serve a 404 page with js that redirects the user. Then you get the correct status code and good UX. ;)

@browner12
Copy link
Contributor

Just so I'm understanding correctly, you are suggesting that as a way to solve OP's need with my internal routing solution, correct?

@taylorotwell taylorotwell merged commit 8bbd689 into laravel:8.x Jan 27, 2021
@hotmeteor hotmeteor deleted the route-else branch January 27, 2021 18:33
@adampatterson
Copy link
Contributor

It would be nice if this could work with a route group.

I have a route group prefixed with a username and then a number of routes under that like username/blog. I was hoping that this would help clean up some validation.

@lucagrandicelli
Copy link

How about handling this method on resource controllers (rc)?
It seems that when routing with rc /w route-model binding, if the model is not found (i.e bad ID in the URL), the Exception Handler class takes over any possible try/catch in the code, so basically there ain't no way to return specific response on a 'ModelNotFoundException' or 'NotFoundHttpException' in the controller code. Especially when dealing with API.

@hotmeteor
Copy link
Contributor Author

@lucagrandicelli Look in ResourceRegistrar, you'll need to catch an exception in there when trying to resolve the resource method.

@smarano
Copy link

smarano commented Jun 27, 2021

Maybe is too late, but what about if you have a route with more than one implict-binded model ?`
ex: '/locations/{location:slug}/{sublocation:slug}'
and you want to show different pages if one of the models are missing?

@sakihl
Copy link

sakihl commented Jul 3, 2022

Coming late to this I have just started using missing() and finding it very useful and would like to echo the comment by @smarano that it would be even nicer if you could know which model was missing where there is more than one in the route.

@mehdismekouar
Copy link

mehdismekouar commented Jun 27, 2024

Would be great if one could directly return a view like for example...

Route::get('/tags/{tag}', [TagController::class, 'show'])->missing(function (Request $request) {
    return view('error');
});

In the above case it throws an error

Call to a member function setCookie() on null

@mehdismekouar
Copy link

Nevermind, i found the solution...

Route::get('/tags/{tag}', [TagController::class, 'show'])->missing(function (Request $request) {
    return response()->view('error');
});

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.

10 participants