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 CreditAttribution: 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 CreditAttribution: Damien Tournoud commentedJust for the record: PHP--
Comment #3
chx CreditAttribution: chx commentedWe need a test for this.
Comment #4
chx CreditAttribution: chx commentedComment #5
AaronBaumanconfirmed that this fixes #788102: jpg images do not display for image fields, jpg derivitaves not created
Comment #6
Dries CreditAttribution: Dries commentedI committed #3 but marking it as 'needs work' because we want tests.
Comment #7
asimmonds CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #11
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #12
sun.core CreditAttribution: 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 CreditAttribution: lotyrin commentedThis was normal (major at the highest, never critical), and unless I'm mistaken is just needing tests.
Comment #16
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #19
chx CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: lotyrin commentedFixing version.
The comment in #21 shouldn't be ignored however.
Comment #23
lotyrin CreditAttribution: 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 CreditAttribution: thedavidmeister commentedpatch no longer applies, simpletest.info does not exist.