Last active
March 27, 2018 20:59
-
-
Save KES777/0eec79515ef1c2c5c200ca902c9915ec to your computer and use it in GitHub Desktop.
proper descrition of current behaviour
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DESCRIPTION OF THE PROBLEM | |
use Mojolicious::Lite; | |
use Test::Mojo; | |
use Test::More; | |
get '/test' => { format => 'xxx' }; | |
my $t = Test::Mojo->new; | |
$t->get_ok('/test')->content_is( "xxx\n" ); | |
$t->get_ok('/test.json')->content_is( "json\n" ); | |
$t->get_ok('/test?format=json')->content_is( "json\n" ); | |
done_testing(); | |
__DATA__ | |
@@ test.xxx.ep | |
xxx | |
@@ test.json.ep | |
json | |
$ perl basic.t | |
ok 1 - GET /test | |
ok 2 - exact match for content | |
ok 3 - GET /test.json | |
ok 4 - exact match for content | |
ok 5 - GET /test?format=json | |
not ok 6 - exact match for content | |
# Failed test 'exact match for content' | |
# at basic.t line 11. | |
# got: 'xxx | |
# ' | |
# expected: 'json | |
# ' | |
1..6 | |
# Looks like you failed 1 test of 6. | |
As you can see default stash value for {format} take precedence over format | |
query parameter. | |
DESCRIPTION WHY THIS HAPPEN | |
Currently while route matching | |
1. For request with URL path suffix the default {format} stash value is replaced by "placeholder": | |
https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Routes/Pattern.pm#L29 | |
2. For request without URL path suffix the default {format} stash value **stay on top** of captures | |
because captures 'format' is `undef`: | |
https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Routes/Pattern.pm#L32 | |
Actually here when user requests `undef` format on url path suffix. This `undef` MUST replace default {format} | |
And because this replacement was not done we are fooled here: | |
https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Renderer.pm#L30 | |
about that $c->stash->{format} is requested by user (but, as we have seen, that is default {format} stash value) | |
I. PROPOSITION HOW TO FIX | |
To prevent that and to keep current behaviour I just split 'default' and 'captured' | |
https://github.com/KES777/mojo/pull/36/files#diff-2650135fc08c671d8b875d9b8480f498 | |
default value I put into `_sformat` ( "s" stands for server ) | |
captured value I put into `_cformat` ( "c" stands for client ) | |
When this is done I do content negotiation `around_action` and `before_render` (for actionless route) | |
https://github.com/KES777/mojo/pull/36/files#diff-08d2a3a980946f364c7bf445ea6d4a2cR10 | |
The issue stays for case when exception occur while dispatching: | |
https://github.com/kraih/mojo/blob/master/t/mojolicious/exception_lite_app.t#L176 | |
https://github.com/kraih/mojo/blob/master/t/mojolicious/exception_lite_app.t#L104 | |
because {format} is assigned before `before_render` and `around_action` event occour: | |
https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Plugin/DefaultHelpers.pm#L110 | |
This can be fixed in two ways: | |
1. Add bare `around_action`: | |
https://github.com/KES777/mojo/pull/36/files#diff-fc5e1c91161f82eb6d5f8cfe15a8aba1R105 | |
2. Or delete `DefaultHelpers.pm#L110` line at all. This is safe because we do same later at renderer: | |
https://github.com/kraih/mojo/blob/master/lib/Mojolicious/Renderer.pm#L96 | |
Which would be better? | |
II. PROPOSITION TO IMPROVE FALLBACKS | |
Working on this patch the propositon was born. If we setup default {format} for application and/or | |
route to array this will allow better fallbacks for content negotiation when html is not the default: | |
https://github.com/KES777/mojo/pull/36/files#diff-08d2a3a980946f364c7bf445ea6d4a2cR44 | |
# /custom -> "json" | |
# /custom.html -> undef | |
# /custom.xml -> "xml" | |
# /custom.txt -> undef | |
$r->get( '/custom', { format => [qw( json, xml )] ); | |
Members of the core team. I ask you to include into Mojolicious source core only next change: | |
https://github.com/KES777/mojo/pull/36/files#diff-2650135fc08c671d8b875d9b8480f498 | |
Hope this description will be sufficent. | |
Thank you very much for your advices. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment