-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
Conversation
- `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
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 |
renamed `fixtures` folder to keep it consistent with other test folders, forgot to update it here.
Didn't know that function existed, good to know. I like taking advantage of the existing if ($this->isValid()) {
return file_get_contents($this->getPathname());
} |
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
@sisve after digging in a bit more, I found we already check for a valid file here: therefore I just went with |
what does |
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
Symfony changed the signature of their constructor when switching from symfony/http-foundation@4.0...4.1#diff-6c8d56052f6c60629eaaa4fca7887288L58 |
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