-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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] Added domain helper to route #19245
Conversation
Originally sent by @robinvdvleuten to 5.4 branch, submitting to 5.5 branch
Nice! Thanks @m1guelpf! |
@m1guelpf I opened a PR on your fork (https://github.com/m1guelpf/framework/pull/1) to add a specific test for this. The test passes on your master but would fail without the new feature. |
@deleugpn Thank you! Merged |
I'd rather transitioning from 5.4 to 5.5 to be as simple as possible, but changing things like this will make it more difficult. I'm not saying I disagree with the feature, I'd just rather it not change the current behaviour to be honest. |
Maybe it would be better to keep current domain() functionality, but use the new functionality when passing a parameter, like: domain() would act as it does now, but domain($domain) would do new behaviour. |
I think it would be quite weird to have a method that return either a string or a Route object. |
I tried to stay as close to the Laravel API as possible; if you would like to add a prefix you call |
AFAIK now you can define domain only on route group. Does this PR bring opportunity to define domain for every single Route? Or I misunderstood something? |
@decadence I would infer that it does since the test I wrote for this do exactly that. |
@m1guelpf yes you should make the existing domain method function as both a getter and setter depending on if an argument is passed. We do that in other parts of the framework occasionally. |
Eh, nevermind, I guess we have other get* methods on this class. |
Excellent PR! |
Originally sent by @robinvdvleuten to 5.4 branch in #19243, submitting to 5.5 branch