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

Fri May 4 10:42:39 EDT 2007

On 5/4/07, Dmitri Tikhonov <dtikhonov at vonage.com> wrote:
> 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.
Ok, my fault, but this means that we have preferred solution. All we
need to do is review of all Create methods and check that they use
wantarray on return.

Jesse?

> - 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.
I don't think you should spend your time on resubmitting the patch, we
can do that during the global review, but I don't know if we'll have
time this year -- 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.
> >
>
>

-- 
Best regards, Ruslan.