Hooks for plugins not registered #510

Closed
e11it opened this Issue Nov 24, 2013 · 11 comments

Comments

Projects
None yet
5 participants

e11it commented Nov 24, 2013

It's possible to register hook only for last included plugin.
I have modified test to demonstrate this issue.

Please take a look: e11it/Dancer2@9ed2da3

Test fails with:

t/plugin_syntax.t ............ 1/? Invalid hook name `third_hook' at /Dancer2/Core/DSL.pm line 143.
# Child (hooks in plugins) exited without calling finalize()
#   Failed test 'hooks in plugins'
#   at /System/Library/Perl/5.16/Test/Builder.pm line 252.
# Tests were run but no plan was declared and done_testing() was not seen.

@racke racke added the Bug label Nov 29, 2014

Owner

racke commented Nov 29, 2014

Got another report on this issue by garo on IRC - it is reproducible.

This crashes:

use Dancer2::Plugin::Database;
use Dancer2::Plugin::Email;

with:

Invalid hook name `database_connected' at /home/racke/perl5/perlbrew/perls/perl-5.20.0/lib/site_perl/5.20.1/Dancer2/Core/DSL.pm line 155.
Compilation failed in require at ./bin/app.pl line 6.

It works if you do:

use Dancer2::Plugin::Email;
use Dancer2::Plugin::Database;

@xsawyerx xsawyerx added the Testing label Nov 29, 2014

Owner

xsawyerx commented Nov 29, 2014

More information we found out:

  • When using multiple plugins, multiple plugins are created in the app.
  • The first one is a single composed plugin from all the merged ones.
  • The rest are references of the first.
  • When it's alright, there are postponed hooks for the plugin.

Sigh

Owner

racke commented Nov 29, 2014

OK, let me try to explain how convoluted the plugin system currently is in Dancer2.

When we register a plugin, the package of the plugin is tacked as role to the dsl object of the app:

        Moo::Role->apply_roles_to_object( $caller->dsl, $plugin );
        $caller->dsl->export_symbols_to($caller);
        $caller->dsl->dancer_app->register_plugin( $caller->dsl );

So in our test we have two plugins t::lib::Hookee and t::lib::Empty.

After we imported the first plugin, the ref of the dsl object looks like:

Dancer2::Core::DSL__WITH__t::lib::Hookee

After we imported the first plugin, the ref of the dsl object looks like:

Dancer2::Core::DSL__WITH__t::lib::Hookee__WITH__t::lib::Empty

If we retrieve now the plugins from the App with the plugins method, we get
these ref values:

Dancer2::Core::DSL__WITH__t::lib::Hookee__WITH__t::lib::Empty
Dancer2::Core::DSL__WITH__t::lib::Hookee__WITH__t::lib::Empty

So this method returns the DSL object twice and not any plugin.

The reason why only the hooks of the last plugin are available is that
we query the DSL object twice about the hook aliases and it has
only the hooks of the second plugin.

Major brain pain!

👎

@jacqueslareau jacqueslareau referenced this issue in PerlDancer/Dancer2-Plugin-Auth-Extensible Dec 8, 2014

Closed

undefined subroutine database #4

veryrusty added a commit to veryrusty/Dancer2 that referenced this issue Mar 27, 2015

drmuey commented Apr 14, 2015

this looks like the cause of bigpresh/Dancer-Plugin-Database#62 and PerlDancer/Dancer2-Plugin-Auth-Tiny#4

If I move the Dancer2::Plugin::DataBase use() statement to the plugin to the last plugin loaded I get:

Hook 'plugin.database.database_connected' does not exist at …/5.16.0/Dancer2/Plugin/Database.pm line 46.

instead of

Hook 'database_connected' does not exist at …/5.16.0/Dancer2/Plugin/Database.pm line 45.

Of note Dancer2::Plugin::DataBase uses wraps the v1 Dancer::Plugin::DataBase for the heavy lifting, the all-v2 plugins seem to work no matter what

drmuey commented Apr 14, 2015

+1 this is stopping a major project I am doing in its tracks so I will paypal $100 to whoever fixes this (I am not familiar enough w/ Dancer2 to offer a patch at this time but am looking into it) ;)

Owner

veryrusty commented Apr 21, 2015

I've made some progress on this, enough for the test from @e11it to pass. See my comment in #889.

veryrusty added a commit that referenced this issue Apr 25, 2015

Add test case from #510
This is still broken. Need to get postponed hooks populated into plugins
(well, the DSL).
Owner

veryrusty commented Apr 25, 2015

There is a working implementation for review in #892. 👯

veryrusty added a commit that referenced this issue Apr 25, 2015

Add test case from #510
This is still broken. Need to get postponed hooks populated into plugins
(well, the DSL).
Owner

veryrusty commented Apr 26, 2015

Resolved by #892.

Thanks to @e11it for reporting the issue and thank you to everyones' research, poking and prodding to get the fix/workaround merged.

There is still "brain pain" for anyone working through the plugin code - though we should now have the time to think about the reimplementation :)

@veryrusty veryrusty closed this Apr 26, 2015

xsawyerx added a commit that referenced this issue Apr 26, 2015

v0.160000
    [ 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)

drmuey commented May 7, 2015

Unfortunetely, #510 is not resolved by #892 :( its behaving like it was in my previous post

Dancer2 v.160000 and Dancer2::Plugin::Database v 2.12 still give me:
Hook 'database_connected' does not exist at /Users/dmuey/perl5/perlbrew/perls/perl-5.16.0/lib/site_perl/5.16.0/Dancer2/Plugin/Database.pm line 45.

commenting out line 45, $dsl->execute_hook(@_);, (since I am not using the hooks) allows it to work but that is a limited temporary hack :)

drmuey commented May 7, 2015

will open new issue since I can't re-open this one

drmuey commented May 7, 2015

Done it is, #905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment