I've noticed that suggestions.pl, opac-suggestions.pl, and other old code
make use of the Vars() CGI method.
"Called in a scalar context, it returns the parameter list as a tied hash
reference. Changing a key changes the value of the parameter in the
underlying CGI parameter list."
This has some problems.
When you manipulate the tied hash reference, you're changing the underlying
CGI object. That means that calls to $input will change depending on what
you've entered into $tied_hashref. 99% of the time, we probably do NOT want
to do that.
In suggestion.pl, it creates all sorts of weirdness:
~line 245:
unless ( exists $$suggestion_ref{'branchcode'} ) {
$$suggestion_ref{'branchcode'} = C4::Context->userenv->{'branch'};
}
~line 303:
my $branchfilter = ($displayby ne "branchcode") ?
$input->param('branchcode') : '';
$$suggestion_ref itself is tied to $input->param(s). So in the first case,
by default, there won't be a branchcode. So we stuff in the user context
branch. In this case, it's a debatable idea. I've noticed a few librarians
complaining about not being able to find suggestions, and it's because of
this default branch filter which isn't obvious at all. But that's a side
note...
Anyway, once $$suggestion_ref{'branchcode'} is changed,
$input->param('branchcode') will also be changed, as they rely on the same
hash in the CGI object.
I was going crazy over this one... I couldn't see how $input was being set
at all! Then I figured out that $suggestion_ref was a tied hash ref. It
makes the following line utterly redundant:
($branchfilter and $branches->{$thisbranch}->{'branchcode'} eq $branchfilter
) || ( $$suggestion_ref{'branchcode'} and
$branches->{$thisbranch}->{'branchcode'} eq $$suggestion_ref{'branchcode'} )
$branchfilter will always be the same as $$suggestion_ref{'branchcode'}.
In terms of the end result. it might not matter so long as we keep that
earlier condition "unless ( exists $$suggestion_ref{'branchcode'} )" so that
we're not overwriting the real CGI parameter with our user environment
variable.
However, it makes for some ridiculous code. It took longer than I would've
liked to figure out what was going on there. I rather improve code
readability and not have situations where we might accidentally be changing
underlying variables without knowing it. This might even have a few issues
in these scripts that I haven't noticed.
Actually, I have noticed a problem in opac-suggestions.pl because of this
very issue.
~line 37 in opac-suggestions.pl:
"my $suggestion = $input->Vars;"
We later try passing $suggestion to a DBIC object, but it croaks because the
template used "branch" while the DBIC object expects "branchcode".
We should probably be using the param() method to explicitly build our data
structure. Using Vars() also has other consequences. If we add a CGI
parameter and don't delete it in the opac-suggestions.pl script (like we're
already doing for 'op') then we're going to get more errors. If this were
set up as a service and the client sent some additional information, it
would croak.
I suppose what I'm trying to say is. we should keep this in mind when we're
writing new code and when we're looking at old code.
opac-suggestions.pl will need a patch just to start working again (although
the easiest fix there is to change the template rather than the perl
script).
Alas, I don't have time at the moment to tidy up these scripts. I found out
what I need to know and now I have to move onto the next issue.
I don't mean this to be a "someone should fix this" email. Rather a FYI.
David Cook
Systems Librarian
Prosentient Systems
72/330 Wattle St, Ultimo, NSW 2007
_______________________________________________
Koha-devel mailing list
Koha-devel@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
