Bug #45073 for CGI: Needs Peer Review: PATCH: Calling the upload method without argument leads to an error

On Thu Apr 16 02:24:22 2009, KNI wrote: Show quoted text
Thank for you this patch. I recommend that it be accepted because it makes the behavior consistent with how param() works. My only suggestion is that that the documentation be updated as well. Just after this: "In a list context, upload() will return an array of filehandles. This makes it possible to create forms that use the same name for multiple upload fields." Add this: "If arguments are provided to upload(), uploads for all fields are considered." An example could also be added: @all_uploads = $q->upload();

On Thu Apr 16 02:24:22 2009, KNI wrote: Show quoted text
After further review, I'm rejecting the patch as proposed. Here's the line it added: my @param = grep {ref($_) && defined(fileno($_))} map { $self->param($_) } $param_name || $self->param(); First, that line is rather tricky. It deserves at least a code comment if not a rewrite to be clearer. I see two potential problems with it, neither of which are covered by the automated test that was added: - What if "$param_name" is 0? It looks like that would trigger the "no args" behavior unintentionally. We definitely need to check to see if $param_name is defined, not just true. - What happens if the same param name appears more than once, with different file handles? Does it correctly get back all the file handles associated with it, or just the first one? Here's a patch that I'm more comfortable with. It addresses the truth vs. defined bug, and I think also makes it clear that it's it's accumulating the result of processing all the params the same way it handles a single param. Therefore, it should have all the file handles. I'd like another peer review of it before committing it, though: --- a/lib/CGI.pm +++ b/lib/CGI.pm @@ -3716,7 +3716,17 @@ END_OF_FUNC 'upload' =><<'END_OF_FUNC', sub upload { my($self,$param_name) = self_or_default(@_); - my @param = grep {ref($_) && defined(fileno($_))} $self->param($param_name); + my @param; + # If we want the uploads for a specific param name + if (defined $param_name) { + @param = grep {ref($_) && defined(fileno($_))} $self->param($param_name); + } + # With no args, return all the uploads + else { + for my $name ($self->param) { + push @param, grep {ref($_) && defined(fileno($_))} $self->param($name); + } + } return unless @param; return wantarray ? @param : $param[0]; }

No one else has expressed interest in this 2009, so I'm rejecting this change. If a few people speak up in favor it, it could be reconsidered, but there are plenty of people who depend on CGI.pm behaving the way it currently does.

No one else has expressed interest in this 2009, so I'm rejecting this change. If a few people speak up in favor it, it could be reconsidered, but there are plenty of people who depend on CGI.pm behaving the way it currently does.