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.6] fix for route url generator when request()->basePath exists #24074

Merged
merged 3 commits into from May 2, 2018

Conversation

tomschlick
Copy link
Contributor

This is a fix for a bug that was introduced with #24051 and tagged in version 5.6.19 yesterday.

Basically, if you had another script (non Laravel) in a sub-folder in the public root that loaded Laravel (by requiring the bootstrap.php file), every route url would include that sub-folder path.

This is a common thing to do in legacy code you are slowly converting to the framework, or when trying to utilize laravel resources in non laravel code (like with wordpress for instance).

In the past, the url generator stripped the whole url root but that changed in #24051 and introduced this side effect.

This fix simply removes the base path string if it exists on the request.

/cc @staudenmeir

@sisve
Copy link
Contributor

sisve commented May 2, 2018

The test has some ambiguous paths. Could we make them look more like a laravel project by using /var/www/laravel-project/public/subfolder/index.php? I want to avoid scenarios where people point at this test to say "look, laravel can run in subfolders" when that isn't a supported setup.

@tomschlick
Copy link
Contributor Author

@sisve yeah thats a good point. I'll make the edits 👍

@taylorotwell
Copy link
Member

What if the base is in the query string for some reason? Maybe holding a redirect value or something? Will this remove it?

@tomschlick
Copy link
Contributor Author

I'm not sure the base could ever be a query string... that should only be set by to where the script was initialized from by PHP.

Am I missing something there?

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

4 participants