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

Improve support for "has" in Dancer2::Plugin #1292

Closed

Conversation

nfg
Copy link
Contributor

@nfg nfg commented Nov 23, 2016

Hello! I was writing a Dancer2 plugin for $WORK, and couldn't figure out why my attributes weren't pulling their values from the plugin configs. Eventually I figured out it was because I was defining multiple attributes at once in my plugin, a la:

#!/usr/bin/env perl

package PluginTest;
use Dancer2::Plugin;
has [qw(field1)]    => ( is => 'ro', from_config => 1 ); # THIS BREAKS!
has 'field2'        => ( is => 'ro', from_config => 1 ); # This works

plugin_keywords 'test_fields' => sub {
    my $self = shift;
    foreach my $field (qw(field1 field2)) {
        print STDERR "$field:" . ($self->$field // '(undef)') . "\n";
    }
};

package main;
use Dancer2;
PluginTest->import();

set plugins => {
    "PluginTest" => {
        field1 => 'woo',
        field2 => 'hoo',
    },
};

get '/' => sub {
    test_fields();
    return "TEST";
};
dance;

When I run that script and hit localhost:3000/ , I get:

➜  Foo perl server.pl 
>> Dancer2 v0.204001 server 14578 listening on http://0.0.0.0:3000
[main:14578] core @2016-11-22 17:49:30> looking for get / in /home/nfg/.plenv/versions/5.20.3/lib/perl5/site_perl/5.20.3/Dancer2/Core/App.pm l. 34
[main:14578] core @2016-11-22 17:49:30> Entering hook core.app.before_request in (eval 115) l. 1
field1:(undef)
field2:hoo
[main:14578] core @2016-11-22 17:49:30> Entering hook core.app.after_request in (eval 115) l. 1

This PR should fix the issue.

This should work now:

    has [qw(field1 field2)] => (
      is => 'ro',
      from_config => 1,
      plugin_keyword => 1,
    );
@nfg
Copy link
Contributor Author

nfg commented Nov 23, 2016

FWIW, there's weirdness with from_config and plugin_keyword with non-1 values. IE:

has [qw(foo bar)] => (
  is => 'ro',
  from_config => 'foo',
  plugin_keyword => [qw(foo bar)],
);

Both "foo" and "bar" will use the key "foo" for their default value, which is probably OK... but the keywords "foo" and "bar" will both use the "bar" attribute, which is unintuitive. I was thinking of maybe adding a warning to Dancer2::Plugin::p2_has_keyword if $class->keywords->{$} already exists, or a warning in the documentation. What do you guys think?

@xsawyerx xsawyerx added the Bug label Nov 23, 2016
@xsawyerx
Copy link
Member

👍

@nfg
Copy link
Contributor Author

nfg commented Nov 30, 2016

Hello again! Just pinging. Is there anything else you'd like me to do, or is this good?

@xsawyerx
Copy link
Member

No. It's good. We just need a bit of time. The Advent Calendar is starting tomorrow so we've been pressed for time.

@xsawyerx
Copy link
Member

xsawyerx commented Dec 5, 2016

As for your question, I think we should not allow defining two attributes at the same time with two plugin keywords attribute.

There can be different conditions checked here, but I think the simplest one would be "If you're defining an attribute with an array, you cannot provide a plugin_keyword that is an array."

@nfg nfg force-pushed the feature/p2_has_with_multiple_attributes branch from 3344b59 to d33d7d4 Compare December 5, 2016 21:49
@nfg
Copy link
Contributor Author

nfg commented Dec 5, 2016

Cool! I've added that check and a quick unit test (shoe-horned into from-config.t).

@veryrusty
Copy link
Member

Looks good to me 👍 Sorry its taken soooooo long to look over it :(

@veryrusty
Copy link
Member

Cherry-picked onto current master and merged. @nfg++

Apologies again for it taking soooo long.

@veryrusty veryrusty closed this Apr 19, 2018
@nfg
Copy link
Contributor Author

nfg commented Apr 19, 2018

No worries. Thanks, eh!

cromedome added a commit that referenced this pull request Apr 20, 2018
    [ BUG FIXES ]
    * GH #1090, #1406: Replace HTTP::Body with HTTP::Entity::Parser in
      Dancer2::Core::Request. (Russell @veryrusty Jenkins)
    * GH #1292: Fix multiple attribute definitions within Plugins
      (Nigel Gregoire)
    * GH #1304: Fix the order by which config files are loaded, independently
      of their filename extension (Alberto Simões, Russell @veryrusty Jenkins)
    * GH #1400: Fix infinite recursion with exceptions that use circular
      references. (Andre Walker)
    * GH #1430: Fix `dancer2 gen` from source directory when Dancer2 not
      installed. (Tina @perlpunk Müller - Tina)
    * GH #1434: Add `validate_id` method to verify a session id before
      requesting the session engine fetch it from its data store.
      (Russell @veryrusty Jenkins)
    * GH #1435, #1438: Allow XS crush_cookie methods to return an arrayref
      of values. (Russell @veryrusty Jenkins)
    * GH #1443: Update copyright year (Joseph Frazer)
    * GH #1445: Use latest HTTP::Headers::Fast (Russell @veryrusty Jenkins)
    * PR #1447: Fix missing build requires (Mohammad S Anwar)

    [ ENHANCEMENTS ]
    * PR #1354: TemplateToolkit template engine will log (at debug level)
      if a template is not found. (Kiel R Stirling, Russell @veryrusty Jenkins)
    * GH #1432: Support Content-Disposition of inline in
      send_file() (Dave Webb)
    * PR #1433: Verbose testing in AppVeyor (Graham Knop)

    [ DOCUMENTATION ]
    * GH #1314: Documentation tweaks (David Precious)
    * GH #1317: Document serializer configuration (sdeseille)
    * GH #1386: Add Hello World example (Gabor Szabo)
    * PR #1408: List project development resources (Steve Dondley)
    * PR #1426: Move performance improvement information from Migration guide
      to Deployment (Pedro Melo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants