[WIP] Use stream wrapper that allows to seek HTTP files #11000
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/ |
|
This contribution is MIT licensed (one of the files is put in the public domain). |
| if ($this->certPath) { | ||
| curl_setopt($curl, CURLOPT_CAINFO, $this->certPath); | ||
| $url .= "&capath=" . urlencode($this->certPath); |
There was a problem hiding this comment.
Instead of messing with urlencoding it's a lot easier to pass all the options a stream context
|
We might want to add unit tests in |
|
@lokeller have you found how to run the external storage unit tests ? |
|
Only enable it locally, then run |
|
Wouldn't it be easier to wrap an existing http stream and make it seekable by buffering it |
|
Wrapping an existing http stream would be for sure simpler and cleaner but I'm not sure how to handle this part: I need to do it to make the code functionally equivalent to what I'm substituting. |
| rewind($fp); | ||
| return $fp; | ||
| $params = array( 'seekablehttp' => $options ); | ||
| error_log($params); |
There was a problem hiding this comment.
debug code - please remove
|
this code can only go in with a proper unit test - THX |
|
@PVince81 thanks for the pointers to the unit testing stuff. I will try to develop unit tests for this. |
|
@icewind1991 any comment on #11000 (comment) ? This needs rebase + unit tests. |
|
I did a rebase but I didn't have time yet to work on the unit tests. These will come eventually :) |
In external storage we want to start pushing to our clients the content
of files stored on a remote DAV server while we are downloading them from
the server but we also need to seek these files. For this reason we
cannot use fopen('http:/...') and instead we need a custom stream wrapper.
|
Addressed comment from @DeepDiver1975 |
|
Thanks 😄 |
| return FALSE; | ||
| } | ||
|
|
||
| $available_count = min( $this->read_bytes - $this->position, $count); |
There was a problem hiding this comment.
Instead of returning "to little data" I would prefer to do $this->wait_for_position($this->position+$count)
|
Could you use camelCase instead of underscores |
This addresses commment from @icewind1991
This addresses comment from @icewind1991
|
Addressed easy comments. Still missing unit tests, I need to first understand how they work. |
|
The inspection completed: 53 new issues, 15 updated code elements |
LukasReschke
changed the title
|
Can one of the admins verify this patch? |
|
@owncloud-bot ok to test |
|
@icewind1991 is this PR a good alternative to your older PR #10620 ? |
|
I would still prefer having a generic caching stream wrapper that takes an existing non-seekable stream and makes it seekable. |
|
Might be related #18653 |
|
Yes, the objective of my patch was the same as #18653 . The new implementation is clearly better because reuses code already present in the tree. I'm retracting my pull request. |
We want to start pushing to client connecting to owncloud the content of files stored on a remote DAV server while we are still downloading
it from the remote server without waiting to first fully download the file on the owncloud server. We cannot directly use fopen('http://...') because
we want to be able to seek the resource, instead we use the stream wrapper implemented here.
Some things on which feedback would be very useful: