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

[6.x] DRY up path to /home #5173

Merged
merged 1 commit into from Dec 10, 2019
Merged

[6.x] DRY up path to /home #5173

merged 1 commit into from Dec 10, 2019

Conversation

taylorotwell
Copy link
Member

We were defining this path in six places (!).

@driesvints driesvints changed the title DRY up path to /home [6.x] DRY up path to /home Dec 10, 2019
@Lloople
Copy link

Lloople commented Dec 10, 2019

Maybe a method would be better, like the redirectTo() in authentication controllers, in case someone needs to build this value with some logic

@tontonsb
Copy link

I agree to @Lloople that RouteServiceProvider::home() would be more versatile. But I'll gladly take this PR over the current state!

@franksierra
Copy link

Would it be ok if the RedirectsUser trait have access to the RouteServiceProvider? Instead of the config file I was proposing when modifying the framework. This way it could retain the actual functionality of the function and property redirectTo using the HOME constant from the ServiceProvider but letting you define the function redirectTo as you like

@sertxudev
Copy link

Maybe a method would be better, like the redirectTo() in authentication controllers, in case someone needs to build this value with some logic

This solution uses the Route Service Provider, if someone needs to add some logic to the home route, like me, we could use a custom service provider or helper to return the proper string in the controllers we need it.

@Lloople
Copy link

Lloople commented Dec 10, 2019

@sertxudeveloper You mean replacing all code occurrences of RouteServiceProvider::HOME by RouteServiceProvider::getHomeRoute() or something like that? If that's the case and we can already do this replacement for /home, then why we need a PR like this?

Copy link
Contributor

@ninjaparade ninjaparade left a comment

Choose a reason for hiding this comment

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

So much yes!

@sertxudev
Copy link

sertxudev commented Dec 10, 2019

@Lloople Yes, I would have no problem in modifying it with a custom method in the places where I need it.

@taylorotwell
Copy link
Member Author

@Lloople you can still override the redirectTo methods as needed.

@taylorotwell taylorotwell merged commit 972f3cd into master Dec 10, 2019
@taylorotwell taylorotwell deleted the home-route branch December 10, 2019 14:59
@dionysiosarvanitis
Copy link

Still prefer a named route for this.

realodix added a commit to realodix/urlhub that referenced this pull request Dec 11, 2019
@marchershey
Copy link

How does this benefit?

@Lloople Yes, I would have no problem in modifying it with a custom method in the places where I need it.

Yeah, but what's easier... changing the $redirectTo or changing the $redirectTo & service provider?

--

Is this change documented anywhere so I can read up on the benefits? Thanks

@martindilling
Copy link

@taylorotwell

@Lloople you can still override the redirectTo methods as needed.

I would very much prefer the redirectTo method being the only way to define this. Can't see a reason for it to be a property with check for the existence of the method.
If it's just a method, it's allows for using both a plain url, route name or anything you want out of the box. But as it is now, if you "just" want to use a route name, you can't just change the value in 7 places, you have to remove a property, and add a new method in 7 places ;)

socheatsok78 pushed a commit to pp-spaces/laravel that referenced this pull request Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants