Skip Menu |

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

Report information
The Basics
Id: 16154
Status: resolved
Priority: 0/
Queue: Class-DBI

People
Owner: Nobody in particular
Requestors: ask [...] develooper.com
wwalker [...] bybent.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



To: bugs-Class-DBI [...] rt.cpan.org
From: Ask Bjørn Hansen <ask [...] develooper.com>
Subject: new accessor modifier doesn't default to also changing to mutator
Date: Sun, 27 Nov 2005 13:29:12 -0800
The documentation implies that if you change only the accessor the mutator will default to the same name as the accessor. In 3.0.7 (I think), this broke. Below is a test case that fails with v3.0.12. The error I get is: "... cannot alter the value of 'numexplodingsheep' on objects of class 'Film' at t/15-accessor2.t line 38" If I add sub Film::mutator_name_for { my ($class, $col) = @_; return "sheep" if lc $col eq "numexplodingsheep"; return $col; } to the test case then it works, but in the past I didn't have to do that (and again, the documentation implies that I shouldn't :-) ) Thank you! :-D - ask use strict; use Test::More; BEGIN { eval "use DBD::SQLite"; plan $@ ? (skip_all => 'needs DBD::SQLite for testing') : (tests => 6); } INIT { local $SIG{__WARN__} = sub { like $_[0], qr/clashes with built-in method/, $_[0] }; use lib 't/testlib'; require Film; } sub Film::accessor_name_for { my ($class, $col) = @_; return "sheep" if lc $col eq "numexplodingsheep"; return $col; } my $data = { Title => 'Bad Taste', Director => 'Peter Jackson', Rating => 'R', }; my $bt; eval { my $data = $data; $data->{sheep} = 1; ok $bt = Film->insert($data), "Modified accessor - with accessor"; isa_ok $bt, "Film"; }; is $@, '', "No errors"; eval { ok $bt->sheep(2), 'Modified accessor, set'; ok $bt->update, 'Update'; }; is $@, '', "No errors";
Date: Mon, 28 Nov 2005 08:05:15 +0000
From: Tony Bowden <tony [...] kasei.com>
To: "ask [...] develooper.com via RT" <bug-Class-DBI [...] rt.cpan.org>
Subject: Re: [cpan #16154] new accessor modifier doesn't default to also changing to mutator
RT-Send-Cc:
On Sun, Nov 27, 2005 at 04:29:44PM -0500, ask@develooper.com via RT wrote: Show quoted text
> In 3.0.7 (I think), this broke. Below is a test case that fails > with v3.0.12. The error I get is: > "... cannot alter the value of 'numexplodingsheep' on objects of > class 'Film' at t/15-accessor2.t line 38"
Thanks for this. I'm rather surprised that there wasn't a test case that caught this. I don't suppose you've a patch drafted up too? I can't see a trivial fix at the moment, and I don't have a lot of time this week to work on this. Tony
From: ask [...] develooper.com
(I emailed a response but it didn't seem to come through here, so I'm putting it into the ticket via the web interface too now). On Nov 28, 2005, at 0:06 , Tony Bowden via RT wrote: Full context and any attached attachments can be found at: <URL: http://rt.cpan.org/NoAuth/Bug.html?id=16154 > On Sun, Nov 27, 2005 at 04:29:44PM -0500, ask@develooper.com via RT wrote: In 3.0.7 (I think), this broke. Below is a test case that fails with v3.0.12. The error I get is: "... cannot alter the value of 'numexplodingsheep' on objects of class 'Film' at t/15-accessor2.t line 38" Thanks for this. I'm rather surprised that there wasn't a test case that caught this. Me too. It took a good deal of head scratching to figure out that it was a Class::DBI bug. :-) I don't suppose you've a patch drafted up too? I can't see a trivial fix at the moment, and I don't have a lot of time this week to work on this. With an emphasis on _draft_, yes. It does pass all the tests though. The only thing I could come up with was having the default (accessor|mutator)_name_for methods return some extra noise and then use the noise to decide if the name was overridden. I changed the logic of setting up the %method hash a little to make it clearer what is going on. (patch attached and inline). - ask --- lib/Class/DBI.pm~ 2005-11-04 01:22:56.000000000 -0800 +++ lib/Class/DBI.pm 2005-11-28 00:53:15.000000000 -0800 @@ -329,11 +329,27 @@ sub _mk_column_accessors { my $class = shift; foreach my $col (@_) { - my %method = ( - _ro_ => $col->accessor($class->accessor_name_for($col)), - _wo_ => $col->mutator($class->mutator_name_for($col)), + + my @acc = $class->accessor_name_for($col); + my @mut = $class->mutator_name_for($col); + + my %method = (); + + if (($acc[0] eq $mut[0]) # if they are the same + or (!$acc[1] and $mut[1])) { # or only the accessor was customized + %method = ('_' => $acc[0]); # make the accessor the mutator too + $col->accessor($acc[0]); + $col->mutator($acc[0]); + } + else { + %method = ( + _ro_ => $acc[0], + _wo_ => $mut[0], ); - %method = ('_' => $method{_ro_}) if $method{_ro_} eq $method{_wo_}; + $col->accessor($acc[0]); + $col->mutator($mut[0]); + } + foreach my $type (keys %method) { my $name = $method{$type}; my $acc_type = "make${type}accessor"; @@ -356,12 +372,12 @@ sub accessor_name_for { my ($class, $column) = @_; - return $column->accessor; + return $column->accessor, 1; } sub mutator_name_for { my ($class, $column) = @_; - return $column->mutator; + return $column->mutator, 1; } sub autoupdate { -- http://askask.com/ - http://develooper.com/
--- lib/Class/DBI.pm~ 2005-11-04 01:22:56.000000000 -0800 +++ lib/Class/DBI.pm 2005-11-28 00:53:15.000000000 -0800 @@ -329,11 +329,27 @@ sub _mk_column_accessors { my $class = shift; foreach my $col (@_) { - my %method = ( - _ro_ => $col->accessor($class->accessor_name_for($col)), - _wo_ => $col->mutator($class->mutator_name_for($col)), + + my @acc = $class->accessor_name_for($col); + my @mut = $class->mutator_name_for($col); + + my %method = (); + + if (($acc[0] eq $mut[0]) # if they are the same + or (!$acc[1] and $mut[1])) { # or only the accessor was customized + %method = ('_' => $acc[0]); # make the accessor the mutator too + $col->accessor($acc[0]); + $col->mutator($acc[0]); + } + else { + %method = ( + _ro_ => $acc[0], + _wo_ => $mut[0], ); - %method = ('_' => $method{_ro_}) if $method{_ro_} eq $method{_wo_}; + $col->accessor($acc[0]); + $col->mutator($mut[0]); + } + foreach my $type (keys %method) { my $name = $method{$type}; my $acc_type = "make${type}accessor"; @@ -356,12 +372,12 @@ sub accessor_name_for { my ($class, $column) = @_; - return $column->accessor; + return $column->accessor, 1; } sub mutator_name_for { my ($class, $column) = @_; - return $column->mutator; + return $column->mutator, 1; } sub autoupdate {
From: ask [...] develooper.com
I was wondering if you'd had time to look at this and while wondering I thought of a nicer way to fix it. Patch against 3.0.12 below. --- lib/Class/DBI.pm 2005-11-04 01:22:56.000000000 -0800 +++ /Users/ask/DBI.pm 2005-12-06 05:18:18.000000000 -0800 @@ -329,11 +329,29 @@ sub _mk_column_accessors { my $class = shift; foreach my $col (@_) { - my %method = ( - _ro_ => $col->accessor($class->accessor_name_for($col)), - _wo_ => $col->mutator($class->mutator_name_for($col)), + + my $default_accessor = $col->accessor; + + my $acc = $class->accessor_name_for($col); + my $mut = $class->mutator_name_for($col); + + my %method = (); + + if (($acc eq $mut) # if they are the same + or ($mut eq $default_accessor)) { # or only the accessor was customized + %method = ('_' => $acc); # make the accessor the mutator too + $col->accessor($acc); + $col->mutator($acc); + } + else { + %method = ( + _ro_ => $acc, + _wo_ => $mut, ); - %method = ('_' => $method{_ro_}) if $method{_ro_} eq $method{_wo_}; + $col->accessor($acc); + $col->mutator($mut); + } + foreach my $type (keys %method) { my $name = $method{$type}; my $acc_type = "make${type}accessor";
Date: Thu, 08 Dec 2005 12:31:13 +0000
From: Tony Bowden <tony [...] tmtm.com>
To: bug-Class-DBI [...] rt.cpan.org
Subject: Re: [cpan #16154] new accessor modifier doesn't default to also changing to mutator
RT-Send-Cc:
Show quoted text
> > >I was wondering if you'd had time to look at this and while wondering I thought of a nicer >way to fix it. Patch against 3.0.12 below. > >
Thanks for this. I had been pondering the previous patch, but was troubled by it. This one does look better. Ideally I'd like to take this opportunity to also make the PK immutable, but I don't really want to hold things up for that. I'll give this some more thought over the weekend. Thanks, Tony
Subject: mutator/accessor for primary key column named 'id' broken in 3.x
Upgraded from 0.96 to 3.0.12. Now code that works on other servers (and on this one before upgrade and after revert) fails with: Can't locate object method "get_id" via package "gNumber::Person" gNumber::Person is inherited from gNumber::CDBI (our class) which inherits from Class::DBI. gNumber::CDBI causes accessor mutator names to be prefixed with get_ and set (code at bottom). gNumber::Person->columns( Primary => qw/id/ ); # Make the getters/setters with our naming convention sub accessor_name { my ($class, $column) = @_; return "get_" . $column; } # end accessor_name () #-------------------------------------------------------------------- sub mutator_name { my ($class, $column) = @_; return "set_" . $column; } # end mutator_name ()
[guest - Thu Dec 15 15:35:01 2005]: Show quoted text
> gNumber::CDBI causes accessor mutator names to be prefixed with get_ > and set (code at bottom).
At first glance I suspect this is the same problem as https://rt.cpan.org/Ticket/Display.html?id=16154 I'm still pondering the approach described there, but will probably just apply this and work out a better solution later. Can you try the patch there and see if that solves your problem, or whether yours is something else? Sorry for the trouble, Tony
[tony@tmtm.com - Thu Dec 8 07:30:18 2005]: Show quoted text
> > > > > >I was wondering if you'd had time to look at this and while wondering
> I thought of a nicer
> >way to fix it. Patch against 3.0.12 below. > > > >
> > Thanks for this. I had been pondering the previous patch, but was > troubled by it. This one does look better. > > Ideally I'd like to take this opportunity to also make the PK > immutable, > but I don't really want to hold things up for that. > > I'll give this some more thought over the weekend.
Any chance you could put out a release with the patch? The bug is breaking most of my applications and I'd really like to upgrade so I can start using ->insert etc in new applications. :-) I'll be happy to look into making the PK immutable (but probably like yourself I'm mostly out of time for the next few weeks) - ask
Date: Mon, 19 Dec 2005 13:33:05 +0000
From: Tony Bowden <tony [...] tmtm.com>
To: bug-Class-DBI [...] rt.cpan.org
Subject: Re: [cpan #16154] new accessor modifier doesn't default to also changing to mutator
RT-Send-Cc:
Guest via RT wrote: Show quoted text
> Any chance you could put out a release with the patch? The bug is breaking most of my > applications and I'd really like to upgrade so I can start using ->insert etc in new > applications. :-)
Nope. The patch makes your test case pass OK, but it causes a failure in t/15. I suspect it's a relatively simple fix, and if you can spot it I'll put out a release, but as you suspected I'm a bit short of time to find it myself. Tony
[tony@tmtm.com - Mon Dec 19 08:33:50 2005]: Show quoted text
> Guest via RT wrote:
> > Any chance you could put out a release with the patch? The bug is
> breaking most of my
> > applications and I'd really like to upgrade so I can start using
> ->insert etc in new
> > applications. :-)
> > Nope. The patch makes your test case pass OK, but it causes a failure > in > t/15. > > I suspect it's a relatively simple fix, and if you can spot it I'll > put > out a release, but as you suspected I'm a bit short of time to find it > myself.
I don't get a failure here. (t/15-accessor2 is the test case I attached to this ticket initially to keep the new separated from the existing tests). [ask@g5 ~/src/Class-DBI/Class-DBI-ask]$ make test PERL_DL_NONLAZY=1 /pkg/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'blib/ lib', 'blib/arch')" t/*.t t/01-columns.............ok t/02-Film................ok t/03-subclassing.........ok t/04-lazy................ok t/05-Query...............ok t/06-hasa................ok t/08-inheritcols.........ok t/09-has_many............ok t/10-mysql...............ok t/11-triggers............ok t/12-filter..............ok t/13-constraint..........ok t/14-might_have..........ok t/15-accessor............ok t/15-accessor2...........ok t/16-reserved............ok t/17-data_type...........skipped all skipped: needs DBD::Pg for testing t/18-has_a...............ok t/19-set_sql.............ok t/21-iterator............ok t/22-deflate_order.......ok t/23-cascade.............ok t/24-meta_info...........ok t/25-closures_in_meta....ok t/97-pod.................ok t/98-failure.............ok 1/7closing dbh with active statement handles t/98-failure.............ok t/99-misc................ok All tests successful, 1 test skipped. Files=27, Tests=636, 36 wallclock secs ( 4.77 cusr + 2.73 csys = 7.50 CPU)
[tony@tmtm.com - Thu Dec 8 07:30:18 2005]: Show quoted text
> Ideally I'd like to take this opportunity to also make the PK > immutable, > but I don't really want to hold things up for that.
#16808 has a patch for that. It could do a bit more, but I tried to keep it simple for now. Comments welcome. - ask
Date: Tue, 03 Jan 2006 14:00:00 +0000
From: Tony Bowden <tony [...] unite.net>
To: bug-Class-DBI [...] rt.cpan.org
Subject: Re: [cpan #16154] new accessor modifier doesn't default to also changing to mutator
RT-Send-Cc:
via RT wrote: Show quoted text
> I don't get a failure here. (t/15-accessor2 is the test case I attached to this ticket initially to > keep the new separated from the existing tests).
Still failing here. Can you send me a copy of the entire method, rather than just a patch, in case it applied strangely somehow? Thanks, Tony
On Tue Jan 03 09:00:45 2006, tony@unite.net wrote: Show quoted text
> via RT wrote:
> > I don't get a failure here. (t/15-accessor2 is the test case I
> attached to this ticket initially to
> > keep the new separated from the existing tests).
> > Still failing here. Can you send me a copy of the entire method, > rather > than just a patch, in case it applied strangely somehow?
I did that by email but it didn't make it here. Grrh. Here it is again: sub _mk_column_accessors { my $class = shift; foreach my $col (@_) { my $default_accessor = $col->accessor; my $acc = $class->accessor_name_for($col); my $mut = $class->mutator_name_for($col); my %method = (); if (($acc eq $mut) # if they are the same or ($mut eq $default_accessor)) { # or only the accessor was customized %method = ('_' => $acc); # make the accessor the mutator too $col->accessor($acc); $col->mutator($acc); } else { %method = ( _ro_ => $acc, _wo_ => $mut, ); $col->accessor($acc); $col->mutator($mut); } foreach my $type (keys %method) { my $name = $method{$type}; my $acc_type = "make${type}accessor"; my $accessor = $class->$acc_type($col->name_lc); $class->_make_method($_, $accessor) for ($name, "_${name}_accessor"); } } } - ask
On Tue Jan 03 09:00:45 2006, tony@unite.net wrote: Show quoted text
> via RT wrote:
> > I don't get a failure here. (t/15-accessor2 is the test case I
> attached to this ticket initially to
> > keep the new separated from the existing tests).
> > Still failing here. Can you send me a copy of the entire method, > rather
the patch works for me, once the formatting is sorted out (for which my browser is presumably to blame?). Applied to a fresh 3.0.13, all tests pass and everything works. Likewise the additional patch in #16808. .2p will
On Thu Jan 12 21:27:17 2006, ABH wrote: Show quoted text
> I did that by email but it didn't make it here. Grrh.
Thanks - I got the original as well, and made a new release from it, but something went wrong somewhere with actually releasing it. On its way to CPAN now. Tony
On Mon Jan 16 05:35:55 2006, TMTM wrote: Show quoted text
> On Thu Jan 12 21:27:17 2006, ABH wrote:
> > I did that by email but it didn't make it here. Grrh.
> > Thanks - I got the original as well, and made a new release from it, but > something went wrong somewhere with actually releasing it. On its way to > CPAN now.
Great, thank you! :-) - ask