Skip to content

[5.6] get contents of uploaded file - #24924

Merged
taylorotwell merged 4 commits into
laravel:5.6from
browner12:uploaded-file-contents
Jul 27, 2018
Merged

[5.6] get contents of uploaded file#24924
taylorotwell merged 4 commits into
laravel:5.6from
browner12:uploaded-file-contents

Conversation

@browner12

Copy link
Copy Markdown
Contributor

sometimes when we upload a file, we don't actually care about the file itself, but rather the contents of the file. this is simply a helper method to get the contents of the file

- `getContents()` checks if a file exists at the temporary upload location, and returns the contents of the file if found.
- added a test, and a file for the test
@sisve

sisve commented Jul 22, 2018

Copy link
Copy Markdown
Contributor

Should you be checking is_uploaded_file to make sure no-one is tricking you into reading something they shouldn't be reading? Or perhaps check $this->isValid() which does this with support for testing?

renamed `fixtures` folder to keep it consistent with other test folders, forgot to update it here.
@browner12

Copy link
Copy Markdown
Contributor Author

Didn't know that function existed, good to know. I like taking advantage of the existing isValid() method. Can I omit my file_exists() call then, does is_uploaded_file inherently check for the existence of the file?

if ($this->isValid()) {
    return file_get_contents($this->getPathname());
}

@sisve

sisve commented Jul 23, 2018

Copy link
Copy Markdown
Contributor

I think you need both. The isValid() check doesn't call is_uploaded_file when you've created an UploadedFile for testing (it's a parameter in the constructor), so it would be nice to keep the error message for those cases to handle missing fixtures and such.

the `isValid()` method checks whether the file was uploaded successfully, and also calls `is_uploaded_file()`, which validates the file has been uploaded, and helps prevent mailcious users from reading other files on the system.

there is no need to check for the file existence because the constructor on `Symfony\Component\HttpFoundation\File\File` checks if the passed path is a file using `is_file()`.

also updated test
@browner12

Copy link
Copy Markdown
Contributor Author

@sisve after digging in a bit more, I found we already check for a valid file here:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/File.php#L36

therefore I just went with isValid() for this method

@browner12

Copy link
Copy Markdown
Contributor Author

what does setup = lowest mean on the travis build? is that referring to PHP versions, or composer dependencies?

@browner12

Copy link
Copy Markdown
Contributor Author

found it. debugging now why it passes on the normal run, but fails on the 'lowest'

Symfony changed the signature of their constructor when switching from `4.0` to `4.1`, which caused the tests to fail on the 'lowest' composer depenedencies.  This commit switches the test from using the `4.1` signature to the `4.0` signature so it passes for all users in the range of dependencies.  IMO, this was a BC break on Symfony's part that should never have happened, but we'll adjust to satisfy it.

symfony/http-foundation@4.0...4.1#diff-6c8d56052f6c60629eaaa4fca7887288L58
@browner12

Copy link
Copy Markdown
Contributor Author

Symfony changed the signature of their constructor when switching from 4.0 to 4.1, which caused the tests to fail on the 'lowest' composer depenedencies. This commit switches the test from using the 4.1 signature to the 4.0 signature so it passes for all users in the range of dependencies. IMO, this was a BC break on Symfony's part that should never have happened, but we'll adjust to satisfy it.

symfony/http-foundation@4.0...4.1#diff-6c8d56052f6c60629eaaa4fca7887288L58

@taylorotwell
taylorotwell merged commit 94d6b00 into laravel:5.6 Jul 27, 2018
@browner12
browner12 deleted the uploaded-file-contents branch July 27, 2018 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants