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

Fix for #3318 #1377

Merged
merged 1 commit into from Jul 2, 2013
Merged

Fix for #3318 #1377

merged 1 commit into from Jul 2, 2013

Conversation

ravage84
Copy link
Member

Fixes https://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3318

It seems fixing this in the htaccess file(s) isn't going to work even though a url rewriting based solution was more clean. On the plus side this works for any web server.
If a url is called with "index.php" in it then the CakeRequest swallows this part and fixes the path. Any linked url from the requested page will have a clean url. Thus after following one of these urls this problem is gone anyway.

@markstory
Copy link
Member

There should probably be a test case added for this as I'm sure it will break in the future without one.

@@ -283,6 +283,9 @@ protected function _base() {
if (!$baseUrl) {
$base = dirname(env('PHP_SELF'));

if ($indexPos = strpos($base, '/webroot/index.php')) {
Copy link
Member

Choose a reason for hiding this comment

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

We generally avoid inline assignments as they are fickle and prone to error. As an example this will fail if $indexPos == 0.

@ADmad
Copy link
Member

ADmad commented Jun 26, 2013

Wouldn't "swallowing" index.php from the request uri cause problems with apps not using url rewriting?

@ravage84
Copy link
Member Author

I was wondering about that, too. But honestly I assumed something that basic was covered by the test cases and they didn't seem to break...

@markstory
Copy link
Member

It is worth manually double checking as well. Breaking most IIS & cgi based setups would be bad.

@ravage84
Copy link
Member Author

A really quick check on an IIS 7.5 using a vanilla cakephp worked for me...

@ravage84
Copy link
Member Author

With and without url rewriting on IIS by the way...

@ADmad
Copy link
Member

ADmad commented Jun 26, 2013

I would also check on apache without url rewriting with app in a subfolder of document root.

@ravage84
Copy link
Member Author

Seems to work, too (after another really quick check)

Fixes https://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3318

It seems fixing this in the htaccess file(s) isn't going to work even though a url rewriting based solution was more clean. On the plus side this works for any web server.
If a url is called with "index.php" in it then the CakeRequest swallows this part and fixes the path. Any linked url from the requested page will have a clean url. Thus after following one of these urls this problem is gone anyway.

Some code docblock improvements to CakeRequestTest.php
Added test case for fix
Also now you can call just index.php even if you have url rewriting enabled
@ravage84
Copy link
Member Author

Hope the test is ok.

Also now you can call just index.php even if you have url rewriting enabled.
Previously it reported that i couldn't find WebrootController...

@markstory
Copy link
Member

Thanks for the updates. I will give this a check tonight.

@ghost ghost assigned markstory Jun 30, 2013
markstory added a commit that referenced this pull request Jul 2, 2013
Trim off webroot/index.php when determining base and url.

Trimming off index.php from url and webroot/index.php from base url allows the correct values to be created when a path contains index.php in it.

Fixes #3318
@markstory markstory merged commit 9a08aea into cakephp:master Jul 2, 2013
@ravage84 ravage84 deleted the fix-for-3318 branch July 2, 2013 08:45
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

3 participants