Skip Menu |
 

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

Report information
The Basics
Id: 33502
Status: open
Priority: 0/
Queue: Class-DBI

People
Owner: Nobody in particular
Requestors: mschwern [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: v3.0.9
Fixed in: (no value)



Subject: insert() will not call create() overrides
Download (untitled) / with headers
text/plain 315b
3.0.9 changed create() to insert() and left in an alias for create(). create() and insert() should be synonyms. Unfortunately the way it was done, calling insert() bypasses a subclass' create() override. This means older CDBI classes may misbehave when insert() is called. The attached patch fixes the problem.
Subject: create_alias.patch
Download create_alias.patch
text/x-diff 1.7k
--- lib/Class/DBI.pm (revision 54479) +++ lib/Class/DBI.pm (local) @@ -426,7 +426,7 @@ return defined($exists) ? $exists : $class->insert($hash); } -sub insert { +sub create { my $class = shift; return $class->_croak("insert needs a hashref") unless ref $_[0] eq 'HASH'; my $info = { %{ +shift } }; # make sure we take a copy @@ -445,7 +445,12 @@ return $class->_insert($data); } -*create = \&insert; +# Ensure that calls to insert() call old create() overrides. +sub insert { + my $class = shift; + $class->create(@_); +} + #---------------------------------------------------------------------- # Low Level Data Access --- t/create_alias.t (revision 54479) +++ t/create_alias.t (local) @@ -0,0 +1,57 @@ +#!/usr/bin/perl -w + +# Test the alias between create() and insert() for the purposes of backwards +# compatibility + +use Test::More; +use strict; + +BEGIN { + eval "use DBD::SQLite"; + plan $@ ? (skip_all => 'needs DBD::SQLite for testing') : ('no_plan'); +} + +INIT { + use lib 't/testlib'; + use Film; +} + + +{ + my $film = Film->create({ + Title => 'Impulse', + }); + + isa_ok $film, 'Film'; + is $film->Title, 'Impulse', 'create() works'; + + ok $film->delete; +} + +{ + my $create_called = 0; + my $film; + + { + package Film; + no warnings 'once'; + local *create = sub { + my $class = shift; + + $create_called++; + $class->SUPER::create(@_); + }; + + $film = Film->insert({ + Title => 'Inframan', + }); + } + + isa_ok $film, 'Film'; + is $film->Title, 'Inframan'; + + is $create_called, 1, 'insert() calls a create() override'; + + ok $film->delete; +} +
Subject: Re: [rt.cpan.org #33502] insert() will not call create() overrides
Date: Fri, 21 Mar 2008 04:55:48 +1000
To: bug-Class-DBI [...] rt.cpan.org
From: "Tony Bowden" <tony [...] tmtm.com>
Download (untitled) / with headers
text/plain 772b
Show quoted text
> 3.0.9 changed create() to insert() and left in an alias for create(). > create() and insert() should be synonyms. Unfortunately the way it was > done, calling insert() bypasses a subclass' create() override. This > means older CDBI classes may misbehave when insert() is called.
I need a little more convincing that this is The Right Thing™. My thinking here, which I admit isn't entirely well formed, goes: Calling insert() implies that you're coding against a 3.0.9+ interface, in which case the code you're working with should be compatible with that. If you have a create() override then you're still in a sub 3.0.9 world and shouldn't be using insert() until you resolve that. This patch seems like it might have worse side-effects later. Tony
Subject: Re: [rt.cpan.org #33502] insert() will not call create() overrides
Date: Thu, 20 Mar 2008 13:16:15 -0700
To: bug-Class-DBI [...] rt.cpan.org
From: Michael G Schwern <schwern [...] pobox.com>
Download (untitled) / with headers
text/plain 2.6k
tony@tmtm.com via RT wrote: Show quoted text
> <URL: http://rt.cpan.org/Ticket/Display.html?id=33502 > >
>> 3.0.9 changed create() to insert() and left in an alias for create(). >> create() and insert() should be synonyms. Unfortunately the way it was >> done, calling insert() bypasses a subclass' create() override. This >> means older CDBI classes may misbehave when insert() is called.
> > I need a little more convincing that this is The Right Thing™. > > My thinking here, which I admit isn't entirely well formed, goes: > > Calling insert() implies that you're coding against a 3.0.9+ interface, in > which case the code you're working with should be compatible with that. > If you have a create() override then you're still in a sub 3.0.9 world and > shouldn't be using insert() until you resolve that.
That implies the people writing the CDBI classes and using the CDBI classes are the same people. Consider a big pile of code written pre-3.0.9 which has create() overrides. Or a CPAN module. Then new users/developers come in, read the CDBI docs and start using insert(), and it works (depending on how important the create() override was) but is subtly and silently wrong. There's also the pre/post 3.0.9 compatibility to consider. insert() replaces create(), how does one write code compatible across 3.0.9? I suppose one answer would be to use before/after create triggers instead of overriding create() but it's often a whole lot simpler to just do the override and lots of code does that. I guess what it boils down to is this: Is insert() backwards compatible with create()? If the answer is yes, then insert() should honor an overridden create() because that is fully compatible. If the answer is no, then calling create() should issue a deprecation warning and instructions to change to insert(). To give some background, this came up because one "interesting" use of this "feature" is to bypass an overridden create(). The developers were using insert() to mean "bypass our create() override". Unfortunately new developers, like myself, didn't realize and started using insert() to mean just insert(). DBIx::Class::CDBICompat implicitly fixed this bug and revealed the differing uses. Now we have to untangle the "insert() means create()" from the "insert() means bypass create() override". Fun. Show quoted text
> This patch seems like it might have worse side-effects later.
I admit to having a bit of that feeling myself. -- 100. Claymore mines are not filled with yummy candy, and it is wrong to tell new soldiers that they are. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/


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.