Bug #80298 for CGI-Application-Plugin-ActionDispatch: [PATCH] Better whitespace and quote handling in attribute parser

Fri Oct 19 20:58:35 2012 perl [...] dannywarren.com - Ticket created

Subject:

[PATCH] Better whitespace and quote handling in attribute parser

The attribute params parser in ActionDispatch::Attributes is pretty strict, and doesn't accept any variation from the syntax shown in the docs. This means no whitespace and no double quotes, both of which are fairly standard syntax choices. So, this works: sub foo : Path('/foo') {} But these do not: sub foo : Path("foo") {} sub foo : Path( '/foo' ) {} sub foo : Path( "/foo" ) {} When you stray from the expected syntax, the attribute is silently thrown out and you are left scratching your head as to why a match isn't being made. Attached is a patch for more flexible attribute parsing, shamelessly stolen from Catalyst's "_parse_attrs" method. @@ -20,8 +20,11 @@ foreach (@attrs) { # Parse the attribute string ex: Regex('^/foo/bar/(\d+)/'). - my($method, $params) = /^([a-z_]\w*)(?:[(](.*)[)])?$/is; - $params =~ s/(^'|'$)//g if defined $params; + my($method, $params) = /^(.*?)(?:$\s*(.+?)\s*$)?$/; + + if ( defined $params ) { + ( $params =~ s/^'(.*)'$/$1/ ) || ( $params =~ s/^"(.*)"/$1/ ) + } I've also attatched new tests to verify the above patch.

Fri Oct 19 21:02:10 2012 perl [...] dannywarren.com - Correspondence added

On Fri Oct 19 20:58:35 2012, DWARREN wrote: Show quoted text

> Attached is a patch for more flexible attribute parsing, shamelessly > stolen from Catalyst's "_parse_attrs" method. > > I've also attatched new tests to verify the above patch.

Odd, it didn't actually attach the files. Trying again.

Subject:

attribute_parsing_fix.patch

--- lib/CGI/Application/Plugin/ActionDispatch/Attributes.pm.0.98 2010-06-02 07:36:43.000000000 -0700 +++ lib/CGI/Application/Plugin/ActionDispatch/Attributes.pm 2012-10-19 16:47:58.000000000 -0700 @@ -20,8 +20,11 @@ foreach (@attrs) { # Parse the attribute string ex: Regex('^/foo/bar/(\d+)/'). - my($method, $params) = /^([a-z_]\w*)(?:[(](.*)[)])?$/is; - $params =~ s/(^'|'$)//g if defined $params; + my($method, $params) = /^(.*?)(?:$\s*(.+?)\s*$)?$/; + + if ( defined $params ) { + ( $params =~ s/^'(.*)'$/$1/ ) || ( $params =~ s/^"(.*)"/$1/ ) + } # Attribute definition. if($method eq 'ATTR') {

Subject:

TestAppAttributeParsing.pm

package TestAppAttributeParsing; use base 'CGI::Application'; use CGI::Application::Plugin::ActionDispatch; @TestApp::ISA = qw(CGI::Application); sub single_quotes_space : Path( '/single_quotes_space' ) { my $self = shift; return "Runmode: single_quotes_space\n"; } sub double_quotes_space : Path( "/double_quotes_space" ) { my $self = shift; return "Runmode: double_quotes_space\n"; } sub single_quotes_tab : Path( '/single_quotes_tab' ) { my $self = shift; return "Runmode: single_quotes_tab\n"; } sub double_quotes_tab : Path( "/double_quotes_tab" ) { my $self = shift; return "Runmode: double_quotes_tab\n"; } sub single_quotes_tab_space : Path( '/single_quotes_tab_space' ) { my $self = shift; return "Runmode: single_quotes_tab_space\n"; } sub double_quotes_tab_space : Path( "/double_quotes_tab_space" ) { my $self = shift; return "Runmode: double_quotes_tab_space\n"; } sub failed : Default { my $self = shift; return "Failed: fell back to default runmode\n"; }

Subject:

06_attribute_parsing.t

use Test::More tests => 7; use strict; use lib 't/'; BEGIN { use_ok('CGI::Application'); }; use TestAppAttributeParsing; use CGI; $ENV{CGI_APP_RETURN_ONLY} = 1; my @runmodes = qw| single_quotes_space double_quotes_space single_quotes_tab double_quotes_tab single_quotes_tab_space double_quotes_tab_space |; foreach my $runmode ( @runmodes ) { local $ENV{PATH_INFO} = '/' . $runmode; my $app = TestAppAttributeParsing->new(); my $output = $app->run(); my $expected_output = 'Runmode: ' . $runmode; like($output, qr/$expected_output/); }

Fri Oct 19 21:02:12 2012 perl [...] dannywarren.com - Status changed from 'new' to 'open'

Fri Oct 19 21:12:55 2012 perl [...] dannywarren.com - Correspondence added

While I'm here, thank you for ActionDispatch. It's the first thing I use in every CGI::Application I write. As an aside, I feel like the attribute value parsing stuff should come free with "use attributes", since this style of parameter passing is so common with attributes. There are probably deep philosophical or voodoo reasons as to why it doesn't though. Or it's already out there and I keep missing it - but all the other popular modules that use this type of attribute value syntax are still rolling their own parser, so there is probably a reason for it.

Mon Oct 22 12:34:59 2012 jaywhy [...] gmail.com - Correspondence added

Thanks for the great patch and the note that you appreciate ActionDispatch. I've applied your patch and it is already up on the github repo. I'll push version 0.99 to CPAN today. Thanks again. On Fri Oct 19 21:12:55 2012, DWARREN wrote: Show quoted text

> While I'm here, thank you for ActionDispatch. It's the first thing I use > in every CGI::Application I write. > > As an aside, I feel like the attribute value parsing stuff should come > free with "use attributes", since this style of parameter passing is so > common with attributes. > > There are probably deep philosophical or voodoo reasons as to why it > doesn't though. Or it's already out there and I keep missing it - but > all the other popular modules that use this type of attribute value > syntax are still rolling their own parser, so there is probably a reason > for it.

Mon Oct 22 12:35:23 2012 jaywhy [...] gmail.com - Status changed from 'open' to 'patched'

Tue Oct 23 03:55:37 2012 perl [...] dannywarren.com - Correspondence added

On Mon Oct 22 12:34:59 2012, JAYWHY wrote: Show quoted text

> Thanks for the great patch and the note that you appreciate > ActionDispatch. > > I've applied your patch and it is already up on the github repo. I'll > push version 0.99 to CPAN > today.

I just pulled 0.99 from CPAN and went crazy with the attribute spacing on one of my test apps to verify it. It's working great, thanks for patching this in so quickly! (Marking as resolved, if you prefer it stays as patched I won't be offended)

Tue Oct 23 03:55:38 2012 The RT System itself - Status changed from 'patched' to 'open'

Tue Oct 23 03:55:38 2012 perl [...] dannywarren.com - Status changed from 'open' to 'resolved'