Skip Menu |
 

This queue is for tickets about the DBIx-Class CPAN distribution.

Report information
The Basics
Id: 36863
Status: resolved
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: grazz [...] pobox.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: (no value)
Fixed in: 0.08099_06



Subject: CDBICompat/AccessorMapping.pm
Download (untitled) / with headers
text/plain 761b
Hi! The attached patch fixes two problems with CDBICompat::AccessorMapping. 1) mk_group_accessors() isn't prepared to handle arguments in the [ $field, $name ] format - in our installation this caused breakage in the upgrade from 0.07006 to 0.0801. 2) The logic related to ro/wo accessors doesn't seem quite right. If my "customer" class defines accessor_name() but not mutator_name(); and if $customer->accessor_name transforms the field-name "customer_age" into the method-name "age"; then I should get a r/w accessor called age(). But in this case CDBICompat is creating a read-only accessor called age() and a write-only mutator called customer_age(). CDBI doesn't test for #2, so I included a standalone test to demonstrate the problem. Thanks, Steve
Subject: cdbi-accessors.t
Download cdbi-accessors.t
text/x-perl 1.2k
use strict; use Test::More; $| = 1; BEGIN { eval "use DBIx::Class::CDBICompat;"; if ($@) { plan (skip_all => 'Class::Trigger and DBIx::ContextualFetch required'); next; } eval "use DBD::SQLite"; plan $@ ? (skip_all => 'needs DBD::SQLite for testing') : (tests => 3); } INIT { use lib 't/testlib'; require Film; } sub Film::accessor_name { my ($class, $col) = @_; return "sheep" if $col =~ /NumExplodingSheep/i; return $col; } # According to CDBI --> # # mutator name will be the same as accessor name unless you override it. # # If both the accessor and mutator are to have the same method name, # (which will always be true unless you override mutator_name_for), a # read-write method is constructed for it. # # But CDBICompat's current behavior in this case is to create a read-only # accessor with the "overridden" name, and a write-only mutator with the # "original" name. my $film = Film->create({sheep => 1}); eval { $film->sheep(2) }; is $@, '', 'Generated accessor is read-write'; eval { $film->NumExplodingSheep }; like $@, qr/Can't locate object method/, 'Original column name not a method'; unlike $@, qr/cannot access the value/, '... and certainly not a write-only mutator'; $film->discard_changes;
Subject: cdbi-accessors.patch
--- lib/DBIx/Class/CDBICompat/AccessorMapping.pm.orig 2007-08-11 17:07:57.000000000 -0400 +++ lib/DBIx/Class/CDBICompat/AccessorMapping.pm 2008-06-18 16:01:03.000000000 -0400 @@ -9,19 +9,27 @@ unless ($class->can('accessor_name') || $class->can('mutator_name')) { return $class->next::method($group => @cols); } + + # Per the CDBI manpage: + # + # If you override the mutator name, then the accessor method will be + # enforced as read-only, and the mutator as write-only. + foreach my $col (@cols) { - my $ro_meth = ($class->can('accessor_name') - ? $class->accessor_name($col) - : $col); - my $wo_meth = ($class->can('mutator_name') - ? $class->mutator_name($col) - : $col); - #warn "$col $ro_meth $wo_meth"; - if ($ro_meth eq $wo_meth) { - $class->next::method($group => [ $ro_meth => $col ]); + my ($name, $field) = ref $col ? @$col : ($col, $col); + + my $accessor = $class->can('accessor_name') + ? $class->accessor_name($field) + : $name; + my $mutator = $class->can('mutator_name') + ? $class->mutator_name($field) + : $accessor; + + if ($accessor eq $mutator) { + $class->next::method($group => [ $accessor => $field ]); } else { - $class->mk_group_ro_accessors($group => [ $ro_meth => $col ]); - $class->mk_group_wo_accessors($group => [ $wo_meth => $col ]); + $class->mk_group_ro_accessors($group => [ $accessor => $field ]); + $class->mk_group_wo_accessors($group => [ $mutator => $field ]); } } }
Download (untitled) / with headers
text/plain 774b
On Fri Jun 20 10:50:17 2008, GRAZZ wrote: Show quoted text
> 2) The logic related to ro/wo accessors doesn't seem quite right. If my > "customer" class defines accessor_name() but not mutator_name(); and if > $customer->accessor_name transforms the field-name "customer_age" into > the method-name "age"; then I should get a r/w accessor called age(). > But in this case CDBICompat is creating a read-only accessor called > age() and a write-only mutator called customer_age(). > > CDBI doesn't test for #2, so I included a standalone test to demonstrate > the problem.
#2 was fixed in r4161. if ($ro_meth eq $wo_meth or # they're the same $wo_meth eq $col) # or only the accessor is custom And there's a test for it in t/cdbi-t/15-accessor.t Looking into #1.
Download (untitled) / with headers
text/plain 515b
On Fri Jun 20 10:50:17 2008, GRAZZ wrote: Show quoted text
> 1) mk_group_accessors() isn't prepared to handle arguments in the [ > $field, $name ] format - in our installation this caused breakage in the > upgrade from 0.07006 to 0.0801.
I'm not sure what to do about #1. Which takes precedence, the accessor named in the mk_group_accessors() call or the accessor named by accessor_named_for()? I'm inclined to go with the argument given to mk_group_accessors(), if you explicitly pass in an accessor name you probably mean it.
Download (untitled) / with headers
text/plain 610b
On Tue Nov 04 13:09:12 2008, MSCHWERN wrote: Show quoted text
> On Fri Jun 20 10:50:17 2008, GRAZZ wrote:
> > 1) mk_group_accessors() isn't prepared to handle arguments in the [ > > $field, $name ] format - in our installation this caused breakage in the > > upgrade from 0.07006 to 0.0801.
> > I'm not sure what to do about #1. Which takes precedence, the accessor > named in the mk_group_accessors() call or the accessor named by > accessor_named_for()? > > I'm inclined to go with the argument given to mk_group_accessors(), if > you explicitly pass in an accessor name you probably mean it.
Done and pushed to trunk.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.