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

Bug: Redirect to route ignores path set in baseurl #2119

Closed
yol opened this issue Aug 4, 2019 · 14 comments
Closed

Bug: Redirect to route ignores path set in baseurl #2119

yol opened this issue Aug 4, 2019 · 14 comments
Assignees
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Milestone

Comments

@yol
Copy link

yol commented Aug 4, 2019

Describe the bug
When using redirect to route responses in a controller, paths in the configured baseURL are lost.

This is exactly like #1126, but the code has changed since the fix and the cause is now different. I.e., this is a regression.

CodeIgniter 4 version
4.0.0-beta4

Affected module(s)
router

Expected behavior, and steps to reproduce if appropriate
With app.baseURL configured as http://localhost/path/to/app/ and route configured with $route->get('myController/test', 'MyController::test'):

return redirect()->route('MyController::test') in a controller

  • should redirect to http://localhost/path/to/app/myController/test, but
  • in fact redirects to http://localhost/myController/test.

Initial analysis pertaining to the cause:

  • Reverse-resolving the route gives /myController/test (with preceding slash)
  • RedirectResponse->route tries to use base_url to wrap the path
  • base_url uses URI->resolveRelativeURI to set the path over the baseURL
  • this removes all path info from the configured base URL since the route URI begins with / and therefore is absolute, completely overriding the path

Context

  • OS: Linux
  • Web server lighttpd 1.4.54
  • PHP version 7.3.6
@yol
Copy link
Author

yol commented Aug 4, 2019

Addendum: Actually, it would have to use site_url and not base_url (such that index.php or whatever is configured is also included if needed)

@lonnieezell
Copy link
Member

That's a good catch. Care to submit a PR?

@yol
Copy link
Author

yol commented Aug 15, 2019

To be honest, I'm not sure what the concept behind the URL functions is, i.e., which ones should return paths with / and which not, and whether paths beginning with / should be taken as absolute or still relative to the URL? So I can cook up a PR but I would definitely need some guidance on how it is supposed to work.

@lonnieezell
Copy link
Member

Resolving relative URI's follows the HTTP specs, so if a '/' is at the beginning of a request it would override any additional segments already there.

In this case, I think slash should indicate it is relative to App.basePath, since that's being used to specify where the site can be found. Any that do not have a leading slash should be relative to the current page.

@jim-parry jim-parry added this to the 4.0.0-rc.2 milestone Sep 8, 2019
yol pushed a commit to yol/CodeIgniter4 that referenced this issue Sep 8, 2019
base_url does not actually add the base path if the given URL is
absolute (i.e., starts with a slash) and does not add the indexPage.
Both is necessary for the redirect response to work correctly in case
of routes. site_url handles this correctly.

Fixes codeigniter4#2119
yol pushed a commit to yol/CodeIgniter4 that referenced this issue Sep 8, 2019
base_url does not actually add the base path if the given URL is
absolute (i.e., starts with a slash) and does not add the indexPage.
Both is necessary for the redirect response to work correctly in case
of routes. site_url handles this correctly.

Fixes codeigniter4#2119
yol pushed a commit to yol/CodeIgniter4 that referenced this issue Sep 8, 2019
base_url does not actually add the base path if the given URL is
absolute (i.e., starts with a slash) and does not add the indexPage.
Both is necessary for the redirect response to work correctly in case
of routes. site_url handles this correctly.

Fixes codeigniter4#2119
yol pushed a commit to yol/CodeIgniter4 that referenced this issue Sep 8, 2019
base_url does not actually add the base path if the given URL is
absolute (i.e., starts with a slash) and does not add the indexPage.
Both is necessary for the redirect response to work correctly in case
of routes. site_url handles this correctly.

Fixes codeigniter4#2119
@jim-parry jim-parry modified the milestones: 4.0.0-rc.2, 4.0.0-rc.3 Sep 24, 2019
@jim-parry
Copy link
Contributor

The PR to fix this is stalled, so removing from rc.3

@jim-parry jim-parry removed this from the 4.0.0-rc.3 milestone Oct 15, 2019
@jim-parry jim-parry changed the title Redirect to route ignores path set in baseurl Bug: Redirect to route ignores path set in baseurl Oct 19, 2019
@jim-parry jim-parry added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 19, 2019
@jim-parry jim-parry added this to the 4.0.0 milestone Oct 29, 2019
@jim-parry jim-parry self-assigned this Oct 29, 2019
@robertotremonti
Copy link

The bug is also in $pager->getPageURI() method.

@lonnieezell
Copy link
Member

@robertotremonti Have you tested this with the latest develop branch, or just rc-3? We've had some URL fixes go into place a week or two ago.

@robertotremonti
Copy link

@lonnieezell I'm using latest 4.0.0-rc.3 release.

@robertotremonti
Copy link

@lonnieezell Anyway I've just tried a quick test by replacing system folder from latest develop branch and the bug doesn't seem to be fixed.

@walaman
Copy link

walaman commented Nov 26, 2019

Looks like current_url() have the same issue

@robertotremonti
Copy link

All function handling URLs (current_url(), previous_url(), ...) have the same problem.
I've just tried latest develop branch but the problem has not been fixed yet.

@robertotremonti
Copy link

In addition, I report that base_url('/admin/login') doesn't take into consideration $indexPage value.

For example http://localhost/admin/login is not the same as http://localhost/index.php/admin/login when MOD_REWRITE is not set.

@MGatner MGatner assigned lonnieezell and unassigned jim-parry Dec 10, 2019
@lonnieezell
Copy link
Member

In addition, I report that base_url('/admin/login') doesn't take into consideration $indexPage value.

That's correct, and by design. If you need $indexPage taken into account, use site_url().

@lavilaso
Copy link

@lonnieezell Your commit solves the problem. Thanks!

lonnieezell added a commit that referenced this issue Jan 22, 2020
Use site_url for RedirectResponse. Fixes #2119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants