Fix index matching with trailing slashes

[melo]

Mason 2.20 fails in this situation: with a index.mc inside a foo/ directory:

• GET /foo: the index.mc is found and used;
• GET /foo/: no match is found.

This pull request has the failing test cases for now. Working on a fix at the moment.

[melo]

Ok, works now.

I've started by reverting the previous #8 fix, reverting the code changes from PR #11, and then added the new tests, and finally made the tests from #11 (most of them, some were just wrong) work.

@jonswar I'll double check the tests, and test it with my app; then I'll update the docs (in this branch right now, they are wrong) and add them to this PR.

[melo]

Done, all test pass, documentation up-to-date.

[melo]

Done.

Did a bit of cleanup, building @candidate_paths was getting messy.

@pstadt comments?

@jonswar You might need to tidy up later. I tried to use your tidyall.ini but you are using a private JS::Code::TidyAll class

[pstadt]

Testcases are still not complete, it feels like opening pandora's box ;-)

Can you please have a look at test_decline a few lines later and the expected decline-order? As far as I can see

$try->( '/foo.mc',              'bar' );
$try->( '/foo/dhandler.mc',     'bar' );

is not correct the correct order according to the docs but it matches successful.

So here are the new tests:

$try->( '/news/sports', ['/dhandler','/news.mc=1'], '/news.mc', 'sports');
$try->( '/news/sports', ['/news/dhandler','/news.mc=1'], '/news/dhandler', 'sports');
$try->( '/news/sports/', ['/news/dhandler','/news.mc=1'], '/news/dhandler', 'sports/');

Gets more and more confusing. Check order in general has to be @idx_dhandler, @filtered_autoext. Check order has to be @filtered_autoext. @idx_dhandler for the first loop ($path_info eq '') only if $trailing_slash eq ''. I this correct? Then this would be the correct code:

if ($trailing_slash eq '' && path_info eq '') {
    @candidate_paths = (@filtered_autoext, @idx_dhandler);
} else {
    @candidate_paths = (@idx_dhandler, @filtered_autoext);
}

Now I will have a closer look on

if ($path_info eq '' && !@autoextensions)

as path_info was set to '/' in the old code but now it is always empty at the first loop.

[melo]

I need to take a break and step back from it all...

With #8, I only wanted to make sure that path_info included the trailing slash if I requested id with allow_path_info => 1. That is all.

I would like to run the current test suite on this PR with 2.19, and see if those tests that @pstadt mentions fail or not. If not, then they are correct, and the documentation is misleading, and needs to be updated. I don't want to change the matching semantics, just want to capture the trailing slash in path_info.

@jonswar 2.20 is a broken release due to my #11 pull request. I think it should be reverted ASAP and release 2.21, and afterwards, if this PR is stable and makes sense, you can if you wish make a 2.22 release with it.

If you agree, tell me and I'll prepare a new PR to revert #11. It should be as simple as cherry pick baa1643 from this PR and then remove the documentation (this patch https://github.com/melo/perl-mason/commit/7a17fe0230a0663cfe1caec458ad4e24ecbc35f8#L1R71 from lib/Mason/Manual/RequestDispatch.pod).

I need to work now, I'll get back to this tomorrow with a fresher mind.

[pstadt]

I think we are on a good way.

I am not sure if 2.19 is a good basis to work on, as it has contradictory statements about matching in code comments and RequestDispatch.pod (and an implementation). And 2.19 does not define how to deal with trailing slashes except in code.

I think best is to go back to drawing board what should be the matching order.

As far as I can see there might be only one incompatible change between new and old: thats what the decline_test complains about with latest changes.

And it might not be necessary to do a quick 2.21 release because noone complained for half a year about the 2.20 release

[melo]

I don't know right now.

If I ever see someone with /foo.mc and one of /foo/index.mc or /foo/dhandler.mc, my reaction would probably be:

So...

I don't mind doing the work, but clearly, at least for me, a step back is required, and that means 2.19, because 2.20 with #11 clearly broke more that it fixed.

[pstadt]

Strictly spoken a dhandler is identical to an index component with the full meaning of allow_path_info if set: intercept on every level and return path_info. But I think this would go to far ;-)

[pstadt]

Played around a little with the test and 2.19, these are all successful:

    $try->( '/news/sports', ['/news/dhandler','/news/sports.mc'], '/news/sports.mc', '');
    $try->( '/news/sports', ['/news/dhandler','/news/sports.mc=1'], '/news/sports.mc', '');
    $try->( '/news/sports/', ['/news/dhandler','/news/sports.mc=1'], '/news/dhandler', 'sports');
    $try->( '/news/sports/', ['/news/dhandler','/news/sports.mc'], '/news/dhandler', 'sports');
    $try->( '/news/sports/hockey', ['/news/dhandler','/news/sports.mc=1'], '/news/sports.mc', 'hockey');
    $try->( '/news/sports/hockey', ['/news/dhandler','/news/sports.mc'], '/news/dhandler', 'sports/hockey');

I think the result of the 3rd test is a bug (only about components, not about path_info).