arfa has asked for the wisdom of the Perl Monks concerning the following question:

Greetings,

so, first visit, first post so --) here goes:
I am trying to slot an upload routine into an existing database management script so I can include the upload field on the main page (as opposed to a seperated upload page - and script).

So, I am not a coder - just a hacker. BUT, I am a monk - 14 yrs now :))
And, here is my current doodle....
$formdata is the std field defn. for the script I am tweaking
$upload_dir is defined elsewhere
"file" is the upload field name

######____ AK upload from here 

$filename = $formdata{"file"};
$filename =~ s/(\s+|\n)?,(\s+|\n)?/,/g;
$upload_filehandle = upload($formdata{"file"});
&upload_file;
    sub upload_file {
        $filepath=$filename;
        if ($filepath =~ /([^\/\\]+)$/) {$filename="$1";}
            else    {$filename="$filepath";}
               $filename =~ s/\s+//g; 
open UPLOADFILE, ">$upload_dir/$filename";
while ( <$upload_filehandle> )
{print UPLOADFILE;}
close UPLOADFILE;
}
if ($to_upload == 1) {$formdata{'file'} = $filename}; #ensures just fi
+lename written to database

[download]
######____ AK upload TO here

This writes filename to the database - great!
writes filename to $upload_dir - BUT - a zero file; ie. it doesn't move the bits :(

I am hoping for just a simple module but am open to extended suggestions.

with thanks and blessing
> arfa

Edit by tye, remove BR tags, add "/" to closing CODE tag

Replies are listed 'Best First'.
Re: upload mod insert
by Abigail-II (Bishop) on Dec 05, 2003 at 10:09 UTC
    Let's first reformat your code to have proper indentation, and let's extract the sub from the code: 
     1: sub upload_file {
 2:    $filepath=$filename;
 3:    if ($filepath =~ /([^\/\\]+)$/) {$filename="$1";}
 4:    else    {$filename="$filepath";}
 5:    $filename =~ s/\s+//g;
 6:    open UPLOADFILE, ">$upload_dir/$filename";
 7:    while (<$upload_filehandle>) {
 8:        print UPLOADFILE;   
 9:    }
10:    close UPLOADFILE;
11: }

12: $filename = $formdata{"file"};
13: $filename =~ s/(\s+|\n)?,(\s+|\n)?/,/g;
14: $upload_filehandle = upload($formdata{"file"});
15: &upload_file;
16: if ($to_upload == 1) {
17:     $formdata{'file'} = $filename
18: }; #ensures just filename written to database

    [download]
    I'm not sure if lines 2 and 3 do what you want to do. They seem to be an attempt to extract the filename component from a full path, however, if the full path ends with a forward or backward slash, you keep the full path (and then you end up trying to open a directory). Perhaps you want to use the Basename module instead. Furthermore, you do part of the filename manipulation in the upload_file sub, and part outside the sub (line 13). Also, you use global variables to pass arguments to the sub, instead of just using parameters.

    In line 6, you open the file you want to write to. But what if the open fails? You assume it will succeed, but opening might fail. You should check for that, and not blindly proceed, assuming it succeeded.

    In line 13, there's no need to write (\s+|\n)?. Since \s covers \n, the second clause will never match. Furthermore, no need to have an optional 1 or more times; zero or more times means the same. This means that line 13 could be written as: 

        $filename =~ s/\s*,\s*/,/g;

    [download]

    In line 14, you assign the result of upload to $upload_filehandle, from which you read in the sub. But you never test whether upload actually returned a valid filehandle. It might have returned undef.

    In line 16, you check whether $to_upload equals 1. But that variable is never set in your code fragment.

    Abigail

[reply]
[d/l]
[select]
      What's module Basename (I can't find it)?
[reply]
        Basename.pm is included in the standard distro and will be located somewhere in your @INC directories in a directory called File which is its root namespace.

        Try this on the CLI:
        perl -e 'print join("\n", @INC);'

        And you will see that for example in my system it is here with /usr/share/perl/5.8.0 being in my @INC:

        barrd:~# locate Basename.pm
/usr/share/perl/5.8.0/File/Basename.pm

        [download]

        Update: Added more text to make a bit more sense

[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom