[Catalyst] Catalyst::Controller bug when parsing attributes (with patch)

[Seth Daniel]

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. 

If 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:

--- 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. 

I can probably provide a program that demonstrates this, but I've found
that writing a 'simple' Catalyst application requires a lot of setup. :o/

Catalyst::Runtime 5.80031
Catalyst::Controller::ActionRole 0.15

-- 
seth /\ sethdaniel.org