Skip Menu |

This queue is for tickets about the Moo CPAN distribution.

Report information
The Basics
Id: 65636
Status: resolved
Worked: 1 hour (60 min)
Priority: 0/
Queue: Moo

People
Owner: Nobody in particular
Requestors: nomad [...] null.net
Cc:
AdminCc:

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



Subject: BUILD not called in child classes
Date: Fri, 11 Feb 2011 14:59:54 +1300
To: bug-Moo [...] rt.cpan.org
From: Mark Lawrence <nomad [...] null.net>
The docs say this: ... you can define a "BUILD" method on your class and the constructor will automatically call the "BUILD" method from parent down to child after the object has been instantiated. Typically this is used for object validation or possibly logging. But doesn't seem to be the case in practice: $ cat test.t use Test::More tests => 2; use strict; use warnings; { package parent; use Moo; has 'name' => ( is => 'rw', ); sub BUILD { my $self = shift; Test::More::ok 1, "Building parent"; } } { package child; use Moo; extends 'parent'; sub BUILD { my $self = shift; Test::More::ok 1, "Building child"; } } child->new; $ perl test.t 1..2 ok 1 - Building parent # Looks like you planned 2 tests but ran 1. $ perl -p -i -e 's/Moo/Moose/g' test.t $ perl test.t 1..2 ok 1 - Building parent ok 2 - Building child $ perl -p -i -e 's/Moose/Mouse/g' test.t $ perl test.t 1..2 ok 1 - Building parent ok 2 - Building child -- Mark Lawrence
On Thu Feb 10 23:16:29 2011, nomad@null.net wrote: Show quoted text
> The docs say this: > > ... you can define a "BUILD" method on your class and the > constructor will automatically call the "BUILD" method from parent > down to child after the object has been instantiated. Typically > this is used for object validation or possibly logging. > > But doesn't seem to be the case in practice: > > $ cat test.t > > use Test::More tests => 2; > use strict; > use warnings; > > { > package parent; > use Moo; > > has 'name' => ( > is => 'rw', > ); > > sub BUILD { > my $self = shift; > Test::More::ok 1, "Building parent"; > } > } > > { > package child; > use Moo; > > extends 'parent'; > > > sub BUILD { > my $self = shift; > Test::More::ok 1, "Building child"; > } > > } > > child->new; > > > $ perl test.t > > 1..2 > ok 1 - Building parent > # Looks like you planned 2 tests but ran 1. > > $ perl -p -i -e 's/Moo/Moose/g' test.t > $ perl test.t > > 1..2 > ok 1 - Building parent > ok 2 - Building child > > $ perl -p -i -e 's/Moose/Mouse/g' test.t > $ perl test.t > > 1..2 > ok 1 - Building parent > ok 2 - Building child >
I'm experiencing this too. Didn't happen with Moose/Mouse. Apparently, if you do not declare any new attribute in your child class, your BUILD will not get called. My workaround for now is to add a dummy attribute in child classes. Stupid, but it works. -- sh
Fixed.
Subject: build_subclass.t
use Test::More tests => 2; use strict; use warnings; my @ran; { package parent; use Moo; has name => (is => 'rw'); sub BUILD { Test::More::ok 1, "Building parent"; push @ran, "parent"; } } { package child; use Moo; extends 'parent'; sub BUILD { Test::More::ok 1, "Building child"; push @ran, "child"; } } child->new; __END__ { package child2; use Moo; extends 'parent'; has age => (is => 'rw'); sub BUILD { Test::More::ok 1, "Building child2"; push @ran, "child2"; } } child2->new; # works
Subject: build_subclass.patch
diff --git a/lib/Moo.pm b/lib/Moo.pm index f23fe10..3f4a5f4 100644 --- a/lib/Moo.pm +++ b/lib/Moo.pm @@ -24,13 +24,14 @@ sub import { Moo::Role->apply_role_to_package($target, $_[0]); }; $MAKERS{$target} = {}; + my $c = $class->_constructor_maker_for($target); *{_getglob("${target}::has")} = sub { my ($name, %spec) = @_; ($MAKERS{$target}{accessor} ||= do { require Method::Generate::Accessor; Method::Generate::Accessor->new })->generate_method($target, $name, \%spec); - $class->_constructor_maker_for($target) + $c ->register_attribute_specs($name, \%spec); }; foreach my $type (qw(before after around)) {
Subject: Re: [rt.cpan.org #65636] BUILD not called in child classes
Date: Sat, 26 Mar 2011 19:38:57 +1300
To: Rob Bloodgood via RT <bug-Moo [...] rt.cpan.org>
From: Mark Lawrence <nomad [...] null.net>
On Fri Mar 25, 2011 at 10:48:28PM -0400, Rob Bloodgood via RT wrote: Show quoted text
> > Fixed.
That short response is sadly not very illuminating. If I provide a fix for something I usually like to give some indication of where the real problem was, why it didn't work the way it should have, what I did to fix it, and sometimes a "thanks" to whoever found it. I like to encourage bug reporters to be thorough and clear in their reports by being so in my answers... but that's just me. Anyhow, your patch makes this particular test case work, but it broke a bunch of other tests. Try running 'prove' on the t/ directory of the Moo source with your patch applied and you'll see what I mean. One side-effect of your patch that I've noticed is that attributes are no longer populated when the BUILD method is called, and apparently 'isa' checks are not run either. Care to have another stab at the problem? Cheers, Mark. -- Mark Lawrence
On Sat Mar 26 02:39:16 2011, nomad@null.net wrote: Show quoted text
> On Fri Mar 25, 2011 at 10:48:28PM -0400, Rob Bloodgood via RT wrote:
> > > > Fixed.
> > That short response is sadly not very illuminating. If I provide a
fix Show quoted text
> for something I usually like to give some indication of where the
real Show quoted text
> problem was, why it didn't work the way it should have, what I did
to Show quoted text
> fix it, and sometimes a "thanks" to whoever found it. I like to > encourage bug reporters to be thorough and clear in their reports by > being so in my answers... but that's just me.
Well, you're absolutely right, I *did* rush this out. Definitely my bad. To document the problem as requested: The generation of a BUILDALL method, which includes calls to BUILD for each child class, occurs (by trickle down) in the call $class->_constructor_maker_for in Moo.pm. The short version of the problem is, this call is tied to the handler for the 'has' keyword. The subclass listed in the test case doesn't add any attributes, so it doesn't call has(), and doesn't call _constructor_maker_for(). Show quoted text
> Anyhow, your patch makes this particular test case work, but it
broke Show quoted text
> a bunch of other tests. Try running 'prove' on the t/ directory of
the Show quoted text
> Moo source with your patch applied and you'll see what I mean.
Once again, oops. :) Show quoted text
> Care to have another stab at the problem?
Absolutely. (I really did just "whip off" that previous patch. <blush>) But a closer examination of the problem reveals a solution which is cleaner and almost as simple: the results of _constructor_maker_for() are cached, so calling it twice is not an error. Therefore, I duplicated the call in the handler for extends(). This way the required call occurs in subclasses that don't call has(), and is a no-op in classes that do. This fixes the test case, as well as passing all (other) tests. Attached.
Subject: build_subclass_2.patch
diff --git a/lib/Moo.pm b/lib/Moo.pm index f23fe10..4d733e3 100644 --- a/lib/Moo.pm +++ b/lib/Moo.pm @@ -17,6 +17,7 @@ sub import { _load_module($_) for @_; # Can't do *{...} = \@_ or 5.10.0's mro.pm stops seeing @ISA @{*{_getglob("${target}::ISA")}{ARRAY}} = @_; + $class->_constructor_maker_for($target); }; *{_getglob("${target}::with")} = sub { require Moo::Role;
Subject: Re: [rt.cpan.org #65636] BUILD not called in child classes
Date: Sat, 2 Apr 2011 12:44:38 +1300
To: Rob Bloodgood via RT <bug-Moo [...] rt.cpan.org>
From: Mark Lawrence <nomad [...] null.net>
On Tue Mar 29, 2011 at 09:44:02PM -0400, Rob Bloodgood via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=65636 > > > To document the problem as requested: > > The generation of a BUILDALL method, which includes calls to BUILD for > each child class, occurs (by trickle down) in the call > $class->_constructor_maker_for in Moo.pm. The short version of the > problem is, this call is tied to the handler for the 'has' keyword. > > The subclass listed in the test case doesn't add any attributes, so it > doesn't call has(), and doesn't call _constructor_maker_for().
Very illumniating. Seriously. This has contributed to my understanding of how the system works far better than the look I took through the code. Show quoted text
> But a closer examination of the problem reveals a solution which is > cleaner and almost as simple: the results of _constructor_maker_for() > are cached, so calling it twice is not an error. Therefore, I > duplicated the call in the handler for extends(). This way the > required call occurs in subclasses that don't call has(), and is a > no-op in classes that do. > > This fixes the test case, as well as passing all (other) tests.
Marvelous. You've made my satisfaction meter sing. I see that your patch hasn't made its way yet into the Git repo. Who can I ping (or send a pull request to - I can even build the commit if necessary) to make that happen? Mark. -- Mark Lawrence
On Fri Apr 01 20:34:47 2011, nomad@null.net wrote: Show quoted text
> On Tue Mar 29, 2011 at 09:44:02PM -0400, Rob Bloodgood via RT wrote:
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=65636 > > > > > To document the problem as requested: > > > > The generation of a BUILDALL method, which includes calls to BUILD for > > each child class, occurs (by trickle down) in the call > > $class->_constructor_maker_for in Moo.pm. The short version of the > > problem is, this call is tied to the handler for the 'has' keyword. > > > > The subclass listed in the test case doesn't add any attributes, so it > > doesn't call has(), and doesn't call _constructor_maker_for().
That's fixed in current CPAN; please come annoy me in #web-simple on irc.perl.org to get commit bits.