Consistent prefixes: variables and leading forward slashes: #817
Conversation
I couldn't break this down into multiple commits so instead a
single commit which fixes two issues. They are both tricky to
grasp and one might introduce breakage (while reducing breakage,
I guess), so they're important to understand.
The first relates to allowing variables in prefixes with a single
path request. If your route is simply '/', variables in the prefix
are simply ignored:
prefix '/:board' => sub {
get '/' => sub {...}
};
Now the actual route registered is '/:board/', literally. That
means that it won't accept variables but only '/:board/'.
The second issue is accepting anything that does not start with
a leading forward slash. This would be an error:
get '' => sub {...}; # breaks
If you have a prefix and you want to still grab the basic path
without the ending slash it would fail:
prefix '/:board' => sub {
get '' => sub {...}; # breaks
};
This is now fixed too.
One thing to research and perhaps write a test for is the behavior
using path() or path_info(). path() is defined by
path_info() || '/', which might be a problem.
Another issue to recognize is that if someone had the following
code and we don't cover for it, they will receive a 404:
prefix '/:board' => sub {
get '/' => sub {
# expecting to grab both /foo/ and /foo
};
};
This, if successful, closes GH #558 and possibly GH #793.
|
Looks good. The behavior I'm expecting (and getting) is: use strict;
use warnings;
use Test::More tests => 1; # last test to print
{
package foo;
use Dancer2;
prefix '/sawyer' => sub {
get 'x' => sub { 'hell yeah' };
};
prefix '/:thing' => sub {
get '/' => sub {
return 'with the dash';
};
get '' => sub {
return 'without the dash';
};
};
}
use Test::WWW::Mechanize::PSGI;
my $mech = Test::WWW::Mechanize::PSGI->new(
app => Dancer2->psgi_app
);
$mech->get_ok( '/foo/' );
$mech->content_is( 'with the dash' );
$mech->get_ok( '/foo' );
$mech->content_is( 'without the dash' );
$mech->get_ok( '/sawyerx' );
$mech->content_is( 'hell yeah' );
and that's excellent. I'll try to see about |
|
|
|
Christmas festivities has stopped me looking at this in great detail :( Hoping I can in the next 24 hours.. @yanick The PSGI spec does allow |
|
I don't agree with the part of this change that allows route paths to be an There are parallels between (stringified) route paths and the values for In my opinion, @yankic's example of should not work. I'd rather see a construct like While its more verbose, its more predictable. |
|
It might be the hour, but @veryrusty, I wasn't able to follow your response. While PSGI allows for According to HTTP, the following are not equal: Currently, with the following code: will match both /hello and /hello/ which doesn't provide a lot of control for the user. Further more, what happens when we mount? It gets even weirder. We have to sort this out. AFAIK, Ugh. |
|
I'd like to see the possible strings when defining routes as either an empty string OR a string that begins with Hopefully some code helps; I've pushed an extra commit to the branch: veryrusty/prefix-consistency |
|
@veryrusty is there a reason why a route can't be 'this+that' if there's a prefix that starts with '/'? |
|
I think the previous issues that lead to routes needing to begin with a caused confusion since internally the prefix is converted to I can also see advantages in allowing I don't think there is any technical reason @xsawyerx. We usually want consistency; allowing the second case but not the first (which is what I think you are suggesting), while slightly inconsistent, does put us in a better place that we are now. |
|
I think both of these cases should work. A prefix of I don't see why these should differ. |
|
It seems that our main issue here is not the technicality of it, but rather to pin down the specs of how we want the routes and prefix to work. Let's see if I can summarize the options open to us:
Meaning that you can't do have a segment being in part defined by the prefix and the path like Note that this means that this implies a route segment should always begin with '/' or be empty.
That's what, I think, @veryrusty goes for with not allowing things like
Basically, recommend peeps to usually define their routes as specified in (1), but let the door open for them to do as they please. I think that (3) should be easier to implement (the uri segments are just bits of regexes for us to stitch together), and give a lot of freedom in the hands of the power users. Agreed, that could potentially cause confusion with the beginners, but we could mitigate that by (a) having warnings print out something by default if a route is defined that doesn't match |
|
Thanks @yanick for the cool head - pinning down the spec is what's this is about. The 3rd option gives the most flexibility, and after some reflection is my preferred option too. I think the only tricky/technical part is dealing with the "default" prefix does this throw an error for the first route (
Thoughts ? |
|
Just a note: although we might support strange constructs, like that prefix for sawyer, and route for the x, and although we should test them, in order to know things are accordingly our "internal" decision, that doesn't mean we need to document all the details, and just document "what you should do". If some user finds out that he can tweak a little more, he will, hopefully, understand the logic. Well, my cents, but still not sure this is the best approach. |
Adds an explicit test for token handling for constructs like
prefix '/:foo" => sub { get '/' => sub {...} };
as mentioned in #817.
In general, allowing a route regexp to be constructed by concatenating the prefixes and route paths gives amazing flexability. @xsawyerx++ for his work in #817 that allows for this!! However, there is one edge case when the prefix is undef (or the prefix is '/' as it is converted to an undef). The PSGI spec required the PATH_INFO header to be either empty, or begin with a '/'. Without special handling, a route like prefix '/' => sub { get 'x' => sub {...} }; is going to try and match a path of 'x', which is not valid. Instead, if the prefix is undef and the concatenated regexp does NOT start with a '/', the route constructor adds it. This implies prefix '/' => sub { get '' => sub {...} }; prefix '/' => sub { get '/' => sub {...} }; both match the path '/', which could cause some confusion, and is a little different to when the prefix is not undef, viz prefix '/foo' => sub { get '' => sub {...} }; prefix '/foo' => sub { get '/' => sub {...} }; which match '/foo' and '/foo/' respectively.
|
To get this discussion kicked off again, I've created Pr #845 with some extra tests and a possible approach to handling the case when the route prefix is undef but the concatenated route regexp does not begin with a |
Adds an explicit test for token handling for constructs like
prefix '/:foo" => sub { get '/' => sub {...} };
as mentioned in #817.
In general, allowing a route regexp to be constructed by concatenating the prefixes and route paths gives amazing flexability. @xsawyerx++ for his work in #817 that allows for this!! However, there is one edge case when the prefix is undef (or the prefix is '/' as it is converted to an undef). The PSGI spec required the PATH_INFO header to be either empty, or begin with a '/'. Without special handling, a route like prefix '/' => sub { get 'x' => sub {...} }; is going to try and match a path of 'x', which is not valid. Instead, if the prefix is undef and the concatenated regexp does NOT start with a '/', the route constructor adds it. This implies prefix '/' => sub { get '' => sub {...} }; prefix '/' => sub { get '/' => sub {...} }; both match the path '/', which could cause some confusion, and is a little different to when the prefix is not undef, viz prefix '/foo' => sub { get '' => sub {...} }; prefix '/foo' => sub { get '/' => sub {...} }; which match '/foo' and '/foo/' respectively.
[ BUG FIXES ]
* GH #868: Fix incorrect access name in $error->throw. (cdmalon)
* GH #879, #883: Fix version numbering in packaging and tests.
(Russell Jenkins)
* File serving (send_file) won't call serializer. (Russell Jenkins)
* GH #892, #510: Workaround for multiple plugins with hooks.
(Russell Jenkins, Alberto Simões)
* GH #558: Remove "prefix" inconsistency with possibly missing postfixed
forward slash. (Sawyer X)
[ DOCUMENTATION ]
* GH #816, #874 Document session engine changes in migration documentation.
(Chenchen Zhao)
* GH #866, #870: Clarify that you cannot forward to a static file, why,
and two different ways of accomplishing it without forward.
(Sakshee Vijayvargia)
* GH #878: Rework example for optional named matching due to operator
precedence. (Andrew Solomon)
* GH #844: Document Simple session backend is the default. (Sawyer X)
[ ENHANCEMENT ]
* GH #869: Streaming file serving (send_file). (Russell Jenkins)
* GH #793: "prefix" now supports the path definition spec. (Sawyer X)
* GH #817, #845: Route spec under a prefix doesn't need to start with
a slash (but must without a prefix).
(Sawyer X, Russell Jenkins)
* GH #871: Use Safe.pm instead of eval with Dancer2::Serializer::Dumper.
(David Zurborg)
* GH #880: Reduce and cleanup different logging calls in order to handle
the stack frames traceback for logging classes. (Russell Jenkins)
* GH #857, #875: When failing to render in Template::Toolkit, make the
error reflect it's a TT error, not an internal one.
(valerycodes)
|
++ everyone! |
I couldn't break this down into multiple commits so instead a
single commit which fixes two issues. They are both tricky to
grasp and one might introduce breakage (while reducing breakage,
I guess), so they're important to understand.
The first relates to allowing variables in prefixes with a single
path request. If your route is simply '/', variables in the prefix
are simply ignored:
Now the actual route registered is '/:board/', literally. That
means that it won't accept variables but only '/:board/'.
The second issue is accepting anything that does not start with
a leading forward slash. This would be an error:
If you have a prefix and you want to still grab the basic path
without the ending slash it would fail:
This is now fixed too.
One thing to research and perhaps write a test for is the behavior
using path() or path_info(). path() is defined by
path_info() || '/', which might be a problem.
Another issue to recognize is that if someone had the following
code and we don't cover for it, they will receive a 404:
This, if successful, closes GH #558 and possibly GH #793.