Fix: remove all leading dashes from URLSegments #2905

Merged
merged 1 commit into from Mar 2, 2014

Projects

None yet

4 participants

@feejin

Changed behaviour to remove all leading dashes and not just one. Removed redundant check for underscore as this is already handled with underscores and dots to dashes.

@tractorcow

@feejin Thanks for the PR.

Why would the "remove duplicate dashes" part not remove multiple leading dashes? Have you run your test case without the changed URLSegmentFilter code to prove the existing code doesn't work?

@feejin

My bad, it works fine without this due to the remove duplicate dashes line. Sorry!

@Zauberfisch Zauberfisch commented on an outdated diff Feb 28, 2014
model/URLSegmentFilter.php
@@ -33,7 +33,7 @@ class URLSegmentFilter extends Object {
'/[_.]+/u' => '-', // underscores and dots to dashes
'/[^A-Za-z0-9\-]+/u' => '', // remove non-ASCII chars, only allow alphanumeric and dashes
'/[\-]{2,}/u' => '-', // remove duplicate dashes
- '/^[\-_]/u' => '', // Remove all leading dashes or underscores
+ '/^[\-]+/u' => '' // Remove all leading dashes or underscores
@Zauberfisch
Zauberfisch Feb 28, 2014

as this no longer concerns underscores, the comment should be changed too.

@feejin

I'm going to close this as it's a non-fix anyway.

@feejin feejin closed this Feb 28, 2014
@Zauberfisch

Despite the odds of making myself look like an idiot now, may I ask why?
(Its a minor patch, however I still see a point in merging it.)

@feejin

The previous line replaces any instance of "--" with "-" so there's never more than one leading dash, meaning my additional + in the regex is redundant.

I'm pretty sure there's no need to look for underscores with this replace but should that be an amend or a new pull request?

@Zauberfisch

since this pull request already exists, I would have voted for just merging it. but I guess appending it to another related commit is fine to.

@simonwelsh

This is still a good idea. If you change the regex so it's once again removing leading underscores (as the current one does), then I'm for merging it.

@Zauberfisch

isn't the _ already replaced with a - a few lines above?

@simonwelsh
@Zauberfisch

so yeah, I would also still like to see this merged.
Its really minor, but it still is something, and now that we have already addressed the issue, why not merge it.
I'd say fix the comment and get it merged.

@feejin feejin reopened this Feb 28, 2014
@feejin

I did the comment but I've ballsed something up because I've included someone elses commit. I'm out of my git-depth here, any advice on how to rectify?

Edit: think I've sorted it. Sorry!

@tractorcow

Better documentation and test cases are always welcome @feejin

@tractorcow tractorcow merged commit f543041 into silverstripe:3.1 Mar 2, 2014

1 check passed

Details default Scrutinizer: No new issues — Travis: Passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment