-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
Fix double base path in url #360
Conversation
could you add a test for this? |
I have added a test that would have picked up on this issue. |
src/Response.php
Outdated
@@ -99,7 +99,7 @@ public function toResponse($request) | |||
$page = [ | |||
'component' => $this->component, | |||
'props' => $props, | |||
'url' => $request->getBaseUrl().$request->getRequestUri(), | |||
'url' => $request->getBaseUrl().$request->getPathInfo(), |
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.
getRequestUri()
contains the query string whereas getPathInfo()
doesn't. You might be able to replace this line with
'url' => $request->fullUrl(),
fullUrl()
appears to do what you want
/**
* Get the full URL for the request.
*
* @return string
*/
public function fullUrl()
{
$query = $this->getQueryString();
$question = $this->getBaseUrl().$this->getPathInfo() === '/' ? '/?' : '?';
return $query ? $this->url().$question.$query : $this->url();
}
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.
fullUrl seems to work on my initial tests, but also adds scheme/protocol and host to the url. This would break some of the other tests. The current change in this pull request ('url' => $request->getBaseUrl().$request->getPathInfo().$queryString,
) would technically be a smaller change, but fullUrl might be better, but unsure if there are other consequences except for broken tests.
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 changed to 'url' => $request->getRequestUri(),
but I will test it sooner.
Would like to add this here as well: #359 (comment) |
Could this be added in the next release? It's causing headaches in our testing environment. |
Any solution for this? I just facing the same issue today when trying to get back with Laravel. |
Ridiculous nearly two years later and this is still an issue. As if I needed any more reasons to not use Inertia in my next app. |
Please guys fix this errors. It's still not working and i have double wrong urls. |
I use this forked version to solve this issue: https://github.com/feature-ninja/inertia-laravel |
Thanks for the PR, and I'm sorry for the delay! I ended up going with a slightly different approach (#592), but this PR was very helpful, and I borrowed your test. |
This is a fix for the error described in issue #359