Route Problem with optional Parameters and End Trailing Slash

[gingebaker]

Hello

I have a problem that I cannot use optional route parameters like shown in Fast Route Example:

// The /{title} suffix is optional
$r->addRoute('GET', '/articles/{id:\d+}[/{title}]', 'get_article_handler');

I get following exception:

{
    "error": "Optional segments can only occur at the end of a route",
    "devmessage": {
        "class": "FastRoute\\BadRouteException",
        "code": 0,
        "message": "Optional segments can only occur at the end of a route",
        "location": "/site/modules/AppApi/vendor/nikic/fast-route/src/RouteParser/Std.php",
        "line": 35
    }
}

The Problem refers to the added trailing slash in Router.php
https://github.com/Sebiworld/AppApi/blob/master/classes/Router.php#L59
So the last ] bracket can never be at the end of the route.

I don´t know exactly if the adding of this end trailing slash is crucial? At least in my setup all the routes are working so far without the trailing slash.

An option would be to check not only for "/" but also for "]" at the end. Like:

// add trailing slash if not present:
if (!in_array(substr($url, -1),array("/","]"))) {
  $url .= '/';
}

[Sebiworld]

Hi @gingebaker , thank you for reporting this issue!

I don't see that it is necessary to add the slash after each url, and it would be good to support all of FastRoute's features. But we must be careful, because we do not want to create a breaking change that requires updating existing route-definitions. I will look into it!

This part of the code is actually still from the RestApi module, which was the basis for AppApi. So, maybe @thomasaull can say something about this?

[thomasaull]

@gingebaker, @Sebiworld I honestly can't remember the exact reason I included this part, I guess there was a reason though. Maybe it had something to do with the template setting in ProcessWire „Should URL segments end with a trailing slash” — the initial version used a template instead of a hook to provide the endpoint.

Anyway I've made some tests:
When I comment those lines out in Router.php

// add trailing slash if not present:
// if (substr($url, -1) !== '/') {
//     $url .= '/';
// }

and I define this route in Routes.php:

['GET', 'test-default/', Example::class, 'testDefault'],

requests to /test-default aswell as /test-default/ both work.

However if I remove the trailing slash on the route:

['GET', 'test-default', Example::class, 'testDefault'],

requests to /test-default aswell as /test-default/ both don't work.. I'm getting a Route not found error.

So my guess is, I put this in so the user does not have to remember to do this… Probably this could be fixed differently.

[gingebaker]

Hello @thomasaull
Thanks for the information.

I also think that is not a good idea to remove the traling slash addon completely.
Did you see my workaround:

// add trailing slash if not present and last char is not an ]
if (!in_array(substr($url, -1),array("/","]"))) {
  $url .= '/';
}

So only when the last char is an "]" we don´t add an trailing slash. This should be no problem with existing setups, because this would be an exception anyway. But you could then add the optional parameters from fast route. ( https://github.com/nikic/FastRoute#usage )

[thomasaull]

@gingebaker Yes, I noticed that and this could be a potential fix yes :) However, I think it's worth investigating why the module has problems with a non-existing trailing slash in the first first place. According to the FastRoute documentation these should not be necessary.

[Sebiworld]

Hey @thomasaull ,
I did some tests and tried to figure out the problems that you mentioned in your previous post:

"However if I remove the trailing slash on the route:

['GET', 'test-default', Example::class, 'testDefault'],

requests to /test-default aswell as /test-default/ both don't work.. I'm getting a Route not found error."

That is because we manually add a trailing slash to the current route, too: https://github.com/Sebiworld/AppApi/blob/master/classes/Router.php#L117

If I understand FastRoute correctly, it does not automatically ignore non-existing trailing slashes. If you define a route with a trailing slash, you have to call it with a trailing slash. The module automatically adds a trailing slash to every route (if it does not exist) and does the same with the incoming current route as well.
And because of this behaviour, it doesn't matter whether we call up a URL with or without a trailing slash, it will always be found. I think that's good too and it helps to avoid errors. I don't see any reason why you wouldn't want this usability improvement in any case.

But I also want to support all features that FastRoute offers. @gingebaker Are there perhaps more characters in the FastRoute route-definition that we should look out for? Or do you know if there is another way to ignore the trailing-slash in FastRoute? Then maybe we could omit the manual adding completely?

I will have another close look at the FastRoute documentation, too...

[thomasaull]

@Sebiworld In my tests I removed this part of the code, which adds the traling slashes to the routes (check my comment again 🙃) The strange thing happening was, that for the endpoint test-default (no trailing slash) both calls, with and without a trailing slash, resulted in a Route not found error. That's why I suggested there might be some other problem with the module, since in this case the call without the trailing slash should work.

Otherwise I agree with what you said :)

[twinklebob]

I remember there being some weirdness with slashes when I was making the change to how the route map was built up, so that I could have nested arrays. That could be having some impact here too.