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

[Ruslan Zakirov]

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.