feat: [Routing] add option to pass multiple URI segments to one Controller parameter

[kenjis]

Description
See https://forum.codeigniter.com/showthread.php?tid=88954&pid=414392#pid414392

• add an option to pass multiple URI segments to one Controller parameter

The current implementation passes the captured value to the controller after splitting it with /.
So if path/to/image-file.png is captured, the controller will receive three parameters.
This PR adds an option to pass it to one parameter.

E.g.,

$routes->get('img/(:any)', 'Image::display/$1');

<?php

namespace App\Controllers;

use App\Controllers\BaseController;
use CodeIgniter\HTTP\ResponseInterface;

class Image extends BaseController
{
    public function display(...$params)
    {
        dd($params);
    }
}

Navigate to http://localhost:8080/img/path/to/image-file.png.

The current behavior:

$params array (3)
    ⇄0 => string (4) "path"
    ⇄1 => string (2) "to"
    ⇄2 => string (14) "image-file.png"

The option is enabled:

$params array (1)
    ⇄0 => string (22) "path/to/image-file.png"

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[lonnieezell]

For what it's worth, (:any) used to catch everything to the end of the path. I'm not sure when that changed, but I think it's worth investigating that. If it's recent enough I would find it better to fix that unintended bug and put it back the way it was instead of adding yet another configuration option.

[kenjis]

No, there is no bug. (:any) catches all segments all the time.
But if it caches multiple URI segments, the controller receives multiple parameters.

For example, define the following:

$routes->get('products/([a-z]+)/(.+)', 'Products::show/$1/id_$2');

And navigate to the URI products/shirts/123/456, theshow(...$params) method parameters would be like:

$params array (3)
    ⇄0 => string (6) "shirts"
    ⇄1 => string (6) "id_123"
    ⇄2 => string (3) "456"

But it is intuitive that 'Products::show/$1/id_$2' means the show() method will receive two parameters 'shirts' and 'id_123/456'.

[michalsn]

Ok, so this PR will give us the ability to change the way we pass the parameters to the controller?

Now, if I have a route like this:

$routes->get('product/(:any)', 'Catalog::product/$1');

Then my controller may look like this:

class Catalog extends BaseController
{
    public function product($seg1 = false, $seg2 = false, $seg3 = false)
    {
        d($seg1, $seg2, $seg3);
    }
}

Then calling the URLs will return:

// product/1
// returns: 1, false, false

// product/1/2
// returns: 1, 2, false

// product/1/2/3/4
// returns: 1, 2, 3

With this change I will have option to build my controller like this:

class Catalog extends BaseController
{
    public function product($segments)
    {
        d($segments);
    }
}

Then calling the URLs will return:

// product/1
// returns: [1]

// product/1/2
// returns: [1, 2]

// product/1/2/3/4
// returns: [1, 2, 3, 4]

If that's correct then I'm not sure if we need this feature. I believe using _remap should handle such a scenario already.

// Edit
Sorry, I linked to the wrong docs.

[kenjis]

With this option, the $segments will be:

// product/1
// returns: '1'

// product/1/2
// returns: '1/2'

// product/1/2/3/4
// returns: '1/2/3/4'

[michalsn]

I see, thanks.

Still, this can be handled with _remap(), which gives us a lot of flexibility in parameter formatting.

[kenjis]

This changes the number of route parameters.
So I'm not confirmed, but the parameters to _remap() also would be changed.

[michalsn]

I'm not quite sure what you're referring to. To the changes in this PR or to _remap() in general?

I'm referring to the use of _remap() right now (without this PR).

In _remap() I can simply use implode() and have the result, you have presented before - all parameters as a string.

[kenjis]

Without using _remap(), using ...$params can handle multiple parameters.
See the second sample controller in https://codeigniter4.github.io/CodeIgniter4/incoming/routing.html#the-behavior-of-any

$routes->get('product/(:any)', 'Catalog::product/$1');

When I first saw the above route, I interpreted the product() method will receive one parameter.
The / after the method means the separator of parameters.
So if (:any) matches 123/456, $1 would be 123/456, and the first parameter will be also 123/456.

$routes->get('product/(:segment)/(:any)', 'Catalog::product/$1/$2');

I thought this means the first parameter is the value of (:segment) and the second parameter is the value of (:any). I couldn't imagine not knowing how many parameters the method would receive.

Am I the only one who thinks this interpretation is intuitive?

[michalsn]

Without using _remap(), using ...$params can handle multiple parameters.

LOL... you're right! I missed this one 😅

$routes->get('product/(:segment)/(:any)', 'Catalog::product/$1/$2');

I thought this means the first parameter is the value of (:segment) and the second parameter is the value of (:any). I couldn't imagine not knowing how many parameters the method would receive.

Am I the only one who thinks this interpretation is intuitive?

Ok, with this example, I get your point a little better now. Although it's nothing that can't be handled right now. We can simply use:

public function product(...$segments)
{
        d(array_shift($segments));
        dd(implode(',', $segments));
}

Or even move it to the _remap(), to not handle these operations inside the controller's method.

The question is how common this problem is. I agree that this PR would simplify the handling of such cases, but is it worth complicating the router code?

// Edit
Fixed a typo.

[kenjis]

I think this option's behavior is easier to understand than the current behavior.
I would like to make it the default behavior in the future.

For example,

$routes->get('img/(:img)', 'Image::display/$1');

If (:img) matches path/to/image_file.png, we can get the value as the first parameter.

Also, I don't know if this use case actually exists, but if you want a route that uses two or more placeholders that match multiple segments, it may be impossible to get the placeholder values with implode().

$routes->get('foo/(.+)_(.+)', 'Foo::bar/$1/$2');

[lonnieezell]

First off, I don't know that I'm a fan of this as a default. I understand why it makes sense, but when you compare it to the others it kind of doesn't. Here's why:

Each placeholder defines a single string that it matches and passes the controller. Changing the behavior means that now this one placeholder has a different function than the rest.

It was never intended to return each segment, it was to return from the beginning of the segment to the end of the URI's path. Which is what it does. This is useful if you know you need to capture other URIs in a segment, or whatever special codes you use that might include a forward slash.

So, this isn't a bug. It operates as designed. This is not something worth being a BC break in the future for, I don't think.

To me this is something that has affected very few people (this is the first I've heard someone bring it up personally). And it's something that they can easily change in the _remap method as discussed, or in the designated method with a simple explode call. To me that's also not worth the additional complexity in the code. I know it's a small thing, but they add up. I'd be interested to see what the cyclomatic complexity of the routing system is already.

I think we need to be more discerning in what we add or change, based in large part on how much its causing a problem and how much it impacts current devs.

[kenjis]

I understand that enabling this option will certainly change the behavior and that we should be cautious about changing the default settings.

However, the following explanation is not clear to me what you are trying to say.

Each placeholder defines a single string that it matches and passes the controller. Changing the behavior means that now this one placeholder has a different function than the rest.

It was never intended to return each segment, it was to return from the beginning of the segment to the end of the URI's path. Which is what it does. This is useful if you know you need to capture other URIs in a segment, or whatever special codes you use that might include a forward slash.

Placeholders are regular expression patterns. So the string it captures will be not changed when this option is enabled. The idea is to change the behavior that the captured value is currently passed to the controller as multiple parameters to always pass it as a single parameter.

To be sure, I put a sample of the differences in behavior.

$routes->get('img/(:any)', 'Image::display/$1');

<?php

namespace App\Controllers;

use App\Controllers\BaseController;
use CodeIgniter\HTTP\ResponseInterface;

class Image extends BaseController
{
    public function display(...$params)
    {
        dd($params);
    }
}

Navigate to http://localhost:8080/img/path/to/image-file.png.

The current behavior:

$params array (3)
    ⇄0 => string (4) "path"
    ⇄1 => string (2) "to"
    ⇄2 => string (14) "image-file.png"

The option is enabled:

$params array (1)
    ⇄0 => string (22) "path/to/image-file.png"

[lonnieezell]

Oh, wait, I think I've been looking at this backward the whole time for some reason. I thought the current implementation was to pass it as a single string and it was being changed to pass it in as an array.

In that case I approve because I'm pretty sure that's how it functioned when I wrote it years ago and not sure when it got changed to split it up. At least that's what I recall my intent being. :)

So thank you for the detailed examples.

[kenjis]

The current implementation passes the captured value to the controller after splitting it with /.
So if path/to/image-file.png is captured, the controller will receive three parameters.
This behavior comes from CI3.
In the sample code above, I use ...$params, so we receive as an array.

[michalsn]

Ok, this will be a nice feature for more complex routes.

[MGatner]

One small request.

Also, are there any security implications to this? Trying to think of places that a slash delimitation could be used to inject route code... not coming up with anything though.

[MGatner]

Would you please add a test showing that queries are not included?

product/123/456?id=789

[kenjis]

@MGatner Do you mean $router->handle('product/123/456?id=789');?
If so, it is not needed, because the parameter is $uri. It never contains queries.
In other words, if product/123/456?id=789 is passed to handle(), the client code is wrong.

CodeIgniter4/system/CodeIgniter.php

Lines 826 to 830 in b28af1d

	$uri = $this->request->getPath();
	$this->outputBufferingStart();
	$this->controller = $this->router->handle($uri);

[kenjis]

Can I merge this?

[michalsn]

Let's wait until the weekend. If @MGatner does not raise further questions I think we can consider that doubts are clarified.