Add support for http range #14903
Add support for http range #14903
Conversation
|
Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/ |
|
The inspection completed: 3 new issues, 10 updated code elements |
|
nice. |
| * @param string $path | ||
| * @param string $httpRange | ||
| */ | ||
| public function setFileRange($path, $httpRange) { |
There was a problem hiding this comment.
API-wise this is the wrong approach - the range is unrelated to the read operation
There was a problem hiding this comment.
The range should be passed to readfile instead
|
@watermark thanks a lot for your contribution - please respect that any new code addition requires unit tests as declared in b533aad#diff-6a3371457528722a734f3c51d9238c13 Furthermore: we have been discussing range support in the past - because we cannot ensure the file content not two change between two http requests we don't want this to be implemented at the moment. |
|
On the API point, I'm not sure where else it would be appropriate. You must do a fseek on the stream, which must be done after the fopen and before the first fread. Furthermore, to support the end param of the partial content request, the fread buffer size must be conditionally changed in the fread loop. I can see a case for abstracting the range out of readfile, such that "readfile($filename)" changes to "readfile($filename, $start = false, $end = false)". Is that more what you're looking for? On the point of content changing between two requests. I believe browsers send the if-modified-since header along with a request for partial content, would honoring that header be sufficient to handle this concern? Or are you looking for something more complex, such as preventing writes to the file while it's being downloaded? |
If this is 100% sure to be the case - I'm fine with it - but I doubt that |
that would make even more sense |
I've verified that Firefox sends the "If-Unmodified-Since" header and IE11 sends the "If-Range" header, either header should be sufficient to accomplish what you want. Chrome's resume support is pretty bad. Chrome does offer a pause/resume function, but it functions by just keeping the connection alive, it doesn't actually do a true range request. If you pause a download in chrome and close chrome, the download cannot be resumed at all. It will send range requests for time skipping steaming videos, but does not send any headers useful to address your changing content concern. Which browsers need to be verified to make everyone content enough to declare "100%"? In general, as long as OC honors the "If-Unmodified-Since" and the "If-Range" header, I think that's plenty. |
|
Can one of the admins verify this patch? |
|
Have you tried using the Webdav endpoint instead ? http://example.com/owncloud/remote.php/webdav/path/to/file ? AFAIK it should support range requests. |
|
Please use the Webdav endpoint instead, it already supports range requests. |
OC does not currently offer native support the HTTP_RANGE header. This header allows for pause/resume of downloads and time skipping of streaming videos (from the video app).
OC does offer partial support by allowing the use of the xsendfile apache module, but in addition to requiring the additional module, it has limited use. OC's xsendfile implementation only supports local files and doesn't seem to work properly in Firefox and IE (likely due to issues in xsendfile). XSendFile is also difficult to get working with OC when PHP is run in CGI mode.
The provided patch adds native support for the HTTP_RANGE header. It supports local files and it also seems to support files supplied by the external storage app. I say "seems to" because I have only tested a few.
This code released under MIT license, some code ported from CakePHP (MIT License).