| Subject: | Is the call to __ConvertOldArg() really useful ? |
| Date: | Wed, 13 Jul 2011 19:06:32 +0200 |
| To: | bug-RT-Action-NotifyGroup [...] rt.cpan.org |
| From: | Gerard FENELON <gerard [...] eve-team.com> |
Hi
I was looking at
http://cpansearch.perl.org/src/RUZ/RT-Action-NotifyGroup-0.02/lib/RT/Action/NotifyGroup.pm
and I was surprised by a part of the code.
I must start by saying that I am not a Perl expert at all,
I probably wrote 10000 times less lines of Perl as any of you.
But this code really foxes me and I prefer to mention in case there is a
real bug.
sub SetRecipients
{
my $self = shift;
my $arg = $self->Argument;
my $old_arg = eval { Storable::thaw( $arg ) };
unless( $@ ) {
$arg = $self->__ConvertOldArg( $old_arg );
return;
}
[...]
First, return has no value whereas lower down in the same function I see
"return 1";
I suspect that it is OK, Perl will probably do the "Right Thing"
But maybe that call to return is wrong and you wanted to use $arg in
the code ?
Secondly $arg is not used
so the line
$arg = $self->__ConvertOldArg( $old_arg );
does not do anything unless __ConvertOldArg() does.
So I looked at
sub __ConvertOldArg
{
my $self = shift;
my $arg = shift;
my @res;
foreach my $r ( @{ $arg } ) {
my $obj;
next unless $r->{'Type'};
if( lc $r->{'Type'} eq 'user' ) {
$obj = RT::User->new( $RT::SystemUser );
} elsif ( lc $r->{'Type'} eq 'user' ) {
$obj = RT::Group->new( $RT::SystemUser );
} else {
next;
}
$obj->Load( $r->{'Instance'} );
my $id = $obj->id;
next unless( $id );
push @res, $id;
}
return join ';', @res;
}
It seems to me that this function just build the @res array.
I do not see anywhere where there could be side-effects
or modifications of the state of some objects.
I hope that I have covered myself with ridicule.
Regards
Gerard