Remove zero size special case for copy_to_stream #6807
Conversation
|
Looks like we have a test for copying from an empty stream here: https://github.com/php/php-src/blob/07fa13088e1349f4b5a044faeee57f2b34f6b6e4/ext/standard/tests/streams/bug47997.phpt I didn't spot one for empty files in particular though, so I'll add one. |
|
Should this be considered as a bugfix, thus for 7.4? |
nikic
force-pushed
the
stream-copy-size-check
branch
from
9ed1d94 to
5f63fd8
Compare
|
I just found that this (22517ce) was introduced as bug fix, where a comment in the bug report claims it would be an issue for platforms not supporting mmap(). However, when I tested on Windows, php_stream_mmap_range() failed, but the rest worked well, so maybe this has been resolved otherwise in the meantime. |
|
Maybe not relevant, but the end of the function says: |
|
@nicolas-grekas Good point. This code doesn't actually get reached when the size is zero, but the comment is certainly confusing, and the special handling there is unnecessary. I've simplified that code a bit and dropped the comment. |
This doesn't seem to serve a purpose anymore. Stats are expensive, so doing an unnecessary stat just to short-circuit the zero size case is rather dubious. It can also break with stream wrappers that return inaccurate sizes (symfony/symfony#40574) and probably can also break with stream filters. Drop the special case and adjust code to make it more obvious that it will still be handled correctly. Closes phpGH-6807.
This doesn't seem to serve any purpose. Stats are expensive, so doing an unnecessary stat just to short-circuit the zero size case seems rather dubious. It can also break with stream wrappers that return inaccurate sizes (symfony/symfony#40574) and probably can also break with stream filters. I think we should just drop it...