The PHPdoc for stream_seek() says:
/**
* Support for fseek().
*
* @param $offset
* The byte offset to got to.
* @param $whence
* SEEK_SET, SEEK_CUR, or SEEK_END.
*
* @return
* TRUE on success.
*
* @see http://php.net/manual/en/streamwrapper.stream-seek.php
*/
and the current implementation of stream_seek() returns the result of a fseek() call, which is documented as having a different return value on http://www.php.net/manual/en/function.fseek.php:
fseek()
Return Values:
Upon success, returns 0; otherwise, returns -1. Note that seeking past EOF is not considered an error.
So, for success, stream_seek() should return TRUE but fseek() returns 0.
The bug is noticeable when the image API calls getimagesize() for a .jpg image that has some EXIF metadata that it has to skip over (using fseek())
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | 844676-streams_tests.patch | 4.17 KB | andypost |
| #24 | 844676-streams_tests.patch | 4.16 KB | andypost |
| #7 | 844676_tests.patch | 4.44 KB | asimmonds |
| #3 | php_is_a_sorry_mess.patch | 1.4 KB | chx |
| stream_wrappers_seek.patch | 662 bytes | asimmonds |
Comments
Comment #1
asimmonds commentedLooks like dir_rewinddir() and dir_closedir() could have similar problems, their equivalent non-stream functions both return void instead of a boolean.
Comment #2
damien tournoud commentedJust for the record: PHP--
Comment #3
chx commentedWe need a test for this.
Comment #4
chx commentedComment #5
aaronbaumanconfirmed that this fixes #788102: jpg images do not display for image fields, jpg derivitaves not created
Comment #6
dries commentedI committed #3 but marking it as 'needs work' because we want tests.
Comment #7
asimmonds commentedFirst try at some stream_wrappers.inc tests.
- Should these be true unit tests testing the class methods, or abstracted through the PHP filesystem functions like I've done here?
Comment #8
damien tournoud commentedMy guess would be that we can probably reuse some tests from PHP itself here. What we want is general tests for a stream-wrapper implementation, that can be easily reused by contrib.
Comment #9
andypostGreat start, suppose this needs more extended tests to catch bugs like file_uri_target() pointed in #278425-35: Using basename() is not locale safe
Comment #10
marcingy commentedChanging to major as per tag.
Comment #11
mustanggb commentedTag update
Comment #12
sun.core commentedAll that remains here are tests, and we'll always miss some tests - not major.
Comment #13
skyredwangThis patch was committed by someone. Mark fixed.
Comment #14
skyredwang#3 patch is committed, but it doesn't solve the problem.
Comment #15
lotyrin commentedThis was normal (major at the highest, never critical), and unless I'm mistaken is just needing tests.
Comment #16
Tor Arne Thune commentedComment #19
chx commentedGuys, #3 applies to D7. Requests for a backport are deleted.
Comment #20
andypost#3 already in 7 & 8 so only tests #7 should be backported
Comment #21
tac11tac commentedNewbie here... This issue is still showing in v7.9 when uploading .jpg images. #16 says a backport to v7 is needed, and #19 says that the commit in #3 was for the v7 branch. However, #3 looks like it only hit the v8 branch. Could someone validate this and hopefully patch v7 for the next release? Thanks.
Comment #22
lotyrin commentedFixing version.
The comment in #21 shouldn't be ignored however.
Comment #23
lotyrin commentedReverting other changes from #21
Comment #24
andypostRe-roll of tests
PS: @tac11tac and others - #3 has been commited a long ago! check the code base
Comment #26
andypostRe-roll with p1 and fixed remains of $Id$
Comment #27
thedavidmeister commentedpatch no longer applies, simpletest.info does not exist.
Comment #46
kim.pepperThis was fixed in 9.1.x. It was marked needs work for a backport to D7 however that is no longer supported. Marking fixed.
Comment #47
quietone commented@kim.pepper, thanks for finding this.
The commit was actually made to Drupal 7. Changing version and adding credit for that work.
Comment #48
quietone commented