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

When there are only two simple parameters, routing fails... #162

Closed
wants to merge 2 commits into from
Closed

When there are only two simple parameters, routing fails... #162

wants to merge 2 commits into from

Conversation

al-the-x
Copy link
Contributor

I added a test to lithium/tests/cases/net/http/RouteTest.php to reflect a route that we were attempting to use in our project that was failing to route. My guess is that the regex in Route::compile():366 is incorrectly seeing the second pattern as the regex part of the first pattern. I made that capture pattern lazy and got greens. Please review. Thanks,

David @ OrlandoPHP.org

…tain routes, i.e. "/personnel/{:personnel_id}/position/{:position_id}/action/create".
@al-the-x
Copy link
Contributor Author

BTW, I rebased to the current (as of today) master after making that pull request, which of course changed my commits. If github doesn't know what to do with it, I'll reopen my pull request.

@mehlah
Copy link
Contributor

mehlah commented Oct 17, 2011

This is related to issue #157 #154
and solve it !
I was completely on the wrong track with my solution

more tests cases here mehlah@33089d4

@mehlah
Copy link
Contributor

mehlah commented Oct 17, 2011

Is there a regex master who can explain why this happens and what's exactly going on ? :-)

@ifunk
Copy link

ifunk commented Oct 18, 2011

@mehlah
Copy link
Contributor

mehlah commented Oct 18, 2011

@ifunk: sure, I understand the role played by the ? in the regex, but how come increasing pcre backtrack_limit make it as well ?
That's the part I don't get :-)

nateabele added a commit that referenced this pull request Oct 20, 2011
@nateabele nateabele closed this Oct 20, 2011
@al-the-x
Copy link
Contributor Author

Since you essentially used the code from this pull request in your own commit verbatim, wouldn't it have been easier just to accept the pull request? Or am I missing some contributor politics here...?

@nateabele
Copy link
Member

Sorry, it was mostly laziness. The fix itself was the same, but I ended up completely rewriting the test to be exhaustive in terms of the cases it covers. Anyway, I couldn't remember the series of commands to amend someone else's work but still maintain attribution.

@al-the-x
Copy link
Contributor Author

See git commit --amend [commit-ish] for future reference... ;)

@nateabele
Copy link
Member

Heh, nice. Thanks.

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

4 participants