Rewrite the WeBWorK::Upload module. #2693
Open
+131
−233
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pstaabp
reviewed
Apr 8, 2025
| =over | ||
| An upload stored in the upload cache can be retrieved by supplying its temporary | ||
| file name and hash (accessible from the above C<tmpFile> and C<hash> methods, | ||
| respectively. The file can then be accessed by name or file handle, moved, and |
There was a problem hiding this comment.
minor thing: close the ) after 'respectively' ?
|
Thinking about testing this. There's clearly the email that came from the issue, but does this get called for a file upload to through the File Manager? I notice the code in |
|
There are currently two places this is used, and you mentioned both of them. |
|
We should also remove |
pstaabp
approved these changes
Apr 8, 2025
|
I believe that the |
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This takes advantage of things available with Mojolicious.
This does not use
Data::UUIDanymore. 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 theMojo::Asset::Memory::to_filemethod which converts to aMojo::Asset::Fileobject and saves the file to a temporary file usingMojo::File::tempfile(which usesFile::Tempunder the hood). That is guaranteed to give a safe filename that does not conflict with any existing files. Setting the environment variableMOJO_TMPDIRlocally 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 #2690.
The
diroption 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.