IncomingRequest - Trim trailing slash

[najdanovicivan]

Description
If trailing slash is forced via apache or nginx the router will be unable to find the appropriate route. The router will try to match path from request uri with trailing slash but it is impossible to define such route in the route collection since slash is used as separator.

This changes the way request uri is handled so that trailing slash is removed at the end which enables the router to handle the url in both cases with or without trailing slash

Checklist:

• Securely signed commits
• [] Component(s) with PHPdocs
• [] Unit testing, with >80% coverage
• [] User guide updated
• Conforms to style guide

[paulbalandan]

Can you add a test for this?

[najdanovicivan]

@paulbalandan Since this is only being handled inside IncomingRequest instead of Router I've added test for the Incoming request.

With this implementation URI path will always be returned without trailing slash. I'm not sure it this is the best way to do it since with this implementation it is impossible to detect if url have trailing slash or not. The better solution might be to handle the stripping of slash inside Router class instead of stripping it from the URI path in incoming request.

With current setup $request->getPath() will return the same value of 'fruits/banana' for both $_SERVER['REQUEST_URI'] = '/index.php/fruits/banana/'; and $_SERVER['REQUEST_URI'] = '/sub/folder/index.php/fruits/banana';

[MGatner]

I'm hesitant about this one with all the URI issues we've had in the past. My hunch is that we need a way to distinguish between fruits/banana and fruits/banana/, but I'm not sure if that actually matters? Anyone know what the RFC says about trailing slashes?

[najdanovicivan]

@MGatner looking at PSR-7 underlying RCF document I found this https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.4

But I also found this middleware https://github.com/middlewares/trailing-slash which can do a redirection to force urls either with or without trailing slash

But none of this defines if there should be slash there or no

[MGatner]

After reading those and looking around a little more I think it is more clear that trailing slashes may differentiate a URI. For framework purposes I think this means we need to leave the URI intact for the developer. However, I also think it is logical for the Router to treat them as equivalent, since we don't have a meaningful way to differentiate the trailing slash in Route Collection anyways.

Instead of changing IncomingRequest, check out Router::checkRoutes(). There are two ltrim() calls in there that could be changed to trim() to ignore trailing slashes during route comparison. If you decide to implement this, we would need some updated test cases and maybe a side note in the User Guide.

[najdanovicivan]

Sure thing. I also found another issue that will occur if this is done in such way. Web Page Caching uses url to generate the md5 filename which means urls with slash and without will have different caches which in the end will result in cache using double amount of space if urls work with and without slash.

So maybe generating the cache filename should also strip the trailing slash

[lonnieezell]

I don't see a problem stripping trailing slash for the cache filename.

[najdanovicivan]

I've updated the code trim the path in Router class. I also fixed the trailing slash in page cache and while I was there I added method to be used for manually purging single cached page

[MGatner]

Much better! I like the new method but it will need test coverage. Please either add the tests or pull the method for a separate PR.

[najdanovicivan]

I've split the page cache in separate PR #5073 so that this one can be merged