Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dancer2 0.200003] Weird behaviour with Plugin Inclusion #1216

Closed
pierre-vigier opened this issue Jul 18, 2016 · 12 comments
Closed

[Dancer2 0.200003] Weird behaviour with Plugin Inclusion #1216

pierre-vigier opened this issue Jul 18, 2016 · 12 comments

Comments

@pierre-vigier
Copy link
Contributor

Hi,

i got an app that was working on Dancer 0.166001 and is breaking with 0.200003 .
Here is a trimmed down version of the problem. Once including Dancer2::Plugin::Database , i get a Can't locate object method "dsl" via package "DemoApp" error message

  • bin/app.psgi
#!/usr/bin/env perl
use strict;
use warnings;
use lib 'lib';
use DemoApp;

return DemoApp->to_app;

*lib/DemoApp.pm

package DemoApp;
use DemoApp::Extension;
use Dancer2;

get '/' => sub { return 'root' };

true;

*lib/DemoApp/Extension.pm

package DemoApp::Extension;
use Dancer2 appname => 'DemoApp';
use Dancer2::Plugin::Database;

get '/extension' => sub { 'extension'; };

That code runs on Dancer2 0.166001, but not on 0.200003.

Fix is pretty simple, in my code, if i reverse in lib/DemoApp.pm the inclusion of Dancer2 and DemoApp::Extension, it is then working as expected:

package DemoApp;
use Dancer2;
use DemoApp::Extension;

instead of

package DemoApp;
use DemoApp::Extension;
use Dancer2;

It might be worth a hint in documentation for people upgrading from a version below 0.200001 to 0.200001 and above.

The code is working properly in 0.200001 and above if in my case the DemoApp::Extension package do not include plugin Dancer2::Plugin::Database .
I can't tell if it's a bug of Dancer2 or of my code, but it's a breaking change. That's happening with other plugin as well, like Dancer2::Plugin::Email.

@xsawyerx
Copy link
Member

What version of the plugins are you using?

@pierre-vigier
Copy link
Contributor Author

Should be the last ones,
Dancer2::Plugin::Database => 2.16
Dancer2::Plugin::Email => 0.003

> perl -I local/lib/perl5 -MDancer2::Plugin::Database -E 'say $Dancer2::Plugin::Database::VERSION;'
2.16
>perl -I local/lib/perl5 -MDancer2::Plugin::Email -E 'say $Dancer2::Plugin::Email::VERSION;'
0.0003

@xsawyerx
Copy link
Member

Preliminary debugging shows that the problem is due to the order here:

  • You load DemoApp
  • This first loads another class: DemoApp::Extension.
  • It registers an app called DemoApp.
  • It then loads a plugin which will try to use the dsl keyword in DemoApp namespace.

Because DemoApp itself hasn't run use Dancer2 yet, it will not have that DSL keyword (or any other DSL keywords) available yet. They will only get used when you call use Dancer2 inside that class.

You're basically calling a plugin that tries to use keywords for a class that did not import them yet.

@xsawyerx
Copy link
Member

This might work if we shift the call for keywords on the calling class rather than on the app's name as class, but I'm not sure if we can reach it. caller is probably not reliable once you have plugins within plugins. We can use the old trick of trying to find dsl in it. I'm not sure that's correct design though. (Do we allow someone to define "dsl" somewhere in the hierarchy without it being a Dancer2 app?)

@pierre-vigier
Copy link
Contributor Author

I understood the issue, and i already fixed it in my code.
My only concern being that it worked before, i guess an hint in the documentation could be enough, as it makes perfect sense. That's why i am thinking it should probably not considered as a bug. Maybe, if possible, an hint would be enough

@xsawyerx
Copy link
Member

I'm trying to see whether we can really fix it, as in you wouldn't need to change any order. :)

@xsawyerx
Copy link
Member

The following patch in Dancer2::Plugin will sort this out, no matter what your order is:

-                foreach my $keyword ( keys %{$_[0]->app->name->dsl->dsl_keywords} ) {
+                my $app_dsl_cb;
+                ## no critic qw(ControlStructures::ProhibitCStyleForLoops)
+                for ( my $i = 0; my $caller = caller($i); $i++ ) {
+                    $app_dsl_cb = $caller->can('dsl')
+                        and last;
+                }
+
+                $app_dsl_cb
+                    or croak('Could not find Dancer2 app');
+
+                foreach my $keyword ( keys %{ $app_dsl_cb->()->dsl_keywords} ) {

I'll prepare a pull request for this.

@pierre-vigier
Copy link
Contributor Author

Nice!

@xsawyerx
Copy link
Member

Thank you for raising this issue. :)

xsawyerx added a commit that referenced this issue Jul 18, 2016
The problem in GH #1216 is when a user loads a plugin which tries
to access a DSL, while only loading "Dancer2" (and thus instating
the DSL) comes after.

In that case, the DSL in the app class does not exist yet, and
thus fails.
@xsawyerx
Copy link
Member

Pull request created. :)

xsawyerx added a commit that referenced this issue Jul 21, 2016
The problem in GH #1216 is when a user loads a plugin which tries
to access a DSL, while only loading "Dancer2" (and thus instating
the DSL) comes after.

In that case, the DSL in the app class does not exist yet, and
thus fails.
xsawyerx added a commit that referenced this issue Jul 21, 2016
@xsawyerx
Copy link
Member

Merged and will be released soon. Thanks for reporting the issue.

@pierre-vigier
Copy link
Contributor Author

Thanks for the fix

cromedome added a commit that referenced this issue Jul 22, 2016
    [ BUG FIXES ]
    * GH #1216: Make DSL work in edge-case of plugins calling DSL before the
      app class loaded Dancer2. (Sawyer X)
    * GH #1210: Show proper module/line number in log output (Masaaki Saito)

    [ ENHANCEMENTS ]
    * GH #900: Switch from to_json to encode/encode_json (Nuno Ramos Carvalho)
    * GH #1196: Move serializer from JSON to JSON::MaybeXS (Nuno Ramos Carvalho)
    * GH #1215: Remove unused DANCER2_SHARE_DIR env variable (Jason A. Crome)

    [ DOCUMENTATION ]
    * PR #1213: Clarify params merging docs and related examples
      (Daniel Perrett)
    * Add Peter Mottram (@SysPete) to list of core developers. (Russell Jenkins)
    * PR #1208: Introduce appdir before it's used; simplify description of what
      a view is (James E Keenan)
    * GH #1218: By request, remove David Golden from list of core developers.
      Created "emeritus" section to honor the contributions of former core
      developers. Thanks, xdg!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants