Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Before hook check moved out of routes matching in dispatcher #464

Open
wants to merge 4 commits into from

3 participants

@cym0n

Run the test in the branch: t/hook_changing_path.t
It will go well.
Now take it in a different directory and run it again (obviously still with Dancer2 in your @INC). It will fail.

It's a self contained test, why should it fail?

Problem is that the dispatcher runs before hooks during the route matching and the before hook we introduced change routes, modifying something we didn't configure (/prefix/configured) in something we did (/configured).

When the script is in the t directory this is not a problem because the Dancer2::Handler::File appends a megasplat route to the app's routes and the before hook can be triggered when testing /prefix/configured against it.
In a directory different from t, Dancer2 environment can't determinate a public directory (under t we have a public...) so the Dancer2::Handler::File can't register itself to the app and the megasplat can't come saving us.

I tried to make the before hook triggered before the route matching, but I'm not sure this is the right thing to do. It makes sense for you?

This is still very dirty code. I left all the old halted context management inside the loop, just copying it also beofre it. But I want your feedback about the question before refactoring.

Problem I exposed has bad side effects in real world. I have a plugin that fails dzil test for that. A good workaround is creating a public directory under the t, but it's a bit dirty...

@veryrusty
Owner

One side effect of moving the call to the before hook prior to route matching is that route params will no longer be available to before hooks.

@cym0n - could you elaborate on what your plugin does and/or why you need to modify the request like you have in the test case in the pull request?

@cym0n

It's a multilang plugin. Here's the code:
https://github.com/cym0n/Dancer2-Plugin-Multilang
I designed it to have a transparent management of the language routes (/it/, /en/...).

As i said, I was afraid that a modification like this could have side-effect :-)
Problem is that, at this time, every before hook that try to modify path works only for a side effect of a feature completely not related (File Handler).

I was thinking about a new status, different from halted, that could be configured in the Context to manage situations like this.
This way the developer could decide if he needs the path-parsing or if he's changing the path itself and he can trigger the hook before it.

What do you think about a solution like this?

@cym0n

One side effect of moving the call to the before hook prior to route matching is that route params will no longer be available to before hooks.

I added to the branch a test to keep an eye on this.

@cym0n

New solution for the problem.
Thinkinkg about it, before hook works well as it is, also managing the route params and it has to be executed after route match to manage them.
So, for situations where you know you're going to rewrite the path and route params are not important I introduced a new hook, the before_match, working before the matching loop.

I'll wait for feedback on this solution before doing some refactoring and documentation.

@xsawyerx
Owner

Ping?

@cym0n

I suspended my work on this because at the time there was the big refactoring about routes, redirects and forward.
Anyway, my question is still there: do you think it's a good idea add an hook before route matching? In my opinion it could be used by plugin that modify paths...

@xsawyerx
Owner

If it's meant to change the path, people should use Plack::Middleware::Rewrite. This would basically be replacing that for every request inside Dancer. I'd really prefer not to do that.

@cym0n

My idea was to give the opportunity to develope plugins that can change App routing, using forward directive, keeping all the development inside Dancer2.
Consider that this is already possible for a side effect of the File Handler. The File Handler introduces a megasplat on the bottom of all the routes so route checking always return true and you can manipulate any route.
Introducing an hook as i considered is just a way to avoid route matching in a cleaner way.

@xsawyerx
Owner

While I see your point, I can't think of a single reason to do that other than reimplementing Plack::Middleware::Rewrite at the moment.

I think it would be useful to raise this again when there's an specific thing wished to be implemented. What do you think?

@cym0n

Actually my Dancer2::Plugin::Multilang use that behaviour, exploiting the File Handler megasplat.
I understand that you see this kind of rewrite work at an architectural level higher then inside the framework and you're right about that.
Allowing rewrite inside a plugin, however, allows developers to create solutions easy to install e ready to work out of the box. Someone could use them with no knowledge about the architecture under the hood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 15, 2013
  1. @cym0n
Commits on Sep 18, 2013
  1. @cym0n
Commits on Sep 28, 2013
  1. @cym0n

    before_match hook introduced

    cym0n authored
  2. @cym0n
This page is out of date. Refresh to see the latest.
View
1  lib/Dancer2/Core/App.pm
@@ -240,6 +240,7 @@ sub _init_for_context {
sub supported_hooks {
qw/
+ core.app.before_route_match
core.app.before_request
core.app.after_request
core.app.route_exception
View
24 lib/Dancer2/Core/Dispatcher.pm
@@ -51,6 +51,28 @@ sub dispatch {
$app->log( core => "looking for $http_method $path_info" );
+ $app->execute_hook( 'core.app.before_route_match', $context );
+ if ( $context->response->is_halted ) {
+ if ( ! $context->response->header('Content-type') ) {
+ if ( exists( $app->config->{content_type} ) ) {
+ $context->response->header(
+ 'Content-Type' => $app->config->{content_type} );
+ }
+ else {
+ $context->response->header(
+ 'Content-Type' => $self->default_content_type );
+ }
+ }
+ if ( ref $context->response->content eq 'Dancer2::Core::Response' ) {
+ $context->response = $context->response($context->response->content);
+ }
+ else {
+ $context->response->content( defined $context->response->content ? $context->response->content : '' );
+ $context->response->encode_content;
+ }
+ return $context->response;
+ }
+
ROUTE:
foreach my $route ( @{ $app->routes->{$http_method} } ) {
@@ -70,7 +92,7 @@ sub dispatch {
# next if $context->request->path_info ne $path_info
# || $context->request->method ne uc($http_method);
-
+
$app->execute_hook( 'core.app.before_request', $context );
my $response = $context->response;
View
1  lib/Dancer2/Core/Role/Hookable.pm
@@ -23,6 +23,7 @@ sub BUILD { }
# their own aliases for their own hooks
sub hook_aliases {
{ before => 'core.app.before_request',
+ before_match => 'core.app.before_route_match',
before_request => 'core.app.before_request',
after => 'core.app.after_request',
after_request => 'core.app.after_request',
View
47 t/before_reading_params.t
@@ -0,0 +1,47 @@
+use strict;
+use warnings;
+use Test::More;
+
+use Test::TCP;
+use LWP::UserAgent;
+
+
+Test::TCP::test_tcp(
+ client => sub {
+ my $port = shift;
+
+ #Enter with a language-equipped URL
+ my $ua = LWP::UserAgent->new;
+ $ua->cookie_jar({file => "cookies.txt"});
+ my $res = $ua->get("http://127.0.0.1:$port/readthis/some");
+ ok($res->is_success);
+ is($res->content, 'some');
+ },
+ server => sub {
+ my $port = shift;
+ use Dancer2;
+
+ hook before => sub {
+ my $context = shift;
+ my $p = $context->request->params->{string};
+ $context->vars->{test} = $p;
+ };
+
+ get '/readthis/:string' => sub {
+ return vars->{test};
+ };
+
+ set(show_errors => 1,
+ startup_info => 1,
+ public => '.',
+ environment => 'development',
+ port => $port,
+ logger => 'console',
+ log => 'debug',
+ );
+
+ Dancer2->runner->server->port($port);
+ start;
+ },
+);
+done_testing;
View
52 t/hook_changing_path.t
@@ -0,0 +1,52 @@
+use strict;
+use warnings;
+use Test::More;
+
+use Test::TCP;
+use LWP::UserAgent;
+
+
+Test::TCP::test_tcp(
+ client => sub {
+ my $port = shift;
+
+ #Enter with a language-equipped URL
+ my $ua = LWP::UserAgent->new;
+ $ua->cookie_jar({file => "cookies.txt"});
+ my $res = $ua->get("http://127.0.0.1:$port/prefix/configured");
+ ok($res->is_success);
+ is($res->content, 'prefix');
+ },
+ server => sub {
+ my $port = shift;
+ use Dancer2;
+
+ hook before_match => sub {
+ my $context = shift;
+ my $path = $context->request->path_info();
+ if($path =~ m/^\/prefix/)
+ {
+ $path =~ s/^\/prefix//;
+ $context->response( $context->request->forward($context, $path, { cut => 'prefix'}, undef));
+ $context->response->halt;
+ }
+ };
+
+ get '/configured' => sub {
+ return request->params->{'cut'};
+ };
+
+ set(show_errors => 1,
+ startup_info => 1,
+ public => '.',
+ environment => 'development',
+ port => $port,
+ logger => 'console',
+ log => 'debug',
+ );
+
+ Dancer2->runner->server->port($port);
+ start;
+ },
+);
+done_testing;
Something went wrong with that request. Please try again.