Rails core - Jun 2009 - Aggregations misbehave when converter returns nil

For those of you that are familiar with composed_of/aggregations, I''ve
created a patch to address a small bug in the writer''s behavior when
the converter returns nil.

Currently the writer first checks for nil assignment, and only then
does it set the mapped attributes to nil.  If the assigned value is
not nil, then it calls the converter and, finally, applies the mapping
rules.  The problem occurs when the converter returns nil: the mapping
methods are invoked on nil.

My patch runs the converter first (BUT NOT with nil assignments) and
then looks for nil values to decide to map or convert mapped
attributes to nil.

I''ve created a nice test case which exercises the theoretical
difference between to_s and to_str.

Reaction-provoking comment?: :allow_nil => false''s behavior seems
contrived now that we have all the goodness of :converter
and :constructor.  At a minimum, someone should consider making the
default for :allow_nil be true.  But I was too scared to add that to
my patch.

Comments/votes welcome: 
https://rails.lighthouseapp.com/projects/8994/tickets/2747

I know aggregations are not the most exciting part of Rails these
days, but can someone please look at this ticket/post?

-Chris

On Jun 3, 8:56 am, Chris Cruft <c...@hapgoods.com>
wrote:
> For those of you that are familiar with composed_of/aggregations,
I''ve
> created a patch to address a small bug in the writer''s behavior
when
> the converter returns nil.
>
> Currently the writer first checks for nil assignment, and only then
> does it set the mapped attributes to nil.  If the assigned value is
> not nil, then it calls the converter and, finally, applies the mapping
> rules.  The problem occurs when the converter returns nil: the mapping
> methods are invoked on nil.
>
> My patch runs the converter first (BUT NOT with nil assignments) and
> then looks for nil values to decide to map or convert mapped
> attributes to nil.
>
> I''ve created a nice test case which exercises the theoretical
> difference between to_s and to_str.
>
> Reaction-provoking comment?: :allow_nil => false''s behavior
seems
> contrived now that we have all the goodness of :converter
> and :constructor.  At a minimum, someone should consider making the
> default for :allow_nil be true.  But I was too scared to add that to
> my patch.
>
> Comments/votes welcome:
 https://rails.lighthouseapp.com/projects/8994/tickets/2747