Skip Menu |
 

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

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

People
Owner: Nobody in particular
Requestors: fitz.elliott [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 0.08198
Fixed in: 0.08204

Attachments
0001-replace-calls-to-rs-new-with-rs-new_result.patch
0002-test-that-new-on-a-ResultSet-object-returns-a-Row.patch



Subject: new_related() uses a strange interface, killing Moosified resultset classes
Download (untitled) / with headers
text/plain 1.3k
Hello, DBIC::Relationship::Base::new_related() creates the new object by calling $self->search_related($rel)->new($values) instead of $self->search_related($rel)->new_result($values) This is semantically strange (using ResultSet->new to create a Row?) and breaks Moose- based custom ResultSet classes. Unmoosed classes work because ResultSet detects when the first arg to new() is $self and proxies the call to new_result(). Moose munges the arguments to new() so that it receives ($class, $values, $attrs) instead of ($self, $values, $attrs). new() then calls ->isa() on $values and dies. I changed new() to new_result() as above and both my code and the DBIC test suite passed without failures. To see if any other code in DBIx::Class used the $rs_obj->new pattern, I updated the proxying code to die. Two test files, t/52leaks.t and t/60core.t, use that syntax but nothing else does. I don't know if $rs->new() can be changed, since it's a published interface (albeit old, circa 2006), but I don't think it should be used internally. I've attached two patches. The first changes the ->new() calls in Base.pm, 52leaks.t, and 60core.t to new_result(). The second adds a test to 60core.t to make sure that $rs->new() returns a DBIx::Class::Row object, if you wish to keep this behavior. Thank you for your time and for maintaining DBIC! Cheers, Fitz Elliott
Subject: 0002-test-that-new-on-a-ResultSet-object-returns-a-Row.patch
From 02c8390379638e8281b6bb389fb59f43d050f438 Mon Sep 17 00:00:00 2001 From: Fitz Elliott <fitz.elliott@gmail.com> Date: Thu, 12 Jul 2012 17:26:59 -0400 Subject: [PATCH 2/2] test that ->new() on a ResultSet object returns a Row diff --git a/t/60core.t b/t/60core.t index 81a98f1..9a90906 100644 --- a/t/60core.t +++ b/t/60core.t @@ -553,6 +553,14 @@ lives_ok (sub { my $newlink = $newbook->link}, "stringify to false value doesn't ); } +# test to make sure that calling ->new() on a resultset object gives +# us a row object +{ + my $new_artist = $schema->resultset('Artist')->new({}); + isa_ok( $new_artist, 'DBIx::Class::Row', '$rs->new gives a row object' ); +} + + # make sure we got rid of the compat shims SKIP: { skip "Remove in 0.082", 3 if $DBIx::Class::VERSION < 0.082; -- 1.7.11.1
Subject: 0001-replace-calls-to-rs-new-with-rs-new_result.patch
From be4e5a3aba05759b35c55c86afdbda79564401fd Mon Sep 17 00:00:00 2001 From: Fitz Elliott <fitz.elliott@gmail.com> Date: Thu, 12 Jul 2012 17:07:31 -0400 Subject: [PATCH 1/2] replace calls to $rs->new() with $rs->new_result() When new() is called on an instantiated ResultSet object, the call is proxied to new_result(). This patch replaces those new() calls with direct calls to new_result(), so that the intent is clearer and to avoid breaking Moose-based resultsets. Moose munges the arguments to new(), replacing $self with $class bypassing the proxy call. diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index 6cfe28c..7267702 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -600,7 +600,7 @@ sub new_related { } } - my $row = $self->search_related($rel)->new($values, $attrs); + my $row = $self->search_related($rel)->new_result($values, $attrs); return $row; } diff --git a/t/52leaks.t b/t/52leaks.t index 7b51dc4..724b42f 100644 --- a/t/52leaks.t +++ b/t/52leaks.t @@ -173,7 +173,7 @@ my @compose_ns_classes; # dbh_do codepath my ($rs_bind_circref, $cond_rowobj) = $schema->storage->dbh_do ( sub { - my $row = $_[0]->schema->resultset('Artist')->new({}); + my $row = $_[0]->schema->resultset('Artist')->new_result({}); my $rs = $_[0]->schema->resultset('Artist')->search({ name => $row, # this is deliberately bogus, see FIXME below! }); diff --git a/t/60core.t b/t/60core.t index edf5758..81a98f1 100644 --- a/t/60core.t +++ b/t/60core.t @@ -191,7 +191,7 @@ is($cd->title, 'Spoonful of bees', 'Correct CD returned with include'); is($cd->get_column('artist_name'), 'Caterwauler McCrae', 'Additional column returned'); # update_or_insert -$new = $schema->resultset("Track")->new( { +$new = $schema->resultset("Track")->new_result( { trackid => 100, cd => 1, title => 'Insert or Update', -- 1.7.11.1
Download (untitled) / with headers
text/plain 1.5k
On Thu Jul 12 17:36:46 2012, felliott wrote: Show quoted text
> Hello, > > DBIC::Relationship::Base::new_related() creates the new object by > calling > $self->search_related($rel)->new($values) > instead of > $self->search_related($rel)->new_result($values) > > This is semantically strange (using ResultSet->new to create a Row?) > and breaks Moose- > based custom ResultSet classes. Unmoosed classes work because > ResultSet detects when the > first arg to new() is $self and proxies the call to new_result(). > Moose munges the arguments > to new() so that it receives ($class, $values, $attrs) instead of > ($self, $values, $attrs). new() > then calls ->isa() on $values and dies.
This is a 7 year old design decision. Just because Moose is the cool kid in town doesn't mean we get to break backcompat on this front. Moreover this particular bit of wartiness is documented here: https://metacpan.org/module/DBIx::Class::ResultSet#CUSTOM-ResultSet-CLASSES-THAT-USE-Moose I applied your second patch, since an explicit test for this behavior (which will not change) is a good thing. https://github.com/dbsrgits/dbix-class/commit/5b80662b As far as your first patch goes - I am changing the new() call to new_result() for the sake of efficiency (one less method lookup). The test-modifying bits I left off, for the reasons stated above. Interestingly enough while looking into this a gross API mismatch poped up. Luckily it went unnoticed enough to not be documented and is now resolved with copious amounts of docs and tests. https://github.com/dbsrgits/dbix-class/commit/36f18206 Thank you for your help!


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.