Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

splited application post sub doesn't call `before` hook #525

Closed
chunzi opened this Issue · 10 comments

5 participants

chunzi Steven Humphrey baynes Russell Jenkins Sawyer X
chunzi

package top;
use Dancer2;
use top::sub;
prefix undef;
hook before => sub { vars->{from} = 'top' };
get '/' => sub { vars->{from} };
post '/' => sub { vars->{from} };
true;

package top::sub;
use Dancer2;
prefix 'sub';
get '/' => sub { vars->{from} };
post '/' => sub { vars->{from} };
true;

Only the GET /sub returns 'top'
the POST /sub returns ''
that proves the post subroutine doesn't called hook before.

any comments?

Steven Humphrey
Owner

Normally neither sub routines would call the before hook. Before hooks in Dancer2 are app specific and the use Dancer2 line has created two different apps.
This is by design.
The fact that get /sub returns top is a bit odd

baynes

I have just spent a day chasing the same issue as reported by chunzi. We also had a before hook in the sub package and this does get called for both the post and get requests.

shumphrey's comment is a bit concerning - this indicates an intended change in hook behaviour from Dancer1 (something that is not clear from the documentation). This gives us a big problem - we want to spread our get and post route declarations across multiple files (it is a big application) but have a before hook (to handle authentication) that is called before all routes, whichever file they are declared in. Having to put this before hook in each file would a significant maintenance cost.

baynes

Some investigation indicated that this could be linked to the /** route handler referred to in bug #239 created in the top level package being triggered for all gets and thus triggering the get hook at the top level. At that point I got lost in the code and was not able to trace any further.

Also raised the lack of a global hook as bug #536

Russell Jenkins
Owner

Following the initial app from @chunzi, would an approach like

package top::sub;
use Dancer2 appname => 'top'
# DSL imported but all hooks/routes defined are added to the 'top' app
1;

be a suitable?

@baynes Provided you always specify the "appname" on import, you shouldn't need a global hook (as you only have one app).

Steven Humphrey
Owner

I like @veryrusty 's idea. It allows an easy mechanism for migrating a Dancer1 app to Dancer2 in the future.

The approach we take at the moment is to override the DSL,
use Dancer2 dsl => 'My::DSL',

our own DSL extends Dancer2::Core::DSL but adds new dsl keywords, and during the BUILD section, adds a hook to the app.

baynes

@veryrusty - appname => 'top' would do fine. However can I suggest we have this discussion on global hooks under bug #536 rather than confusing this one about the different hook behaviour for POST and GET.

Russell Jenkins veryrusty referenced this issue from a commit
Russell Jenkins veryrusty Add routes and hooks to an existing app on import.
Allows routes and hooks for larger apps to be split into multiple source
files, but collected together to form a single "app". Resolves #525.
bad7f2f
baynes

@veryrusty I see there are some commits of a fix for this. When is this going to make it into the released product? We could do with it ASAP. Otherwise we either have to take the edit as a local patch or use a plugin to set up the same route in every app. Our application is too large to manage in a single Perl package and we need hooks common to all routes.

[P.S. I also notice the example under "Using the prefix feature to split your application" in the Cookbook would break if one added hooks to it so needs to be corrected to show the setting of the app name.]

Steven Humphrey
Owner

@baynes I would strongly recommend the plugin approach regardless of whether this gets merged.

Russell Jenkins veryrusty closed this issue from a commit
Russell Jenkins veryrusty Add routes and hooks to an existing app on import.
Allows routes and hooks for larger apps to be split into multiple source
files, but collected together to form a single "app". Resolves #525.
4a91133
Russell Jenkins veryrusty closed this in 4a91133
Sawyer X
Owner

Fixed by recent merge to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.