Simplify implementation for optionals

Drop the manual parsing code, use regex.
nikic · Jun 18, 2015 · 31fa869 · 31fa869
1 parent 2acf229
commit 31fa869
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 51 deletions.
diff --git a/src/RouteParser/Std.php b/src/RouteParser/Std.php
@@ -12,77 +12,45 @@
  */
 class Std implements RouteParser {
     const VARIABLE_REGEX = <<<'REGEX'
-~\{
+\{
     \s* ([a-zA-Z][a-zA-Z0-9_]*) \s*
     (?:
         : \s* ([^{}]*(?:\{(?-1)\}[^{}]*)*)
     )?
-\}~x
+\}
 REGEX;
     const DEFAULT_DISPATCH_REGEX = '[^/]+';
 
     public function parse($route) {
         $routeWithoutClosingOptionals = rtrim($route, ']');
         $numOptionals = strlen($route) - strlen($routeWithoutClosingOptionals);
-        $routeParts = $this->parsePlaceholders($routeWithoutClosingOptionals);
-        if ($numOptionals === 0) {
-            return [$routeParts];
+
+        // Split on [ while skipping placeholders
+        $segments = preg_split('~' . self::VARIABLE_REGEX . '(*SKIP)(*F) | \[~x', $routeWithoutClosingOptionals);
+        if ($numOptionals !== count($segments) - 1) {
+            throw new BadRouteException("Number of opening '[' and closing ']' does not match");
         }
-        return $this->handleOptionals($routeParts, $numOptionals);
-    }
 
-    private function handleOptionals($routeParts, $numOptionals) {
+        $currentRoute = '';
         $routeDatas = [];
-        $currentRouteData = [];
-        foreach ($routeParts as $part) {
-            // skip placeholders
-            if (!is_string($part)) {
-                $currentRouteData[] = $part;
-                continue;
+        foreach ($segments as $segment) {
+            if ($segment === '') {
+                throw new BadRouteException("Empty optional part");
             }
 
-            $segments = explode('[', $part);
-            $currentNumOptionals = count($segments) - 1;
-            $numOptionals -= $currentNumOptionals;
-            if ($numOptionals < 0) {
-                throw new BadRouteException("Found more opening '[' than closing ']'");
-            }
-
-            $currentPart = '';
-            foreach ($segments as $i => $addPart) {
-                if ($addPart === '') {
-                    if ($currentPart !== '') {
-                        throw new BadRouteException("Empty optional part");
-                    }
-                    $routeDatas[] = $currentRouteData;
-                    continue;
-                }
-
-                $currentPart .= $addPart;
-                if ($i !== $currentNumOptionals) {
-                    $routeData = $currentRouteData;
-                    $routeData[] = $currentPart;
-                    $routeDatas[] = $routeData;
-                } else {
-                    $currentRouteData[] = $currentPart;
-                }
-            }
-        }
-
-        $routeDatas[] = $currentRouteData;
-        if ($numOptionals > 0) {
-            throw new BadRouteException("Found more closing ']' than opening '['");
+            $currentRoute .= $segment;
+            $routeDatas[] = $this->parsePlaceholders($currentRoute);
         }
-
         return $routeDatas;
     }
 
     /**
-     * Parses a route string only considering {placeholders}, but ignoring [optionals].
+     * Parses a route string that does not contain optional segments.
      */
     private function parsePlaceholders($route) {
         if (!preg_match_all(
-            self::VARIABLE_REGEX, $route, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER
+            '~' . self::VARIABLE_REGEX . '~x', $route, $matches,
+            PREG_OFFSET_CAPTURE | PREG_SET_ORDER
         )) {
             return [$route];
         }

diff --git a/test/RouteParser/StdTest.php b/test/RouteParser/StdTest.php
@@ -83,19 +83,23 @@ public function provideTestParse() {
                     ['/test/', ['name', '[^/]+']],
                     ['/test/', ['name', '[^/]+'], '/', ['id', '[0-9]+']],
                 ]
-            ]
+            ],
         ];
     }
 
     public function provideTestParseError() {
         return [
+            [
+                '/test[opt',
+                "Number of opening '[' and closing ']' does not match"
+            ],
             [
                 '/test[opt[opt2]',
-                "Found more opening '[' than closing ']'"
+                "Number of opening '[' and closing ']' does not match"
             ],
             [
                 '/testopt]',
-                "Found more closing ']' than opening '['"
+                "Number of opening '[' and closing ']' does not match"
             ],
             [
                 '/test[]',