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

Routing wildcard :any doesn't work quite as advertised #658

Closed
hulet opened this issue Nov 10, 2011 · 15 comments
Closed

Routing wildcard :any doesn't work quite as advertised #658

hulet opened this issue Nov 10, 2011 · 15 comments
Milestone

Comments

@hulet
Copy link

hulet commented Nov 10, 2011

The User Guide page for URI Routing states:

You can match literal values or you can use two wildcard types:

(:num) will match a segment containing only numbers.
(:any) will match a segment containing any character.

This is actually not the case, however; because of the regex used :any will potentially match multiple segments.

I suggest that either the regex used to replace :any in system/core/Router.php be changed from

$key = str_replace(':any', '.+', str_replace(':num', '[0-9]+', $key));

to

$key = str_replace(':any', '[^/]+', str_replace(':num', '[0-9]+', $key));

to force :any to match only a single segment or that the User Guide be updated to read:

You can match literal values or you can use two wildcard types:

(:num) will match a segment containing only numbers.
(:any) will match any number of segments containing any characters.

@it-can
Copy link
Contributor

it-can commented Mar 12, 2012

pull request?

@narfbg
Copy link
Contributor

narfbg commented Oct 23, 2012

I'd vote for changing .+ to [^/]+.

The current behavior is really hard to debug when you don't realize that (:any) would match slashes as well and if you really need that to happen - you can always just put (.+) in there.

@alexbilbie @pkriete @derekjones

@pkriete
Copy link
Contributor

pkriete commented Oct 23, 2012

Not much to add, except I agree!

@dchill42
Copy link
Contributor

I have found it useful at times that (:any) will accept any number of segments, and wouldn't want to lose that functionality. Can we make these two different wildcards? Maybe (:any) changes as described, but we add (:all) to pick up one or more segments?

@derekjones
Copy link
Contributor

@dchill42 Though I've never used it that way, I've seen that use a number of times as well so it's a valid point, and if the behavior changes, clear callouts in the upgrade notes and change log are certainly in order.

With respect to (:any) and (:all) I'm not sure that they have clear meanings or are distinct enough to make a transition for those relying on the existing behavior any easier than @narfbg's recommendation.

@dchill42
Copy link
Contributor

@derekjones Fair enough. I'm certainly not married to that particular layout. I was just aiming to suggest a starting point with similarly short wildcards. Is there another path by which we might keep the existing functionality for those who are aware of it, and perhaps also offer @narfbg's solution as a separate option? I think both are valid and worthwhile, but agree that they need to be clearly differentiated.

@derekjones
Copy link
Contributor

Been thinking about this off and on and nothing really settles. (:all), (:full), (:complete), (:next), (:each), etc.

@narfbg
Copy link
Contributor

narfbg commented Oct 25, 2012

IMO, there's no point in having (:any) if it doesn't do anything different than (.+) and (:num) is even more pointless.

I can only imagine that back in the day, both of these replacements have been implemented in a way that didn't enable proper usage of regular expressions, but now they are both just legacy features. Having examples in the user guide that show usage of (.+) and/or (\d+) should be sufficient. If it was all up to me I'd change (:any) to do ([^/]+) and deprecate (:num), but since we don't have an actual issue with the latter - I'll agree on keeping it.

To a developer who is not familiar with regular expressions it wouldn't make any difference. For one that knows it - would most likely never use (:num) and would also most likely assume that (:any) isn't quite the same as (.+).

So why introduce another so called "wildcard" as a replacement to something that we're changing to actually be useful?

@dchill42
Copy link
Contributor

@narfbg I understand your perspective, but I don't completely agree. It's true that anyone who knows basic regular expressions can easily introduce whatever functionality they need without the wildcard replacement. As someone who has used (:any) - knowing what it actually does - and taken advantage of it in some places, I guess I'm mostly arguing for not breaking existing applications.

At the same time, I see the confusion between the user guide and the real functionality, and would like to see that resolved. It almost seems as though the easiest fix is to just update the docs to accurately reflect what (:any) has always done.

If we were to change (:any) and add one more wildcard, I suppose I'd have to go change existing apps anyway, so I'm not sure it really makes a difference to me. But, those who don't really know regex may appreciate at least having an alternative to switch their routes to if they had relied on the multi-segment capturing. For them, and for a degree of improved readability over (.+), I will offer another name suggestion for what (:any) has historically done - how about (:everything)? (I didn't think (:kitchen_sink) would actually fly.)

@narfbg
Copy link
Contributor

narfbg commented Oct 25, 2012

@dchill42 I agree on the backwards-compatibility issue, but what is written in the docs is the intentional behavior - (:any) is supposed to match a (part of a) segment, not multiple segments. If it doesn't do what it's indended to do, then we have a bug and we need it to be fixed.

People who don't have knowledge of regexp will follow the user guide, because that's their ONLY guide unless they learn how to use regular expressions. If the docs say (.+), then they'll write (.+) without questioning it and granted - most of them will forget it right after they do their configuration.
But chances are that a person unfamiliar with regular expressions will also forget (:any) or any other alias, simply because they are not used to the concept of using patterns. And for those who will learn - they will most likely learn basic regexp rules as well.

Plus, while it is indeed a framework's job to make development easier, that should not be done by replicating or aliasing native functionalities 1-to-1. Following this principle, I wouldn't care about the readability of (.+) vs. (:any), (:all), etc.

@dchill42
Copy link
Contributor

@narfbg you make a pretty convincing argument. I'm still not sure I completely agree, but I'm willing to accept more thorough and complete guidance in the user docs to go with the change to (:any) and call it good enough. Besides, I am of the opinion that every coder should know at least basic regex - pushing a bit of that in the User Guide sure won't make the world a worse place.

@derekjones
Copy link
Contributor

100% agree, improved documentation is the best solution. Education is powerful.

@narfbg
Copy link
Contributor

narfbg commented Oct 26, 2012

Before I go and commit it, how does the gist below look? Something more that we should include?

https://gist.github.com/3957904

@dchill42
Copy link
Contributor

I think that looks pretty good. Quick fix - line 115 should say "There are certainly ...".

I'm wondering if we might point users at a good regex tutorial or something. I know the selection of which one(s) can be dicey, but if I were a new user I'd sure appreciate a link or three to begin learning.

In a quick search, I found a few learning resources that seemed worthy of consideration:

...and one that's too theoretical to put in the docs, but a fascinating read:
http://gnosis.cx/publish/programming/regular_expressions.html

@narfbg
Copy link
Contributor

narfbg commented Oct 30, 2012

Okay, commited with the suggested improvements.

@narfbg narfbg closed this as completed Oct 30, 2012
GDmac pushed a commit to GDmac/CodeIgniter that referenced this issue Oct 30, 2012
@narfbg narfbg mentioned this issue Jan 6, 2013
nonchip pushed a commit to nonchip/CodeIgniter that referenced this issue Jun 29, 2013
nonchip pushed a commit to nonchip/CodeIgniter that referenced this issue Jun 29, 2013
baypup pushed a commit to baypup/CodeIgniter that referenced this issue Aug 20, 2015
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

No branches or pull requests

6 participants