BUILD called twice.

[Bill Moseley]

Hi all,

I had a problem that I tracked down to BUILD getting run twice.

It turned out a subclass was (incorrectly) using "after 'BUILD'". When
that happens the BUILD in the base class runs twice.

I realize it's an improper use of BUILD but just wanted to note it in case
it's not an expected behavior.

package baseclass;
use Moose;
sub BUILD { warn "in baseclass BUILD\n" }

package subclass;
use Moose;
extends 'baseclass';
# WRONG usage
after 'BUILD' => sub { warn "in bad subclass BUILD\n" };

package main;
subclass->new;

1;


Results in:

in baseclass BUILD
in baseclass BUILD
in bad subclass BUILD



$Moose::VERSION = '2.0604';

--
Bill Moseley
mos...@hank.org

[Dave Rolsky]

This is definitely a bug but it might be tricky to fix. Basically, method
modifiers effectively copy the modified sub into the class that has the
modifier. Moose then sees a BUILD in the subclass and baseclass and
dutifully calls both.

But as you point out, what you did is just wrong anyway. The whole point
of BUILD is that defining it in subclasses doesn't hide the parent's
implementation.

The only time you need modifiers on BUILD is when you want a role to
modify BUILD.


-dave

/*============================================================
http://VegGuide.org http://blog.urth.org
Your guide to all that's veg House Absolute(ly Pointless)
============================================================*/

[Bill Moseley]

On Thu, Feb 14, 2013 at 10:05 AM, Dave Rolsky <aut...@urth.org> wrote:

>
> But as you point out, what you did is just wrong anyway. The whole point
> of BUILD is that defining it in subclasses doesn't hide the parent's
> implementation.
>
> The only time you need modifiers on BUILD is when you want a role to
> modify BUILD.
>

Hum, ok, so on a related question:

What's the correct approach if you need to completely override a BUILD in a
base class with a replacement version in a subclass? That is you do not
want the baseclass BUILD to run?



--
Bill Moseley
mos...@hank.org

[Karen Etheridge]

On Thu, Feb 14, 2013 at 12:05:02PM -0600, Dave Rolsky wrote:
> This is definitely a bug but it might be tricky to fix. Basically, method
> modifiers effectively copy the modified sub into the class that has the
> modifier. Moose then sees a BUILD in the subclass and baseclass and
> dutifully calls both.
>
> But as you point out, what you did is just wrong anyway. The whole point
> of BUILD is that defining it in subclasses doesn't hide the parent's
> implementation.
>
> The only time you need modifiers on BUILD is when you want a role to
> modify BUILD.

Would it be sensible to warn in this case - when the BUILD sub is copied
from the base class into the subclass to be modified? That would force the
user to create an empty BUILD first before modifying it. I can't think of
a case where this behaviour is actually intentionally wanted.

[Jesse Luehrs]

In this case, you don't really want to be using BUILD at all, because
running everything is basically the entire point of BUILD. Maybe your
base class BUILD method should just call a separate init method, which
your subclasses can override?

-doy

[Jesse Luehrs]

On Thu, Feb 14, 2013 at 11:11:12AM -0800, Bill Moseley wrote:

> On Thu, Feb 14, 2013 at 11:00 AM, Jesse Luehrs <d...@tozt.net> wrote:
>
> >
> > In this case, you don't really want to be using BUILD at all, because
> > running everything is basically the entire point of BUILD. Maybe your
> > base class BUILD method should just call a separate init method, which
> > your subclasses can override?
> >
>

> Yes, I agree.
>
> What I'm trying to do is override the BUILD method in
> Catalyst::Model::DBIC::Schema -- essentially to change one line. So, the
> right thing to do with that is to move the code into an init() method,
> true. But, I'm trying to override third-party code.
>
> So, in a case where I cannot easily change the third-party base class, what
> about this ugly approach?:
>
> Baseclass->meta->remove_method( 'BUILD' );

No, that's almost certainly a bad idea. I think the best option in this
case is to take this question to #catalyst or #dbix-class, and see if
they can come up with a better solution(:

-doy

[Bill Moseley]

On Thu, Feb 14, 2013 at 11:13 AM, Jesse Luehrs <d...@tozt.net> wrote:

> > Baseclass->meta->remove_method( 'BUILD' );
>
> No, that's almost certainly a bad idea. I think the best option in this
> case is to take this question to #catalyst or #dbix-class, and see if
> they can come up with a better solution(:
>

How bad? :) Like bad break or bad ugly? I think the only other solution
is to build my own version of Catalyst::Model::DBIC::Schema.



The back story here is Catalyst::Model::DBIC::Schema blesses the DBIC
Result classes into a namespace that is application-specific.

$self->composed_schema($schema_class->compose_namespace(*$class*))
unless $is_installed;


But, the same DBIC schema is shared by more than one application meaning
the same row objects are blessed into different classes in different apps.

Some of these row objects are cached and when another app attempts to thaw
the object it blows up because the class name serialized into Memcached
doesn't exist in that app. App1 caches, then App2 reads but blows up when
trying to create App1::Model::Foo objects.

So, the attempted fix was:

$self->composed_schema($schema_class)
unless $is_installed;

so that all Result classes have the same class name. But, that line of
code is in the BUILD method.





--
Bill Moseley
mos...@hank.org

[Bill Moseley]

On Thu, Feb 14, 2013 at 11:34 AM, Bill Moseley <mos...@hank.org> wrote:

>
>
> On Thu, Feb 14, 2013 at 11:13 AM, Jesse Luehrs <d...@tozt.net> wrote:
>
>> > Baseclass->meta->remove_method( 'BUILD' );
>>
>> No, that's almost certainly a bad idea. I think the best option in this
>> case is to take this question to #catalyst or #dbix-class, and see if
>> they can come up with a better solution(:
>>
>
> How bad? :) Like bad break or bad ugly? I think the only other solution
> is to build my own version of Catalyst::Model::DBIC::Schema.
>

Sorry to poke on this, but in a meeting yesterday afternoon discussing
solutions other developers asked this same question.

Mostly wondering if it can potentially break Moose or it was simply that
brute-force removal of a parent's method is simply a bad idea in general --
especially one that the parent's author might have assumed would always
run. It would be great if I could give them a more "official" answer.

In the end we replaced Catalyst::Model::DBIC::Schema with our own modified
version. But, that's about as brute-force replacement of the parent's
BUILD method as you can get. :) But, there could be cases where one would
not want to do something so global, yet need to override or somehow modify
the BUILD method as one might normally do for other methods via a subclass.

Thanks,


--
Bill Moseley
mos...@hank.org

[Jesse Luehrs]

It could easily break things because first of all, it's a global change,
which could break other code that isn't expecting it, and second of all,
there's no way to guarantee that your code will continue to do the right
thing, since a new release could easily add additional functionality
into BUILD which your overridden version would no longer do (and this
sort of thing would be quite difficult to track down). Can you perhaps
override compose_namespace in your schema classes instead? Otherwise,
again, I highly recommend you ask the Catalyst or DBIC communities for
help in doing what you want, since they are much more likely to have
useful solutions.

-doy

[Bill Moseley]

On Fri, Feb 15, 2013 at 7:35 AM, Jesse Luehrs <d...@tozt.net> wrote:

>
> It could easily break things because first of all, it's a global change,
> which could break other code that isn't expecting it, and second of all,
> there's no way to guarantee that your code will continue to do the right
> thing, since a new release could easily add additional functionality
> into BUILD which your overridden version would no longer do (and this
> sort of thing would be quite difficult to track down). Can you perhaps
> override compose_namespace in your schema classes instead? Otherwise,
> again, I highly recommend you ask the Catalyst or DBIC communities for
> help in doing what you want, since they are much more likely to have
> useful solutions.
>

Ok, so it doesn't sound like it's a specific problem related to Moose,
which was what most were curious about. But just a bad idea in general.
I certainly agree with the risks you enumerate above.

Yes, over the last few days I have asked both Catalyst and DBIC
communities. #dbix-class response was essentially "No idea why the code is
like that. Try the change it and report back." Seems ok so far.

I couldn't get a clear understanding of compose_namespace so I was a
little hesitant to override in the schema class -- not sure what else might
call it. If the code was in an init() instead of BUILD then overriding
init() would be the answer.


Thanks very much for the feedback,


--
Bill Moseley
mos...@hank.org