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

[5.5] Add Route::redirect() method #19794

Merged
merged 1 commit into from
Jun 28, 2017
Merged

[5.5] Add Route::redirect() method #19794

merged 1 commit into from
Jun 28, 2017

Conversation

brayniverse
Copy link
Contributor

This adds a new Route::redirect() method that handles simple redirects from one route to another. This implementation works with route caching because it uses an internal RedirectController.

The method accepts three arguments $url, $destination, and $status. The status is optional and defaults to 301.

Route::redirect('contact_us', 'contact', 302);

The implementation is based off my package Laravel route macros with one minor difference, the controller makes use of __invoke instead of a handle method. This is because when writing the test I couldn't find a way to attach the Route to the Request instance, and the __invoke method is handed each "parameter" - for some reason this works with Laravel's test suite but $request->route('parameter') does not 🤷‍♂️ . I can't think of any reason this __invoke approach will cause any problems.

✌️

@brayniverse brayniverse changed the title Add Route::redirect() method [5.5] Add Route::redirect() method Jun 27, 2017
@taylorotwell
Copy link
Member

Is it useful to pass along the current query string?

@brayniverse
Copy link
Contributor Author

brayniverse commented Jun 28, 2017

Yes definitely. I did a naive test for this early on but it was hardcoding the query into the destination URL. What do you think about also passing through the headers? Is there a way to create a redirect from an existing request object?

@masterwto
Copy link
Contributor

If we needed a route::redirect method to redirect an URL to another, why don't we use that route directly? I don't think this any useful.

@brayniverse
Copy link
Contributor Author

Because you may need to keep that old route available for backwards compatibility, but a new route is handling the request. I've had to do this many times with applications that have a long enough shelf life.


class RedirectController extends Controller
{
public function __invoke($destination, $status = 301)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docblocks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, thanks for pointing this out.

* @param string $url
* @param string $destination
* @param int $status
* @return Route
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return \Illuminate\Routing\Route

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll amend this after work.

*/
public function redirect($url, $destination, $status = 301)
{
return $this->any($url, '\Illuminate\Routing\RedirectController')
Copy link
Contributor

@lucasmichot lucasmichot Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ::class notation is also fine: return $this->any($url, RedirectController::class)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked a couple other cases and the core and strings seemed to be the convention. I'm happy to use class notation, Taylor can always change it if he's not happy with it.

@masterwto
Copy link
Contributor

If we need to keep that old route available for backwards compatibility, but a new route is handling the request, we simply need to define the two routes. If the old route is to be unavailable, just give out the new route.

@taylorotwell
Copy link
Member

@brayniverse what do you mean hard-coding the query into the destination URL?

@taylorotwell taylorotwell merged commit 55bee11 into laravel:master Jun 28, 2017
@brayniverse
Copy link
Contributor Author

I tried Route::redirect('hello?name=jane', 'greet') which I realised wasn't evidence that query strings are persisted across redirects. I wasn't ready for this PR to be merged because query strings don't work with this implementation. I could append the results of $request->all() if it's not empty, but it doesn't feel quite right. Is it possible to create a redirect from a request object?

@rs-sliske
Copy link

would it be possible to get this added to 5.4 aswell?

@brayniverse
Copy link
Contributor Author

@rs-sliske you can back-port this functionality using macros. However, if you don't want to bother with that I've got a [route macro package] (https://github.com/brayniverse/laravel-route-macros) which adds this functionality for Laravel 5.1 onwards.

@rs-sliske
Copy link

@brayniverse i realised not long after replying that this wasnt the right solution for my use case, if i need in future i will look into that package (or just use 5.5). thanks :)

@joaomnb
Copy link

joaomnb commented Jul 17, 2018

If there are multiple routes with the same uri but different http verbs, this applies to all of them. Any chance of adding a way of specifying the desired http verb? Or perhaps a route name?

@brayniverse
Copy link
Contributor Author

@joaomnb it'd be quite easy to allow an additional parameter to specify the verb - use self::$verbs as the default. You would need to use addRoute directly and pass the new verb(s) parameter. You'd be able to specify a single verb (as a string) or an array of verbs. Fancy giving a PR a go? I'd be happy to help if you need anything.

@joaomnb
Copy link

joaomnb commented Jul 19, 2018

I’ve never done it before, but yeah! Seems like a good chance to start 🙂 Thanks!!

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.

None yet

6 participants