Skip to content

Commit 1f39678

Browse files
committed
Rewrite the WeBWorK::Upload module.
This takes advantage of things available with Mojolicious. This does not use `Data::UUID` anymore. A comment stated that it was overkill. It is and it was never a good idea to use that for this purpose. This just uses the `Mojo::Asset::Memory::to_file` method which converts to a `Mojo::Asset::File` object and saves the file to a temporary file using `Mojo::File::tempfile` (which uses `File::Temp` under the hood). That is guaranteed to give a safe filename that does not conflict with any existing files. Setting the environment variable `MOJO_TMPDIR` locally to be the passed in directory (which is `$ce->{webworkDirs}{uploadCache}`) ensures that the file is saved in the usual webwork2 upload location (although there is no need for that, and we could drop that configuration variable and just use the system temporary directory for this). The info file that is written is now UTF-8 encoded and decoded. This fixes issue openwebwork#2690. The `dir` option which was previously required and the only "option" is now just a required argument. It doesn't make sense to use an "option" hash things that are required. Particularly if there is only one option and it is required. Other than what is mentioned above the module behaves much the same.
1 parent 6395021 commit 1f39678

File tree

4 files changed

+131
-233
lines changed

4 files changed

+131
-233
lines changed

lib/WeBWorK.pm

+6-9
Original file line numberDiff line numberDiff line change
@@ -122,17 +122,14 @@ async sub dispatch ($c) {
122122

123123
my @uploads = @{ $c->req->uploads };
124124

125-
foreach my $u (@uploads) {
126-
# Make sure it's a "real" upload.
127-
next unless $u->filename;
128-
125+
for my $u (@uploads) {
129126
# Store the upload.
130-
my $upload = WeBWorK::Upload->store($u, dir => $ce->{webworkDirs}{uploadCache});
127+
my $upload = WeBWorK::Upload->store($u, $ce->{webworkDirs}{uploadCache});
131128

132-
# Store the upload ID and hash in the file upload field.
133-
my $id = $upload->id;
134-
my $hash = $upload->hash;
135-
$c->param($u->name => "$id $hash");
129+
# Store the upload temporary file location and hash in the file upload field.
130+
my $tmpFile = $upload->tmpFile;
131+
my $hash = $upload->hash;
132+
$c->param($u->name => "$tmpFile $hash");
136133
}
137134

138135
# Create these out here. They should fail if they don't have the right information.

lib/WeBWorK/ContentGenerator/Feedback.pm

+2-2
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ sub initialize ($c) {
174174
my $fileIDhash = $c->param('attachment');
175175
if ($fileIDhash) {
176176
my $attachment =
177-
WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), dir => $ce->{webworkDirs}{uploadCache});
177+
WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $ce->{webworkDirs}{uploadCache});
178178

179179
# Get the filename and read its contents.
180180
my $filename = $attachment->filename;
@@ -184,7 +184,7 @@ sub initialize ($c) {
184184
local $/;
185185
$contents = <$fh>;
186186
};
187-
close $fh;
187+
$fh->close;
188188
$attachment->dispose;
189189

190190
# Check to see that this is an allowed filetype.

lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm

+12-10
Original file line numberDiff line numberDiff line change
@@ -720,8 +720,7 @@ sub Upload ($c) {
720720
return $c->Refresh;
721721
}
722722

723-
my ($id, $hash) = split(/\s+/, $fileIDhash);
724-
my $upload = WeBWorK::Upload->retrieve($id, $hash, dir => $c->{ce}{webworkDirs}{uploadCache});
723+
my $upload = WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $c->{ce}{webworkDirs}{uploadCache});
725724

726725
my $name = $upload->filename;
727726
my $invalidUploadFilenameMsg = $c->checkName($name);
@@ -794,21 +793,24 @@ sub Upload ($c) {
794793
if ($type ne 'Binary') {
795794
my $fh = $upload->fileHandle;
796795
my @lines = <$fh>;
796+
$fh->close;
797797
$data = join('', @lines);
798798
if ($type eq 'Automatic') { $type = isText($data) ? 'Text' : 'Binary' }
799799
}
800800
if ($type eq 'Text') {
801801
$upload->dispose;
802802
$data =~ s/\r\n?/\n/g;
803+
804+
my $backup_data = $data;
805+
my $success = utf8::decode($data); # try to decode as utf8
806+
unless ($success) {
807+
warn "Trying to convert file $file from latin1? to UTF-8";
808+
utf8::upgrade($backup_data); # try to convert data from latin1 to utf8.
809+
$data = $backup_data;
810+
}
811+
803812
if (open(my $UPLOAD, '>:encoding(UTF-8)', $file)) {
804-
my $backup_data = $data;
805-
my $success = utf8::decode($data); # try to decode as utf8
806-
unless ($success) {
807-
warn "Trying to convert file $file from latin1? to UTF-8";
808-
utf8::upgrade($backup_data); # try to convert data from latin1 to utf8.
809-
$data = $backup_data;
810-
}
811-
print $UPLOAD $data; # print massaged data to file.
813+
print $UPLOAD $data;
812814
close $UPLOAD;
813815
} else {
814816
$c->addbadmessage($c->maketext(q{Can't create file "[_1]": [_2]}, $name, $!));

0 commit comments

Comments
 (0)