| Subject: | Catalyst::Controller bug when parsing attributes (with patch) |
| Date: | Fri, 4 Feb 2011 14:28:09 -0800 |
| To: | bug-Catalyst-Runtime [...] rt.cpan.org |
| From: | Seth Daniel <catalyst [...] sethdaniel.org> |
Hi,
I believe I've discovered a bug in Catalyst::Controller. I triggered it
while using Catalyst::Controller::ActionRole so my examples below will
refer to that module.
perl 5.10.1
Catalyst::Runtime 5.80031
Catalyst::Controller::ActionRole 0.15
I have a Catalyst controller with a configuration like the following:
__PACKAGE->config(
action => {
some_action => { Does => [ '+Some:ActionRole' ] }
}
);
This will break *if* the Action module that processes the 'Does' attribute
changes the value that is passed to it. For a real world example of this see
Catalyst::Controller::ActionRole which changes the value passed to its
_parse_Does_attr method.
The _parse_attrs method in Catalyst::Controller does the following:
[...]
foreach my $key (keys %raw_attributes) {
my $raw = $raw_attributes{$key};
foreach my $value (ref($raw) eq 'ARRAY' ? @$raw : $raw) {
my $meth = "_parse_${key}_attr";
if ( my $code = $self->can($meth) ) {
( $key, $value ) = $self->$code( $c, $name, $value );
}
push( @{ $final_attributes{$key} }, $value );
}
}
[...]
The problem lies in the inner foreach. When $row is an array reference (and
only when $row is an array reference) $value will be an *alias* to each element
of that array. So if $value is changed within the attribute parsing method
this means that the next time this attribute parsing method is called the
$value passed to it will be the changed value...but this only occurs when
using a configuration like what I pasted above because the actual configuration
hash gets changed. I don't recall which perl introduced aliasing, but I use
perl 5.10.1.
Here is a simple patch which resolves the issue. This is merely meant
to be illustrative:
--- Controller.pm 2010-07-28 15:03:32.000000000 -0700
+++ Controller.pm.bak 2011-02-02 16:55:47.000000000 -0800
@@ -322,11 +322,12 @@
foreach my $value (ref($raw) eq 'ARRAY' ? @$raw : $raw) {
+ my $t_val = $value;
my $meth = "_parse_${key}_attr";
if ( my $code = $self->can($meth) ) {
- ( $key, $value ) = $self->$code( $c, $name, $value );
+ ( $key, $t_val ) = $self->$code( $c, $name, $t_val );
}
- push( @{ $final_attributes{$key} }, $value );
+ push( @{ $final_attributes{$key} }, $t_val );
}
}
A workaround, at least when using Catalyst::Controller::ActionRole, is
to not use the array ref syntax:
__PACKAGE->config(
action => {
some_action => { Does => '+Some:ActionRole' }
}
);
Of course this workaround isn't useful if you want to apply multiple roles to
an action.
There are three conditions I have identified that trigger this bug:
1) There is an attribute parser that changes the passed in $value (which
Catalyst::Controller::ActionRole does if you use the '+' or '~'
syntax).
2) An ActionRole is assigned to some action via the configuration.
3) The controller that contains the modified action is subclassed *more
than once*. The reason for this is that the first time the action is
applied everything seems fine, however the $value was changed and
this consquently changes the configuration. The second time it is
applied the '+' or '~' will be missing and the application of the
role will fail. The test program I provide below illustrates this.
http://doubleplusgood.org/ccbug.tar
The above link is as simple a program as I could make to exhibit the
problem. Mostly I just whittled down the
Catalyst::Controller::ActionRole tests to as few files as I could and
changed a few things.
Unpack the tar file and cd to the 't' dir.
Run 'prove execution-via-config.t'. It should fail with the bug I
reported.
You can get the test to pass if you either patch Catalyst::Controller
with the patch I proved above or simply change
t/lib/TestApp/Controller/ExecutionViaConfig.pm to apply the '+Moo' role
like so:
__PACKAGE__->config(
action => {
three => { Does => '+Moo' }
}
);
Thank you.
--
seth /\ sethdaniel.org