Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

placeholder restrictions not matched #183

Closed
bduggan opened this Issue Jul 22, 2011 · 10 comments

Comments

Projects
None yet
2 participants
Contributor

bduggan commented Jul 22, 2011

Hello,

I've noticed a little bug in the handling of restrictions to placeholders in Mojolicious::LIte routes :

When a list of possible values for a placeholder contains two strings, one of which is a prefix of another, and the shorter one is listed before the longer one, the longer one is not treated as a legal value for the placeholder. This seems to happen even if a regex is used instead of a list of values.

Here is a test :
https://gist.github.com/1099355
This fails for perl version 5.10.1.

Owner

kraih commented Jul 22, 2011

Both are equal and get compiled to the exact same regex, don't think this can be considered a bug, it's just how regular expressions work.

@kraih kraih closed this Jul 22, 2011

Contributor

bduggan commented Jul 22, 2011

Sorry if I'm missing something obvious .. the regex for the entire route (as reported by "app.pl routes") is
(?-xism:^/one/(bar|barfly), and this does match both "/one/bar" and "/one/barfly". Yet app.pl get /one/bar succeeds whereas app.pl get /one/barfly fails.

Owner

kraih commented Jul 22, 2011

Take a closer look at the regex, it has no end, and that means "barfly" can't ever match.
It will always end up with "/one/bar" and leave "fly" in the buffer, which then prevents a real match.

Owner

kraih commented Jul 22, 2011

To make it short, the most specific word has to go first.

Contributor

bduggan commented Jul 22, 2011

Okay, I see, so I would need to do something like qr[bar(?!fly)|barfly] instead.

Owner

kraih commented Jul 22, 2011

Not at all, [qw/barfly bar/] would work too.

Contributor

bduggan commented Jul 22, 2011

Okay. It would be nice if Mojolicious::Routes::Pattern automatically did a "reverse sort" of values when making the regex, but I suppose I can just do this in my app instead.

Owner

kraih commented Jul 22, 2011

Yes, that would make sense for word lists.

Owner

kraih commented Jul 24, 2011

And implemented. :)

604b71f

Contributor

bduggan commented Jul 25, 2011

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment