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