Video Resumable uploader #489
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
| public function uploadVideo($target, $pathToFile, $accessToken, $maxTransferTries = 5) | ||
| { | ||
| $uploader = $this->getResumableUploader($accessToken, $maxTransferTries); | ||
| $endpoint = '/'.$target.'/videos'; |
There was a problem hiding this comment.
We shouldn't auto-suffix the endpoint with videos since it might not always be that and none of the other parts of the SDK auto-suffix endpoints. :)
There was a problem hiding this comment.
@SammyK uploading a video can only be done with an endpoint formated like /{node-id}/videos, so I think it's OK here, end-user will only need to give the node id.
There was a problem hiding this comment.
@yguedidi I see what you're saying. I'm also concerned that users will be reading the Graph docs and then try to enter the endpoint directly here. I for one wouldn't assume the SDK would add the /videos suffix for me. So I'm still against auto-suffixing. :)
There was a problem hiding this comment.
I'd prefer an easy to use signature with a good doc rather an opened signature to any endpoint with a pattern check in the definition :)
There was a problem hiding this comment.
Personally, I'm with @yguedidi here. This uploader is designed for uploading videos. If we specify clear that 'target' means user/page id in the doc, user won't pass in a full endpoint path in.
|
I did a quick scan a pointed out a few things. I think after @yguedidi has a look we should do some refactoring after it gets merged before we tag it as 5.1 :) |
| * @param int $target | ||
| * @param string $pathToFile | ||
| * @param string $accessToken | ||
| * @param int $maxTransferTries |
There was a problem hiding this comment.
Add some parameter descriptions here
There was a problem hiding this comment.
this comment apply to all other public methods :)
|
Also - can you add some docs? :) |
|
|
||
| while ($maxTransferTries > 0) { | ||
| try { | ||
| $res= $this->client->sendRequest($request)->getDecodedBody(); |
|
@yguedidi Maybe we should send PR's to @YuanhuiChen's fork? |
|
@SammyK it's a solution, but are we in a hurry? I think it's better to let @YuanhuiChen finish it, at least to learn more, as code review is an important part of the learning process :) |
|
@yguedidi No rush. Just thought we could submit some PR's to "talk code" instead of English as we try to figure out the best way to implement this feature since it's such a beast. :) But I'm OK with letting @YuanhuiChen do it without PR's from us as long as he has the time! |
|
@SammyK sure I talk code better than english haha, but we also can't have some time, you know that too |
|
Thank you, guys, especially for @YuanhuiChen to work on this task! I just left some comments in diff. We are pretty close, wow! |
|
I'm just the shepherd :) ... I am quite happy to see all the discussion and progress, and whenever you're all ready, we'll get 5.1 out. 👍 |
|
@YuanhuiChen will you be able to make the requested changes? Thanks |
|
@gfosco, I just heard yesterday that @YuanhuiChen already moved to another high priority task and she will not able to finish this diff. We are short of resource right now. Just wonder, is it possible to find some love from your side on this diff? Otherwise, we need to wait for another two weeks before we can find a new resource. |
|
I might have some time next week if we can't get anyone else on it. :) |
|
Great! Thanks, @SammyK. |
|
Hello @SammyK, how things going? :) |
|
Hey @codingtmd! Thanks for the ping! I didn't get a chance to look at this last week so I'm going to see what's involved with this tomorrow and let you know. :) |
|
Hey @YuanhuiChen! If I send a PR to your branch for this feature, will you be able to merge any changes so we can continue the discussion here? :) |
|
Hello @SammyK, @YuanhuiChen already left this project, so I'm not sure whether she will response or not. Is it possible to send a PR on the master?:) |
|
I think I'll try to create a new branch from @YuanhuiChen's fork so that all the existing commits will stay in place. Things got busy all of a sudden on my end but I'm still trying to work on this when I can. :) |
|
Moved this PR to #498 so we can close this one. :) |
Finally comes another update. Sorry I opened a new branch. You cannot see the old comments. I committed to some others' diff by mistake in the old branch.
The new changes:
Thanks again for your patience!