-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for multipart range requests according to rfc 7233 #1418
base: master
Are you sure you want to change the base?
Conversation
https://github.com/sabre-io/dav/actions/runs/3134827626/jobs/5090168461
and commit the code-style changes that it makes. That will get past the first bit of CI. |
Then try:
and sort out getting the unit tests to pass, and add some unit tests to cover new functionality. |
Excuse the noob question, but how do I set up the local server for the unit tests? I tried using php -S localhost:8000 in the tests folder (elevated and non elevated console) and failed a lot of them that shouldn't have anything to do with what i wrote. Usually it was some access forbidden or resource not available errors. |
After having done In a terminal from the root of the
Then run the tests in another terminal window:
That gets rid of 3 errors that I had at first. |
Thanks for your help! I did that and it did indeed fix some issues. I still get loads of errors and failures However, i get even more errors when i try to run the unit tests of the original repo. So I guess it's my work environment. I decided to ignore most of them now and only took care about the ones directly affecting get requests and ranges, fixed them and added some unit tests for multipart stuff. |
Codecov Report
@@ Coverage Diff @@
## master #1418 +/- ##
============================================
+ Coverage 97.23% 97.26% +0.03%
- Complexity 2830 2858 +28
============================================
Files 175 175
Lines 8566 9503 +937
============================================
+ Hits 8329 9243 +914
- Misses 237 260 +23
... and 87 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hey, could someone please approve the workflow again? i updated code and tests to make the codecov bot happier |
I am interested in that feature too. Plz approve ... Many thanks to Daniel Phil, would be nice to approve soon and finish this. Thank you |
Hi Phil, are you going to merge soon? |
Please merge asap .... thanks |
lib/DAV/Server.php
Outdated
* If the first offset is null, the second offset should be used to retrieve the last x bytes of the entity | ||
* | ||
* @return int[]|null | ||
* @return string|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the signature of a public function which breaks BC ...
... we need to see how to work around this ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
- keep
getHTTPRange
unchanged - mark it as deprecated
- add new method
getHttpRangeString
(better naming accepted) - base the new implementation in core plugin upon the new method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your feedback! I have added the changes as per your suggestion.
…med new function to getHTTPRangeHeaderValue, moved preprocessRanges to CorePlugin
This implementation of the WebDav standard was missing multipart range request support. These kinds of requests are important for speeding up NextCloud file access of large binary files.