[Rt-devel] Error propagation in RT::CustomField::Create.

Fri May 4 09:14:12 EDT 2007

Hi Ruslan,

I downloaded RT 3.6.3 (so that we all have an immovable point of 
reference) and checked out *.pm files in lib/RT directory.  Here is what 
I found:

- There are 42 Create methods [1].
- RT::Record's (from which all other objects inherit, not from 
DBIx::SearchBuilder::Record, as you have mistakenly stated) Create 
method is context-sensitive -- in one case it returns one, in the other 
two values.
- RT::Template::Create returns 1 value
- RT::GroupMember::Create returns 1 value
- RT::CachedGroupMember::Create returns 1 value
- RT::Ticket::Create returns 3 values

The rest either return SUPER::Create (which would also make it 
context-sensitive, since that's calling RT::Record::Create) or return 
two values.  That makes for 38/42, or about 90% of Create calls will 
happily return two values when called like this:

   ($id, $msg) = $blah->Create

Then I examined how Create methods are actually called in lib/RT/*.pm 
[2].  Most calls are in list context, of form ($id, $msg) =...; a few in 
scalar context, and in another few cases, the return value is ignored.

Looking at these results, I would say that RT does have a convention, 
and that is returning two values -- the first one is id, the second is 
error message in case the first value evaluates to false (indicates an 
error).  Anything that does not conform to that is an exception 
(especially in the case of 3 return values).

To make my patch backward-compatible (in case there are calls to this 
Create method in scalar context), we can do this:

   return (wantarray ? @rv : shift(@rv));

I think that will work.  If you insist on a global patch to cure all 
ills of Create method return values and calling conventions, it won't be 
coming from me -- sorry.

Sincerely,

   - Dmitri.

1. Funny how that number is everywhere :)
2. grep -h '\->Create[ ]*(' *.pm  | grep -v ^ok

Ruslan Zakirov wrote:
> I prefer to avoid such changes unless we have defined behavior of
> Create method in RT.
> 
> Create in SB returns id and no errors or something like that. As far
> as I remember RT followed this rule, but only partially and Create
> methods in some classes returns (id, $msg) in list context. This makes
> development very annoying, you have to check code each time you call
> Create or should write "my ($id) = $obj->Create(...);". And I'm sure
> that not all code follow the latter style.
> 
> "my $id = $obj->Create(...);" problematic in any case.
> * if a sub returns array (for eg @rv) then you get number of elements
> instead of id:
>  > perl -e 'sub create { my @rv = (0, "bad"); return @rv; }; my $id = 
> create(); print "$id\n"'
> 2
> * if a sub returns list (for eg (0, "bad")) then you get a last one element:
>  > perl -e 'sub create { return (0, "bad") }; my $id = create(); print 
> "$id\n"'
> bad
> 
> In the both cases you get defined and true value and skip an error.
> Invoking the sub in array context fixes issue:
>  > perl -e 'sub create { my @rv = (0, "bad"); return @rv; }; my ($id) = 
> create(); print "$id\n"'
> 0
>  > perl -e 'sub create { return (0, "bad") }; my ($id) = create(); print 
> "$id\n"'
> 0
> However as I said we have no consistency here.
> 
> So I see three solutions:
> * call Create sub in array context all the time (requires global cleanup)
> ** this is almost impossible
> * implement wantarray solution for return statements (this makes code
> a little bit hairy)
> ** I can accept it, but this makes code a little bit unreadable, you
> have to parse all this if/else wantarray blocks. However, solution do
> the right thing in all cases we have.
> * die in those Create methods that return (id, msg) or @array if it's
> called in scalar context, something like:
> sub Create {
>   die "this particular create method returns more than id"
>      if defined wantarray && !wantarray;
>   ...
> }
> ** this is the solution I like. I proved above that calling Create
> which returns list or array in scalar context is always a bug in the
> code, so this solution works like 'assert'. you get fatal error,
> description, stack trace and can easy find a point where you should
> fix things.
> 
> I'll accept global patch that implements chosen solution in all RT classes.
> 
> Also, in this particular case SUPER is DBIx::SB and Create in SB never
> returns errors, so this change is useless.
> 
> On 5/3/07, Jesse Vincent <jesse at bestpractical.com> wrote:
>  > Seems 100% reasonable. Ruz: objections?
>  >
>  > On May 3, 2007, at 11:22 AM, Dmitri Tikhonov wrote:
>  >
>  > > Dear RTers,
>  > >
>  > > it would be nice if RT::CustomField's Create() method propagated
>  > > error messages from calls to SUPER::Create.  What follows is a
>  > > patch against the latest 3.7 code (http://svn.bestpractical.com/cgi-
>  > > bin/index.cgi/bps/view/rt/branches/3.7-EXPERIMENTAL/lib/RT/
>  > > CustomField_Overlay.pm):
>  > >
>  > > Thanks,
>  > >
>  > >   - Dmitri.
>  > >
>  > > --- CustomField_Overlay.pm-original     2007-05-03
>  > > 17:03:28.000000000 +0000
>  > > +++ CustomField_Overlay.pm      2007-05-03 17:04:07.000000000 +0000
>  > > @@ -202,7 +202,7 @@
>  > >      my ($ok, $msg) = $self->_IsValidRegex( $args{'Pattern'} );
>  > >      return (0, $self->loc("Invalid pattern: [_1]", $msg)) unless $ok;
>  > >
>  > > -    my $rv = $self->SUPER::Create(
>  > > +    my @rv = $self->SUPER::Create(
>  > >          Name        => $args{'Name'},
>  > >          Type        => $args{'Type'},
>  > >          MaxValues   => $args{'MaxValues'},
>  > > @@ -217,7 +217,7 @@
>  > >          $self->SetValuesClass( $args{'ValuesClass'} );
>  > >      }
>  > >
>  > > -    return $rv unless exists $args{'Queue'};
>  > > +    return @rv unless exists $args{'Queue'};
>  > >
>  > >      # Compat code -- create a new ObjectCustomField mapping
>  > >      my $OCF = RT::ObjectCustomField->new( $self->CurrentUser );
>  > > @@ -226,7 +226,7 @@
>  > >          ObjectId => $args{'Queue'},
>  > >      );
>  > >
>  > > -    return $rv;
>  > > +    return @rv;
>  > >  }
>  > >
>  > >  =head2 Load ID/NAME
>  > > _______________________________________________
>  > > List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/
>  > > rt-devel
>  > >
>  >
>  >
>  > _______________________________________________
>  > List info: 
> http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel
>  >
>  >
> 
> 
> --
> Best regards, Ruslan.
> 