Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Processed routes #1636

Merged
merged 21 commits into from

10 participants

@jdfm

This patch allows back references of routes with a "php:" prefix to be processed with php, original style routes are unaffected by these changes. You can use original style routes and "php:" prefixed routes in the same route config file.

@jdfm jdfm allow for routes that can be processed with php, ex: $route['([^/]+)/…
…([^/]+)(/:any)?'] = 'php:"$1" . "/do" . ucfirst("$2") . "$3"';
a101c93
@jdfm

This is my first time contributing to any major project so if I'm doing anything wrong please let me know.

@philsturgeon

Could you give an example of what this does? And document it in the user_guide_src folder.

@jdfm

Instead of having to write all of these routes out like so:

$route['admin/user/list'] = 'admin/doUserList';
$route['admin/project/list'] = 'admin/doProjectList';

I could just write a route like so:

$route['admin/([^/]+)/list'] = 'php:"admin/do" . ucfirst(strtolower("$1")) . "List"';

Such a route would allow me to create other methods of the same type (ex: doSomethingList) without having to create a new route to describe it.

Also, this is a non-breaking change, so, it still allows for both types of routes to be used interchangeably in the same file. Example:

$route['default_controller'] = 'home';
$route['404_override'] = 'errors/page_missing';
$route['admin/user'] = 'admin/doUserIndex';
$route['admin/project'] = 'admin/doProjectIndex';
$route['admin/([^/]+)/([^/]+)(/:any)?'] = 'php:"admin/do" . ucfirst("$1") . ucfirst("$2") . "$3"';
$route['([^/]+)/([^/]+)(/:any)?'] = 'php:"$1" . "/do" . ucfirst("$2") . "$3"';
$route['([^/]+)/?'] = '$1/doIndex';

I used the prepend "php:" at the time because it made sense, it can be something else if people don't like it. You would only need to change the route_prepend property's value in the Router class to something else and it should work fine.

@philsturgeon
@jdfm

You mean something like: php:myUberCoolFunction which would receive all of the back-references as it's arguments?

@philsturgeon

No I mean:

$routes['admin/([^/]+)/([^/]+)(/:any)?'] = function($foo, $bar, $baz) {
   return 'admin/do' . ucfirst($foo) . ucfirst($bar) . $baz;
};
@jdfm

I hadn't thought of that possibility, but I was under the impression that anonymous functions were only available from PHP 5.3 onward, and the system requirements for CI require a minimum of PHP 5.1.6.

@philsturgeon

Minimum requirements of 3.0 are 5.2.4, and you said it yourself: 'minimum requirements'.

If something is OPTIONAL and uses PHP 5.3 then great. If a fallback can be made then use that too.

Technically though, PHP 4.0.1 users could use callbacks: http://uk3.php.net/create_function

@jdfm

The new code allows for callbacks to be used as you specified above, it also still allows for the "php:" prepend to be used as well. Again, these are all non-breaking changes, so, old style routes are working as well.

@philsturgeon
@joelcox

Callbacks look great, but I agree with Phil on getting rid of the php: type routes. Please make sure you take a good look at the guidelines for contributing. Otherwise good job.

@jdfm

I have a line in the patch that uses function()use(){}, is there a similar way to do this with create_function (I haven't been able to figure this out yet)? Also, it seems that create_function has a few memory related problems, should it really be used, or should this feature be limited to people with PHP 5 >= 5.3.0? There are ways to limit the problem, but from what I have been reading, it seems that global variables must be created, and that is a bit of a nono for me...

@jdfm

Also, the php: type route was just an initial idea that was easy to implement and widely supported. It basically only adds the "e" modifier to preg_replace and executes the route as php.

@philsturgeon
@jdfm
<?php

$var = 10;
if(0){
    try {
        $a = function($something)use($var){
            echo $something . $var;
        };  
    } catch(Exception $e) {
        echo $e->getMessage();
    }
} else {
    $a = create_function('$something', 'return $something . " a";');
}

echo $a(10);

I've been trying to run this code in PHP5.2.17 but it's not working at all... It does not throw any errors either. It just fails. Which to me means that this cannot be in any code that is intended to run in PHP < 5.3... I could just make a MY_Router class with this code included, anyone who is using PHP >= 5.3 can use it, and when PHP5.3 is the minimum requirement for CI the code can be included then. Is this acceptable?

@philsturgeon
@jdfm

I wasn't talking about create_function in my last post, I was just posting a piece of code that wasn't working because a syntax error is thrown if I try to use function()use() in PHP5.2. So, to make this an optional feature for 5.3 users, I guess I'll just separate this into a MY_Router class then.

@jdfm

Should I close the pull request now? Or should it be left open for future reference?

@philsturgeon

You don't need to put it into MY_Router, you'd just use is_callable() to see if you can call it.

A little something like this:

php > $route = function($foo, $bar) { return $foo.'_'.strtoupper($bar); };
php > var_dump(! is_string($route) and is_callable($route));
bool(true)

That will give you true if its not a string with a function name, like next() - which would break existing valid string based routes.

Next you use that boolean value to see if you should call it:

php > $routed_uri  = call_user_func_array($route, array('match1', 'match2'));
php > var_dump($routed_uri);
string(13) "match1_MATCH2"

Obviously those matches are hardcoded here. They would normally come from the preg_match() and be passed through.

The next thing to worry about is, how do you manage callbacks that require two params, but only get on - or none?

One idea would be to use reflection:

php > $reflection = new ReflectionFunction($route);
php > $params = $reflection->getParameters();
php > var_dump(count($params));
int(2)

This way you can make sure that the right number of methods have matched. If they do not match the correct number, you can return the 404_override, or just error.

NOW, that wont work if those parameters have optional values, as it will REQUIRE that you set two, when really setting one might work just fine if a route wants a default. So taking it one more step further, you could foreach through the params:

if (count($matches) < count($params))
{
    try
    {
        foreach ($params as $param)
        {
            $param->getDefaultValue();
        }
    }
    catch (ReflectionException $e)
    {
        if ($e->getMessage() === 'Parameter is not optional')
        {
            return $route['404_override'];
        }
    }
}

That'll do it. If they are missing any parameters this code will check to see if thats ok or not, then if not it will return the 404_override.

If somebody wants to put this together I've just given you pretty much everything you need.

@jdfm

I was having trouble trying to understand what you meant by "optional 5.3 feature for optional 5.3 users", I thought you meant something like conditional compilation or such, which isn't possible with PHP as far as I know, which is why I separated the code to a MY_Router class. After this last explanation though it was clear what I needed to change to get the code to work even under PHP 5.2. Should work fine now.

system/core/Router.php
((45 lines not shown))
+ }
+
+ // Loop through the route array looking for wild-cards
+ foreach ($this->routes as $key => $val)
+ {
+ // Convert wild-cards to RegEx
+ $key = str_replace(':any', '.+', str_replace(':num', '[0-9]+', $key));
+
+ // Does the RegEx match?
+ if (preg_match('#^'.$key.'$#', $uri, $matches))
+ {
+ // Are we using a callback?
+ $callable = ! is_string($val) && is_callable($val);
+
+ // Are we using callbacks to process back-references?
+ if($callable){

This looking good, but can you follow the style guide? Brackets on the next line, variables as snake_case, etc.

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

The example that I added in the documentation isn't exactly the best of examples, but it should get the idea across.

@jdfm

Is there a way to use helper functions in the router.php config file?

@philsturgeon
@jdfm

Hey, what's the coding style rule for anonymous functions? The code that I wrote for the documentation may not be within the coding styles of codeigniter.

@timw4mail

@jdfm There isn't any...yet.

@jdfm

What's that you say? Post some suggestions up here? Well, ok, since you asked so nicely:


<?php

$outervar = 100;
$afn = function($param) use($outervar) {
    return $outervar;
};

<?php

$outervar = 100;
$afn = 
    function($param) 
         use($outervar)
    {
        return $outervar;
    };

<?php

$outervar = 100;
$afn = function($param) use($outervar)
{
    return $outervar;
};

<?php

$outervar = 100;
$afn = 
    function($param) use($outervar)
    {
        return $outervar;
    };

<?php

$outervar = 100;
$afn = function($param) use($outervar)
       {
           return $outervar;
       };
@Dentxinho

I'd use the last one

@ericbarnes

For me this seems to match existing standards best:


<?php

$outervar = 100;
$afn = function($param) use($outervar)
{
    return $outervar;
};
@philsturgeon
@jdfm

<?php

$my_uber_long_variable_name = function ($someparam, $anotherparam, $onemoreparam = 'default') use ($first_outer, $second_outer, $third_outer)
{
    return 'something';
};
@philsturgeon

@jdfm My only advice there is dont use uber long variable names... You could write any reasonably line of code and make it look bad with long variable names.

@cryode

+1 to Eric's mentioned solution.

@philsturgeon
@jdfm

Is there anything left to do?

@philsturgeon

It looks fine over here, but you'll need a +1 from @alexbilbie @narfbg @ericbarnes @pkriete @derekjones

I'll sign out of this thread now. Best of luck!

system/core/Router.php
@@ -368,13 +368,39 @@ protected function _parse_routes()
foreach ($this->routes as $key => $val)
{
// Convert wild-cards to RegEx
- $key = str_replace(array(':any', ':num'), array('.+', '[0-9]+'), $key);
+ $key = str_replace(':any', '.+', str_replace(':num', '[0-9]+', $key));
@narfbg Owner
narfbg added a note

Why is this changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
system/core/Router.php
((10 lines not shown))
{
- // Do we have a back-reference?
- if (strpos($val, '$') !== FALSE && strpos($key, '(') !== FALSE)
+ // Are we using a callback?
+ $callable = ! is_string($val) && is_callable($val);
+
+ // Are we using callbacks to process back-references?
+ if($callable)
@narfbg Owner
narfbg added a note

A space needs to be put between keywords and parenthesis (and this has already been discussed in the thread) - same goes for line 393.
Also, you're not using $callable anywhere else so it might as well be just:

if ( ! is_string($val) && is_callable($val))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
system/core/Router.php
((25 lines not shown))
+ // Determine how many parameters the callback has.
+ $reflection = new ReflectionFunction($val);
+ $param_count = $reflection->getNumberOfParameters();
+
+ // Are there more parameters than matches?
+ if($param_count > $match_count)
+ {
+ // Any params without matches will be set to an empty string.
+ $matches = array_merge($matches, array_fill($match_count, $param_count - $match_count, ''));
+ }
+
+ // Execute the callback using the values in matches as its parameters.
+ $val = call_user_func_array($val, $matches);
+ }
+ // Are we using the default routing method for back-references?
+ else if (strpos($val, '$') !== FALSE AND strpos($key, '(') !== FALSE)
@narfbg Owner
narfbg added a note

else if -> elseif
AND -> &&

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

@narfbg The first problem came from the original repo that I was working with (which was using CI 2.1.2). I corrected that as well as the other problems you pointed out in the latest commit.

system/core/Router.php
((6 lines not shown))
{
- // Do we have a back-reference?
- if (strpos($val, '$') !== FALSE && strpos($key, '(') !== FALSE)
+ // Are we using callbacks to process back-references?
+ if(! is_string($val) && is_callable($val))
@narfbg Owner
narfbg added a note

This is still not following the style guide. You MUST put spaces after ifs and around !:

if ( ! is_string($val) && is_callable($val))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
system/core/Router.php
((8 lines not shown))
- if (strpos($val, '$') !== FALSE && strpos($key, '(') !== FALSE)
+ // Are we using callbacks to process back-references?
+ if(! is_string($val) && is_callable($val))
+ {
+ // Remove the original string from the matches array.
+ array_shift($matches);
+
+ // Get the match count.
+ $match_count = count($matches);
+
+ // Determine how many parameters the callback has.
+ $reflection = new ReflectionFunction($val);
+ $param_count = $reflection->getNumberOfParameters();
+
+ // Are there more parameters than matches?
+ if($param_count > $match_count)
@narfbg Owner
narfbg added a note

A space after if here as well.

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

@narfbg Corrected.

@narfbg
Owner

Looks good to me now. Just needs a +1 from some of the other team members that Phil has listed.

@jdfm

I need some input on this point:

The default behavior at the moment when a specific group of the route regex does not match is that the param that corresponds to that group is set to an empty value, what if the user's callback had default values? Example:


<?php

$route['products/([a-z]+)/list/(\d+)/(\d+)'] = function ($product_type, $page = 1, $items_per_page = 10)
{
    return "catalog/product_list/" . strtolower($product_type) . "/" . $page . "/" . $items_per_page;
};

The default behavior would replace the default numeric values of $page and $items_per_page with an empty string.

Should this be changed to use the defaults that the users provide? Should users be advised not to use default values for routes? I know that controllers are fully capable of having default values, so this might not be such a big deal, but if users rely solely on routes for default values it may cause confusion.

@narfbg
Owner

Let them have the ability to provide default values - anybody who decides to use this feature extensively would expect it to work like that. If their application relies solely on user input - they'd get in trouble anyway, even with the current routing system.

@narfbg
Owner

@jdfm ?

@jdfm

@narfbg That should do it. Sorry for the delay in posting this.

Are there any kind of unit tests that I should be writing and committing? I tested this last commit for a simple case or two, but I don't think that really suffices. Any tips or pointers on this subject?

@narfbg
Owner

There currently aren't any unit tests for the routing class (or at least I can't find any), but simply calling this should do the trick with the bonus of having CLI tested as well:

php /path/to/index.php request_uri
@narfbg
Owner

Just did some tests myself and couldn't break it - good work!
Can you add a changelog entry as well?

@narfbg
Owner

Now there's a conflict, you need to pull in changes from the main repository.

@pkriete

This is lovely. Great work, guys!

@jdfm

@narfbg I believe the conflict has been corrected. I'm new to git, so please excuse any n00bish results.

@narfbg
Owner

Okay ... here's what you need to do now - remove all the green lines in the changelog.rst diff, except for your own entry. The conflict markers explained:

<<<<<< HEAD // Start of conflicting code in YOUR repository
...
======= // End of your conflicting code; Start of conflicting code in the REMOTE repository
...
>>>>>>> c0mm17 // End of conflicting code chunks

When you encounter these, you need to manually figure out what needs to be kept and what not, which usually is some of both parts.

@jdfm

@narfbg That should do it.

@narfbg narfbg merged commit 0bae250 into bcit-ci:develop

1 check failed

Details default The Travis build failed
@wiredesignz

Who is ever going to use this? Just adding more bloat to the framework.

@cryode

@wiredesignz started a thread on the forums about his concerns; I've voiced my opinions there.

On another note, might I suggest reorganizing the if statements so that is_callable() is not in the first check. It's a bit nitpicky, but most users will use routes normally without callbacks, so there's a tiny performance drawback to the current order. Might be easier for people to follow the source code, also, if they're learning / trying to pinpoint a bug.

@wiredesignz

I am concerned also, from looking at the examples posted above, that you are attempting to set the base business logic for an application inside the routes closures. Thus if the controller or model don't also enforce business logic then altering routing will cause the application to fail. Routes should not do this, they should direct traffic not establish rules.
Default page numbers and other values should be set in the page controller.

Copying Laravel routing methodology might be cute, but you're missing the point entirely.

@narfbg
Owner

96ea528

The rest of it stays as is, unless you can add performance improvements to it. There's been a 3-months long discussion on this, nobody objected and people actually liked it - now is not the time.

@wiredesignz

@narfbg, That's a very close minded reply. But not unexpected.

I'm assuming this is still the 'develop' branch and nothing has been released yet so discussion here is still totally relevant.

@narfbg
Owner

@wiredesignz You want an already implemented (yet indeed, unreleased) feature to be removed, because you wouldn't use it. How open-minded is that? And the bottom line is this - an is_string() check won't impact your application's performance.

@wiredesignz

@narfbg, You're missing my point. This form of control is outside the scope of routing. I know that this is unlikely to be altered but I am voicing my concerns now because while everyone else seems to see only the shiny new code, I see the design problems this may introduce.

@cryode

I found another bug. Router.php starting on line 329:

// Is there a literal match?  If so we're done
if (isset($this->routes[$uri]))
{
    return $this->_set_request(explode('/', $this->routes[$uri]));
}

Given this test callback route, explode() throws an error, since the value of the array key is obviously not a string.

$route['callback'] = function()
{
    return 'welcome';
}

I think the callback processing code in Router.php will need to be a protected function, so that multiple calls can be made where appropriate.

Also, Andrey, you'll still need to perform is_string before running strpos() calls, or those will throw errors, also.

If I can help pull request any of this, let me know and I will.

@narfbg
Owner

@wiredesignz CI gives you the ability to use an anonymous function as a route. What you do with it is up to you and if your design sucks ... well that's too bad for you, if you care.

@cryode So why then was it important to move the strpos() checks prior to the ones for callbacks? is_string() is the first factor being checked anyway.

@cryode

(facepalm) You're right. That really doesn't impact the performance then (code review @ 4:45 am = bad). I still think it helps the source code flow, though, having the string functions first (and having is_string() paired with strpos()).

@wiredesignz

@narfbg, Do as you will, But I see very little use for this and if anything it is promoting poor design. I can envisage people trying to install apps developed using PHP5.3 with route closures on PHP5.2 servers. I can see developers trying to get access to the $CI super object inside their anonymous functions. It's just ugly.

@narfbg
Owner

@wiredesignz The documentation on this clearly states that it can only be used with 5.3. And I can also see people trying to use the super-object in a pre-system hook, but that's not a reason to not have hooks.

@cryode

A PHP version check with an appropriate error thrown would prevent that issue. The rest just shows a need for quality documentation about this feature. As someone against it, you have a unique perspective on what not to do - I bet that would help in writing said docs quite a bit.

@jdfm

@wiredesignz

I'd say any solution that facilitates a task has the potential to promote poor design if the people using it don't know what problem it was supposed to solve in the first place.

Note, if people think this should not be included in CI then by all means, don't include it. I contributed this code because I solved my problem with it and thought other people might like to have this feature as well. I did not write this code because some other framework has callbacks, notice that my original implementation was simply a string with a "php:" prepend.

My original use case was to reduce the number of routes you have to write for certain cases. I wanted to be able to take a match and transform it to fit my needs, you can already change where the matches will be placed in the final route, why not have the ability to alter the matches to fit your needs as well?

The original routing mechanism isn't affected by the addition of this feature, people who don't use it won't ever have a problem with it. Those who do use it, need to document their minimum requirements accordingly.

@wiredesignz

@jdfm, The issue I have with contributions like this is that they are simply hacks into the CI source to accommodate one developers lack of understanding of application design, or the purpose of routing or simply an inability to create a regex to solve their immediate problems. This provides very little benefit to other developers who know what they are doing and simply adds to code bloat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 20, 2012
  1. @jdfm

    allow for routes that can be processed with php, ex: $route['([^/]+)/…

    jdfm authored
    …([^/]+)(/:any)?'] = 'php:"$1" . "/do" . ucfirst("$2") . "$3"';
Commits on Aug 1, 2012
  1. @jdfm
Commits on Aug 6, 2012
  1. @jdfm
  2. @jdfm
  3. @jdfm

    New optional routing system, v3

    jdfm authored
  4. @jdfm
  5. @jdfm
  6. @jdfm

    Updated documentation

    jdfm authored
Commits on Aug 7, 2012
  1. @jdfm
Commits on Aug 30, 2012
  1. @jdfm
  2. @jdfm
Commits on Sep 27, 2012
  1. @jdfm
  2. @jdfm
Commits on Oct 23, 2012
  1. @jdfm
Commits on Oct 24, 2012
  1. @jdfm
  2. @jdfm

    Merge branch 'develop' of git://github.com/EllisLab/CodeIgniter into …

    jdfm authored
    …develop
    
    Conflicts:
    	user_guide_src/source/changelog.rst
  3. @jdfm

    fix a conflict, part 1

    jdfm authored
  4. @jdfm

    documented routing change

    jdfm authored
Commits on Oct 25, 2012
  1. @jdfm
Commits on Oct 31, 2012
  1. @jdfm

    removed conflict markers

    jdfm authored
  2. @jdfm
This page is out of date. Refresh to see the latest.
View
44 system/core/Router.php
@@ -371,10 +371,48 @@ protected function _parse_routes()
$key = str_replace(array(':any', ':num'), array('[^/]+', '[0-9]+'), $key);
// Does the RegEx match?
- if (preg_match('#^'.$key.'$#', $uri))
+ if (preg_match('#^'.$key.'$#', $uri, $matches))
{
- // Do we have a back-reference?
- if (strpos($val, '$') !== FALSE && strpos($key, '(') !== FALSE)
+ // Are we using callbacks to process back-references?
+ if ( ! is_string($val) && is_callable($val))
+ {
+ // Remove the original string from the matches array.
+ array_shift($matches);
+
+ // Get the match count.
+ $match_count = count($matches);
+
+ // Determine how many parameters the callback has.
+ $reflection = new ReflectionFunction($val);
+ $param_count = $reflection->getNumberOfParameters();
+
+ // Are there more parameters than matches?
+ if ($param_count > $match_count)
+ {
+ // Any params without matches will be set to an empty string.
+ $matches = array_merge($matches, array_fill($match_count, $param_count - $match_count, ''));
+
+ $match_count = $param_count;
+ }
+
+ // Get the parameters so we can use their default values.
+ $params = $reflection->getParameters();
+
+ for ($m = 0; $m < $match_count; $m++)
+ {
+ // Is the match empty and does a default value exist?
+ if (empty($matches[$m]) && $params[$m]->isDefaultValueAvailable())
+ {
+ // Substitute the empty match for the default value.
+ $matches[$m] = $params[$m]->getDefaultValue();
+ }
+ }
+
+ // Execute the callback using the values in matches as its parameters.
+ $val = call_user_func_array($val, $matches);
+ }
+ // Are we using the default routing method for back-references?
+ elseif (strpos($val, '$') !== FALSE && strpos($key, '(') !== FALSE)
{
$val = preg_replace('#^'.$key.'$#', $val, $uri);
}
View
1  user_guide_src/source/changelog.rst
@@ -269,6 +269,7 @@ Release Date: Not Released
- Added method ``strip_image_tags()``.
- Added ``$config['csrf_regeneration']``, which makes token regeneration optional.
- Added ``$config['csrf_exclude_uris']``, which allows you list URIs which will not have the CSRF validation methods run.
+ - Added possibility to route requests using callbacks.
Bug fixes for 3.0
------------------
View
11 user_guide_src/source/general/routing.rst
@@ -131,6 +131,17 @@ might be a good starting point.
..note:: You can also mix and match wildcards with regular expressions.
+Callbacks
+=========
+
+If you are using PHP >= 5.3 you can use callbacks in place of the normal routing
+rules to process the back-references. Example::
+
+ $route['products/([a-z]+)/edit/(\d+)'] = function ($product_type, $id)
+ {
+ return "catalog/product_edit/" . strtolower($product_type) . "/" . $id;
+ };
+
Reserved Routes
===============
Something went wrong with that request. Please try again.