Skip optional parameters in Segment assembly if they are empty

[lklapa]

Feature Request

Q	A
New Feature	yes
RFC	yes
BC Break	no

Summary

With a route defined with optional parameters:

$route = $app->get('/files[/:controller][/:action][/:projects_id][/:file][/:revision]', $middleware, 'files');
$route->setOptions(
    [
        'constraints' => [
            'controller'  => '[a-zA-Z][a-zA-Z0-9_-]*',
            'action'      => '[a-zA-Z][a-zA-Z0-9_-]*',
            'projects_id' => '[0-9]+',
            'file'        => '[0-9]+',
            'revision'    => '[a-zA-Z0-9]*',
        ],
        'defaults'    => [
            'controller'  => 'Index',
            'action'      => 'index',
            'projects_id' => '',
            'file'        => '',
            'revision'    => '',
        ],
    ]
);

the vendor/laminas/laminas-router/src/Http/Segment.php file performs assembly in a way that adds unnecessary slashes.

See the following examples:

$this->url('files', ['projects_id' => 123, 'file' => 456, 'revision' => 789])
produces
/files/Index/index/123/456/789
(correct, both controller and index are substituted from default values).

However, the helper with parameters
$this->url('files', ['projects_id' => 123])
produces
/files/Index/index/123//
(incorrect, as values for file and revision should be empty and therefore skipped along with their slashes).

The expected route assembly should read /files/Index/index/123 as it was done in Laminas and MVC router.

We are trying to migrate from Laminas to Mezzio, and the routing is an important path of our bridge.

The Segment.php file in buildPath function catches the optional parameters correctly, ending up in the following code:

case 'optional':
                    $skippable    = true;
                    $optionalPart = $this->buildPath($part[1], $mergedParams, true, $hasChild, $options);
                    if ($optionalPart !== '') {
                        $path .= $optionalPart;
                        $skip  = false;
                    }
                    break;

At the same time, the $optionalPart is never going to be empty, as it holds the / value, therefore producing an incorrect url.

I am aware that ideally we should divide it into separate routes, but given the amount of various usages of url helpers in all views across the app it would be quite a tedious task.

If in Segment.php the if would be changed from
$optionalPart !== ''
to
$optionalPart !== '' && $optionalPart !== '/'
that would result in optional parameters being really skipped, without adding only the slash to the url.

In case this solution is not possible, I would really appreciate suggesting me some alternative solutions on how to migrate Laminas routers to Mezzio ones in the most effective way. Many thanks in advance.

[gsteel]

It's been a while since I used laminas router, but shouldn't empty defaults be defined as null in the route? i.e:

// ...
        'defaults'    => [
            'controller'  => 'Index',
            'action'      => 'index',
            'projects_id' => null,
            'file'        => null,
            'revision'    => null,
        ],
//...

And also, I thought laminas router could do nested, optional params, i.e.:

[/:projects_id[/:file[/:revision]]]

[lklapa]

Unfortunately, default values of null trigger Laminas\Router\Exception\InvalidArgumentException with Missing parameter "file" message. This is why I tried to make it work using empty strings.

Nested parameters is something we started with, as in Laminas this route is indeed defined in the following way:

/files[/:controller[/:action[/:projects_id[/:file[/:revision]]]]]

However, both routes demonstrate the same behaviour, that is for optional parameters the slashes are always added.

[krytenuk]

I am seeing the same when generating a URL with the LaminasRouter::generateUri() method and using auto-wired routes. This route returns the above Missing parameter error when action is set to null, leaves a trailing forward slash when set to an empty string and works correctly when passing a populated string.

[
    'path'            => '/sculptor[/:action]',
    'middleware'      => Handler\InnerPageHandler::class,
    'allowed_methods' => ['GET'],
    'name'            => 'sculptor',
    'options'         => [
        'defaults'    => [
            'action' => null,
        ],
        'constraints' => [
            'action' => '[A-z0-9_-]+',
        ],
    ],
],

The only workaround would be to use two routes for the above, something like this.

[
    'path'            => '/sculptor,
    'middleware'      => Handler\InnerPageHandler::class,
    'allowed_methods' => ['GET'],
    'name'            => 'sculptor',
],
[
    'path'            => '/sculptor/:action',
    'middleware'      => Handler\InnerPageHandler::class,
    'allowed_methods' => ['GET'],
    'name'            => 'sculptor.action',
    'options'         => [
        'defaults'    => [
            'action' => 'some-default-action',
        ],
        'constraints' => [
            'action' => '[A-z0-9_-]+',
        ],
    ],
],

Not ideal but should work 🤔

[lklapa]

@krytenuk this makes sense. However, in our case we have multiple routes defined with a number of optional parameters (I simply provided just one example), and the current MVC implementation makes use of such routes in a flexible way. Whatever is provided is being used to construct the URL. What is not provided, is optional, and does not have a default value, is skipped (=expected behaviour).

As there are different combinations available when you have multiple optional parameters, for us dividing the routing would be a much more tedious task.

This is something that can of course be changed in the long-term, but at least for the transition period ideally the router should simply ignore entirely optional parameters that are:
a) not provided,
b) do not have default values.

[gsteel]

I'm not convinced this issue belongs here, because url generation is delegated directly to Laminas\Router\Http\TreeRouteStack::assemble():

mezzio-laminasrouter/src/LaminasRouter.php

Lines 109 to 129 in cab61d9

	public function generateUri(string $name, array $substitutions = [], array $options = []): string
	{
	// Must inject routes prior to generating URIs.
	$this->injectRoutes();
	if (! $this->laminasRouter->hasRoute($name)) {
	throw new Exception\RuntimeException(sprintf(
	'Cannot generate URI based on route "%s"; route not found',
	$name
	));
	}
	$name = $this->routeNameMap[$name] ?? $name;
	$options = array_merge($options, [
	'name' => $name,
	'only_return_path' => true,
	]);
	return $this->laminasRouter->assemble($substitutions, $options);
	}

Are you sure that this behaviour isn't broken in a perhaps more recent release of laminas-router itself?

Can you send in a failing test case, or indeed a passing test case, over at laminas-router?

[lklapa]

You are right, I am still getting confused between laminas-router, mezzio-router, and mezzio-laminasrouter.

Anyway, I wanted to give some more info, maybe they will be useful to @krytenuk.

I started with an empty mezzio-skeleton to play around with routing and url helpers a bit more. Following the documentation, I added my sample route to the routes.php:

$testRoute = $app->get('/files-mezzio[/:controller[/:action[/:projects_id[/:file[/:revision]]]]]', function () {}, 'files-mezzio');
$testRoute->setOptions(
    [
        'defaults' => [
            'controller' => 'Index',
            'action' => 'index',
            'projects_id' => '',
            'file' => '',
            'revision' => ''
        ]
    ]
);

Then, in the home-page.phtml I used UrlHelper to echo the link: <?= $this->url('files-mezzio', ['controller' => 'test', 'projects_id' => '1']) ?>. The result was /files-mezzio/test/index/1// (which is wrong).

In the meantime, my colleague was exploring the option of adding another routing using an explicit factory. For this, in factories we added the following:

RouterInterface::class => function ($container) {
    return new \Mezzio\Router\LaminasRouter($container->get(\Laminas\Router\Http\TreeRouteStack::class));
},
TreeRouteStack::class => TreeRouteStackFactory::class,

In the TreeRouteStackFactory we added the following in the __invoke method:

$router = new TreeRouteStack();
$router->addRoute('files-tree-route-stack', [
    'type'    => 'segment',
    'options' => [
        'route'       => '/files-trs[/:controller[/:action[/:projects_id[/:file[/:revision]]]]]',
        'defaults'    => [
            '__NAMESPACE__' => 'Files\Controller',
            'controller'    => 'Index',
            'action'        => 'index',
        ],
    ],
]);
return $router;

just to try thing out.

It turns out that the UrlHelper sees both routes (from routes.php and from the factory), but only the one from the factory produces the correct link through UrlHelper, even though they both end up in the Segment.php file that I was suspecting earlier to be the cause.

At this stage, the results were as follows:

$this->url('files-mezzio', ['controller' => 'test', 'projects_id' => '1']) => /files-mezzio/test/index/1// (incorrect)

$this->url('files-tree-route-stack', ['controller' => 'test', 'projects_id' => '2']) => /files-trs/test/index/2 (correct)

After some more digging and comparing what is provided to the buildPath function, it turned out that the incorrect route is caused by the fact that the parameter $hasChild is set to true in the wrong "mezzio" route. When it is set to false, the UrlHelper produces a valid link.

This is because the route name in case of the "mezzio" route passed to the buildPath is named files-mezzio/GET, whereas the one from the TreeRouteStack is simply named files-tree-route-stack.

The function assemble in TreeRouteStack splits the route name by /, and if it sees the second part, it then looks for child routes, sets the new name of the segment to the part of the current route, and performs another call. If the name of the route does not contain /, then there is unset($options['name']); called.

The unset is critical, as Part from laminas-router executes $options['has_child'] = isset($options['name']);. If the route has a name that has not been unset, has_child becomes true, and therefore buildPath from Segment receives $hasChild = true, resulting in an incorrect link being assembled.

To wrap up this short post, the difference is that "mezzio" routes are named like files-mezzio/GET in injectRoute method inside mezzio/mezzio-laminasrouter/src/LaminasRouter.php, whereas an explicit TreeRouteStack populated manually is named without the HTTP method added as a part of the route name.

I understand this might not be considered a bug or anything, but as it was confusing for me, I thought it might be worth sharing.

[krytenuk]

@lklapa I have found that using the $app->any in the route seems to get it to work for me using a fresh install of the Mezzio skeleton application. Omitting the allowed_methods key when auto-wiring the routes also has the same effect.

$app->any('/application[/:action]', HomePageHandler::class, 'application');

or

[
    'path' => '/application[/:action]',
    'middleware' => HomePageHandler::class,
    'name' => 'application',
    'options' => [
        'defaults' => [
            'action' => null,
        ],
        'constraints' => [
            'action' => '[A-z0-9_-]+',
        ],
    ],
],

I have found a strange behavior when using the URI view helper to generate the link. Using my example link above, when I am on the home page, https://localhost, I can generate the correct URL with.

<?= $this->url('application'); ?>

And I can still get the correct URL when I am in the application page https://localhost/application. However when I am on an action page, such as https://localhost/application/test, the view helper returns a link to the same page, https://localhost/application/test, rather than https://localhost/application. I found, setting the action parameter to null does return the correct URL.

<?= $this->url('application', ['action' => null]); ?>

I have not tried any complex routes as of yet.

I will do some more digging when I get the chance and see if I can come up with a fix. I am new to Mezzio to be fair, come from a Laminas MVC background.

[Kerion]

I've got an simular Problem. This problem occurs, if "allowed_methods" are configured and i try to get an url of the route without the optional Parameter.

[
	'name' => 'foo',
	'type' => 'segment',
	'path' => '/foo[/:id]',
	'allowed_methods' => ['GET', 'POST'],
	'options' => [
		'constraints' =>
			[
				'id' => '[0-9]+'
			],
	],
	'middleware'      =>  FooHandler::class,
]

If i Try to generate an URL without the optinal Parameter 'id', i get an InvalidArgumentException in the Parameter-Case of the buildPath Method of Laminas\Router\Http\Segment. It does so because $hasChild is always true here, $isOptional=true has no effect because of this.

$this->url('foo');

The Problem is, that Mezzio\Router\LaminasRouter defines two "pseudo" child-routes for the allowed HTTP-Methods and one for "method_not_allowed" and adds the allowed_methods to the routeName for the assemble-Options so the name there is "foo/GET:POST" and the Route is set to may_terminate=false.

The assemble-Method of Laminas\Router\Http\TreeRouteStack explodes the name and leaves "GET:POST" in $options["name"]

The assemble-Method of Laminas\Router\Http\Part triggers the assemble -> buildPath - Method of Laminas\Router\Http\Segment. Here we have the problem, that the not terminatable "parent"-Route is assembled and 'has_child' is set to true, because the "pseudo" Child-Routes can't be assembled.

I solved this problem by editing the assemble-Method of Laminas\Router\Http\Part replacing:

$options['has_child'] = isset($options['name']);

with

$options['has_child'] =  (bool) !preg_match('/^(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)(:(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS))*$/i', $options['name'] ?? '');

With this change, in case of an Mezzio-Pseudo-Child-Route with HTTP-Methods as Child-Route, the Segment-Class is called with has_child=false. $option['name'] is still set and the Exception for 'Part route may not terminate' will not be triggered.

Should i report an Issue for this in laminas-router for getting this Change implemented or does someone has an better solution?

[froschdesign]

We need a unit test that illustrates the problem. If the problem is in laminas-router then we need a unit test there.
Including the code here in the report won’t be of much help, as hardly anyone – if anyone at all – will copy it and reproduce it locally. A continuous integration pipeline is running in all repositories, so we can see the test results straight away. It would therefore be useful to have a unit test that fails.

Thanks in advance! 👍🏻