Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.6] get contents of uploaded file #24924

Merged
merged 4 commits into from
Jul 27, 2018
Merged

[5.6] get contents of uploaded file #24924

merged 4 commits into from
Jul 27, 2018

Conversation

browner12
Copy link
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
Copy link
Contributor

sisve commented Jul 22, 2018

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
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
Copy link
Contributor

sisve commented Jul 23, 2018

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
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
Contributor Author

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

@browner12
Copy link
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
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