-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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::view() helper #19835
Conversation
/** | ||
* Create a new view controller instance. | ||
* | ||
* @param \Illuminate\Contracts\View\Factory $view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing @return void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
It would be more useful if we could also pass variables: |
tests/Routing/RoutingRouteTest.php
Outdated
|
||
$router->view('contact', 'pages.contact'); | ||
|
||
$this->assertEquals('Contact us', $router->dispatch(Request::create('contact', 'GET'))->getContent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess assertEquals
is also fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it compares the result AND the return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear whether you like or dislike my use of assertEquals
here. Are you suggesting I should do something different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. But I'm always happy to hear if I could have done something better so please do elaborate later if you change your mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess lucasmichot meant assertSame
;-)
What variables would you have in a route file? I think a view composer would be better if you need to do something like that. |
Route::view('contact/germany', 'pages.contact', ['office' => 'Berlin'])->name('contact.germany');
Route::view('contact/england', 'pages.contact', ['office' => 'London'])->name('contact.england'); The goal of this PR is too make |
Interesting point, I hadn't considered that. If this works with route caching I don't see a problem with adding it. It would be a fairly simple change too. @taylorotwell do you have any strong opinions on this? |
Fine with me. |
Welp there goes my |
@brayniverse are you going to implement the data handling? |
Yes I will do. I've been driving all evening and the cars packed in. So I'll do it as soon as I get a chance but it probably won't be tonight. |
@joshmanders That's the kind of helper probably a lot of people had added in their projects ;-) |
All should be good now. Data can be passed to the view and I tested it still works with route caching. |
This adds a new
Route::view()
method. The implementation works with route caching as it uses an internalViewController
class.The first argument is the route and the second is the view name.