BUG: Fixed bug in SS_HTTPRequest where a url ending in dots would cause match() and param() to return incorrect values

[froog]

There is a bug in HTTPRequest in how setUrl() and getUrl() work, which affects match() and param().

setUrl extracts any (probable) extension from a url that has one, except it then strangely removes the extension from the url.

getUrl resolves this by concatenating the extension back onto the split url.

This is not only unnecessary, it creates a bug in a specific case, where the url has dots after the last slash (and isn’t a file)

eg;

setUrl(‘path/action/test.param.fail’);

then:

$url = '’path/action/test.param’
$extension = ‘fail’

$dirParts = (
[0] => ‘path’,
[1] => ‘action’,
[2] => ’test.param’
)

$dirParts is used in match(), therefore:

match(‘path/$Action/$ID’);

returns:

(
[Action] => ‘action’,
[ID] => ’test.param'
)

which is clearly wrong. $request->param(‘ID’) will return ’test.param’ rather than the expected ’test.param.fail’

SOLUTION:
I’ve included a unit test for match() and param() and a fix that simply removes the code that writes the split url in setUrl(), and the concatenation code in getUrl().

$url is protected so therefore not accessed elsewhere in the codebase.

[dhensby]

Thanks for looking into this and your indepth reasoning, much of which I agree with.

This issue has been raised in various forms before (I don't have links as I'm on my phone), but the general conclusion that the inclusion of the "extension" should be a config option. Ie: people should be able to enable the behaviour you're adding but should remain as it does now for legacy reasons.

If you can amend this so that the "extension" being passed to the param and url functions was defined by a config setting, then there'd be a high chance of this being merged.

[froog]

Thanks for your reply Daniel, it took me a while to figure it all out!

I think the feature you're suggesting should be a seperate pull request though.

This one is not adding a new feature, simply removing redundant code that is also causing a bug.

It's also not removing any features or modifying the API - all legacy code will continue to work.

Cheers,
Dan

On Thu, Jan 22, 2015 at 10:40 PM, Daniel Hensby notifications@github.com
wrote:

Thanks for looking into this and your indepth reasoning, much of which I agree with.
This issue has been raised in various forms before (I don't have links as I'm on my phone), but the general conclusion that the inclusion of the "extension" should be a config option. Ie: people should be able to enable the behaviour you're adding but should remain as it does now for legacy reasons.

If you can amend this so that the "extension" being passed to the param and url functions was defined by a config setting, then there'd be a high chance of this being merged.

Reply to this email directly or view it on GitHub:
#3812 (comment)

[dhensby]

This one is not adding a new feature, simply removing redundant code that is also causing a bug.

No, it's removing a feature. The current behaviour is intended, even though I think there is general agreement that it should be removed and/or optional.

It's also not removing any features or modifying the API - all legacy code will continue to work.

Sites that require an extension to their URLs like example.com/page.html and want that to be intercepted correctly by the controllers as /page will break with this change.

That is, unless I'm mistaken about what this PR is doing...

[froog]

Ah! That does make sense, and yes, this will break existing sites that expect that behaviour. (guess I've just gotten used to friendly URLs!) :)

I'll take another look through httprequest and add that "extension" config.

Maybe a good name is "RetainExtension".

Cheers!

Dan

On Fri, Jan 23, 2015 at 12:53 AM, Daniel Hensby notifications@github.com
wrote:

This one is not adding a new feature, simply removing redundant code that is also causing a bug.
No, it's removing a feature. The current behaviour is intended, even though I think there is general agreement that it should be removed and/or optional.
It's also not removing any features or modifying the API - all legacy code will continue to work.
Sites that require an extension to their URLs like example.com/page.html and want that to be intercepted correctly by the controllers as /page will break with this change.

That is, unless I'm mistaken about what this PR is doing...

Reply to this email directly or view it on GitHub:
#3812 (comment)

[dhensby]

strip_url_extension? Default to true.

The docs and tests would need updating, too

[froog]

Sounds good, I'll amend them too.

On Fri, Jan 23, 2015 at 7:43 AM, Daniel Hensby notifications@github.com
wrote:

strip_url_extension? Default to true.

The docs and tests would need updating, too

Reply to this email directly or view it on GitHub:
#3812 (comment)

[froog]

Oh @dhensby if you could reference the other bugs/forums that would be great, thanks!

[dhensby]

@froog https://groups.google.com/d/topic/silverstripe-dev/3oORS0yuTuY/discussion that was a discussion that touched on it.

I'm sure I've seen other discussions but can't seem to find them

[dhensby]

@froog Ok, I'm going to close it as it needs to be against 3 branch if you do it with a config or against master if not (though I doubt that would be accepted).

Thanks