Skip to content
This repository

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

Closed
wants to merge 2 commits into from

4 participants

David Rogers Mehdi Lahmam Adam Royle Nate Abele
David Rogers

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

al-the-x added some commits
David Rogers al-the-x Making the capture pattern lazy for the optional regex part fixes cer…
…tain routes, i.e. "/personnel/{:personnel_id}/position/{:position_id}/action/create".
9cc2133
David Rogers al-the-x This is an example from production that broke with the previous behav…
…ior, provided for clarity.
24f0d01
David Rogers

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.

Mehdi Lahmam

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

Mehdi Lahmam

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

Mehdi Lahmam

@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 :-)

David Rogers

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...?

Nate Abele
Owner

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.

David Rogers

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

Nate Abele
Owner

Heh, nice. Thanks.

Sloan Mergler smergler referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 2 authors.

Oct 17, 2011
David Rogers al-the-x Making the capture pattern lazy for the optional regex part fixes cer…
…tain routes, i.e. "/personnel/{:personnel_id}/position/{:position_id}/action/create".
9cc2133
David Rogers al-the-x This is an example from production that broke with the previous behav…
…ior, provided for clarity.
24f0d01
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 17 additions and 2 deletions. Show diff stats Hide diff stats

  1. +1 1  net/http/Route.php
  2. +16 1 tests/cases/net/http/RouteTest.php
2  net/http/Route.php
@@ -396,7 +396,7 @@ public function compile() {
396 396 return;
397 397 }
398 398 $this->_pattern = "@^{$this->_template}\$@";
399   - $match = '@([/.])?\{:([^:}]+):?((?:[^{]+(?:\{[0-9,]+\})?)*)\}@S';
  399 + $match = '@([/.])?\{:([^:}]+):?((?:[^{]+(?:\{[0-9,]+\})?)*?)\}@S';
400 400 preg_match_all($match, $this->_pattern, $m);
401 401
402 402 if (!$tokens = $m[0]) {
17 tests/cases/net/http/RouteTest.php
@@ -559,6 +559,21 @@ public function testContinuationRoute() {
559 559
560 560 $result = $route->match(array('admin' => true, 'args' => ''));
561 561 $this->assertEqual('/admin/{:args}', $result);
  562 + }
  563 +
  564 + public function testTwoParameterRoutes ( ) {
  565 + $route = new Route(array(
  566 + 'template' => '/personnel/{:personnel_id}/position/{:position_id}/actions/create',
  567 + 'params' => array('controller' => 'actions', 'action' => 'create'),
  568 + ));
  569 +
  570 + $route->compile();
  571 +
  572 + $data = $route->export(); $actual = $data['pattern'];
  573 +
  574 + $expected = '@^/personnel(?:/(?P<personnel_id>[^\\/]+))/position(?:/(?P<position_id>[^\\/]+))/actions/create$@';
  575 +
  576 + $this->assertEqual($expected, $actual);
562 577 }
563 578
564 579 public function testContinuationRouteWithParameters() {
@@ -592,4 +607,4 @@ public function testContinuationRouteWithQueryString() {
592 607 }
593 608 }
594 609
595   -?>
  610 +?>

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.