Simple PHP Router

Question

Based off simple controller implementations like those seen in micro-frameworks like Slim, Silex, F3, Laravel, Tonic, Flight, Klein, etc.

/EDIT tried to link the above sources, but as a new user I only get 2 links...

Any potential issues? Improvements?

class Router { 
 private static $routes = array();
 private function __construct() {}
 private function __clone() {}
 public static function route($pattern, $callback) {
 $pattern = '/' . str_replace('/', '\/', $pattern) . '/';
 self::$routes[$pattern] = $callback;
 }
 public static function execute() {
 $url = $_SERVER['REQUEST_URI'];
 $base = str_replace('\\', '/', dirname($_SERVER['SCRIPT_NAME']));
 if(strpos($url, $base) === 0) {
 $url = substr($url, strlen($base));
 }
 foreach (self::$routes as $pattern => $callback) {
 if(preg_match($pattern, $url)){
 preg_match_all($pattern, $url, $matches);
 array_shift($matches);
 $params = array();
 foreach($matches as $match){
 if(array_key_exists(0, $match)){
 $params[] = $match[0];
 }
 }
 return call_user_func_array($callback, $params);
 }
 }
 } 
}

Takes a regexp and a callback, checks the URI for the first match... e.g.,

Router::route('blog/(\w+)/(\d+)', function($category, $id){
 print $category . ':' . $id;
});
Router::execute();
// if url was http://example.com/blog/php/312 you'd get back "php:312"...

TYIA.

Answer 1

I'm not seeing why you preg_match_all, considering you (can!) really only deal with the first match. Why not something like

if (preg_match($pattern, $url, $params)) {
 array_shift($params);
 return call_user_func_array($callback, array_values($params));
}

The str_replace to build a regex feels rickety to me; if the pattern were, say, '\/', it'd get regexified wrong and give you a PHP warning. (PCRE is pretty consistent about saying that any punctuation, metacharacter or not, can be escaped without ill effects. Blindly prefixing every / with a \ breaks that.) I'm not sure how you'd fix that, but one way to sidestep the problem might be to require that $pattern already include the delimiters rather than the router trying to tack them on itself. Besides that, it'd also serve as a clear "this is a PCRE-flavored regex" indicator, so the rules are known and people are less likely to accidentally use stuff like . without escaping it.

I'm thinking that private function __clone() {} isn't strictly necessary, and in fact hints that an instance may somehow exist. (If there's no instance possible, there's nothing to clone, and thus no need to prevent cloning.)

• momo: very good point, i'll implement your suggestion. Anything else you see? Any potential problem areas?
• cHao: @momo: Just updated with other things i considered.
• momo: great feedback. i'd upvote but need more rep. thanks again.