Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the CGI CPAN distribution.

Report information
The Basics
Id: 45073
Status: rejected
Priority: 0/
Queue: CGI

People
Owner: MARKSTOS [...] cpan.org
Requestors: KNI [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: (no value)
Fixed in: (no value)



Subject: [PATCH] Calling the upload method without argument leads to an error
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Charset: utf8
X-RT-Original-Encoding: utf-8
Content-Type: multipart/mixed; boundary="----------=_1239863058-2538-416"
Content-Length: 0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: binary
Content-Length: 135
Download (untitled) / with headers
text/plain 135b
Calling the upload method without argument leads to an error - the method returns no reply. Here is a patch attached to fix the issue.
Subject: CGI_upload.patch
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----------=_1239863058-2538-415"
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Charset: utf8
Content-Length: 0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: iso-8859-1
Content-Length: 0
Content-Type: application/octet-stream; name="CGI_upload.patch"
Content-Disposition: inline; filename="CGI_upload.patch"
Content-Transfer-Encoding: base64
Content-Length: 1269
Download CGI_upload.patch
text/x-diff 1.2k
diff -ru CGI.pm-3.43/CGI.pm CGI.pm-new/CGI.pm --- CGI.pm-3.43/CGI.pm Wed Feb 11 18:56:37 2009 +++ CGI.pm-new/CGI.pm Thu Apr 16 08:53:24 2009 @@ -3640,7 +3640,7 @@ '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 = grep {ref($_) && defined(fileno($_))} map { $self->param($_) } $param_name || $self->param(); return unless @param; return wantarray ? @param : $param[0]; } diff -ru CGI.pm-3.43/t/upload.t CGI.pm-new/t/upload.t --- CGI.pm-3.43/t/upload.t Mon Sep 8 16:52:59 2008 +++ CGI.pm-new/t/upload.t Thu Apr 16 09:02:19 2009 @@ -96,6 +96,17 @@ ok( defined $q->upload('100;100_gif') , 'upload_basic_3' ); ok( defined $q->upload('300x300_gif') , 'upload_basic_4' ); + +{ + my @upload = $q->upload(); + my %upload = map { $_ => $_ } @upload; + ok( exists $upload{'does_not_exist.gif'}, 'upload_basic_all_2' ); + ok( exists $upload{'100;100.gif'} , 'upload_basic_all_3' ); + ok( exists $upload{'300x300.gif'} , 'upload_basic_all_4' ); + +} + + { my $test = "file handles have expected length for multi-valued field. "; my ($goodbye_fh,$hello_fh) = $q->upload('hello_world');
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Charset: utf8
Content-Type: text/plain
Message-ID: <rt-3.6.HEAD-2324-1239931708-1014.45073-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 710
Download (untitled) / with headers
text/plain 710b
On Thu Apr 16 02:24:22 2009, KNI wrote: Show quoted text
> Calling the upload method without argument leads to an error - the > method returns no reply. Here is a patch attached to fix the issue.
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();
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Charset: utf8
Content-Type: text/plain
Message-ID: <rt-3.6.HEAD-26882-1250215152-1872.45073-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 2003
Download (untitled) / with headers
text/plain 1.9k
On Thu Apr 16 02:24:22 2009, KNI wrote: Show quoted text
> Calling the upload method without argument leads to an error - the > method returns no reply. Here is a patch attached to fix the issue.
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]; }
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Content-Type: text/plain; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-30166-1358907235-670.45073-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 233
Download (untitled) / with headers
text/plain 233b
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.
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Content-Type: text/plain; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-22162-1358907371-1157.45073-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 233
Download (untitled) / with headers
text/plain 233b
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.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.