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

WIP Use site_url for route-based RedirectResponse #2196

Closed
wants to merge 4 commits into from

Conversation

yol
Copy link

@yol yol commented Sep 8, 2019

Description
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 #2119

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

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
$this->config->baseURL = 'http://example.com';
$this->config = new App();
$this->config->baseURL = 'http://example.com';
$this->config->indexPage = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting indexPage to empty breaks the form & HTML helper tests big-time!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does changing the local App instance here influence other tests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And how to resolve this? I need a different indexPage for the test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change it in setUp(), set the specific values you need only on the test where they differ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh I get it now, you were trying to "undo" the injection of your test. Let me think of another way to approach this...

@jim-parry jim-parry changed the title Use site_url for route-based RedirectResponse WIP Use site_url for route-based RedirectResponse Sep 17, 2019
@MGatner
Copy link
Member

MGatner commented Dec 6, 2019

@lonnieezell I believe this was addressed by your URL function rework. Can you confirm?

@lonnieezell
Copy link
Member

@MGatner This was not fixed and i think the change makes sense here, as base_url would not include the settings of $indexPage, which it definitely should.

@MGatner
Copy link
Member

MGatner commented Dec 27, 2019

Great. I think this is good then but it breaks the default redirect response test because by default the index page is included (which is not reflected in the assertion). Easy fix if @yol is around.

@yol
Copy link
Author

yol commented Jan 7, 2020

Hey, sorry I kind of lost track. So the PR is still good? But the "default" redirect response is broken? Can you clarify what you mean by default?

@MGatner
Copy link
Member

MGatner commented Jan 10, 2020

Basically, in tests/system/HTTP/RedirectResponseTest.php around line 61, you should change the expected response from "http://example.com/exampleRoute" to "http://example.com/index.php/exampleRoute"

Ref. https://travis-ci.org/codeigniter4/CodeIgniter4/jobs/623289981#L586

@lonnieezell
Copy link
Member

Fixed in #2496

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.

Bug: Redirect to route ignores path set in baseurl
5 participants