Skip to content
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

Replacing our Tus upload integration with TusPHP. #186

Merged
merged 4 commits into from
Nov 15, 2018

Conversation

erunion
Copy link
Contributor

@erunion erunion commented Nov 15, 2018

  • Replace our Tus upload integration with https://github.com/ankitpokhrel/tus-php.
  • Changes our PHP requirement from >=5.3.0 to >=7.1.0. Had to do this because tus-php requires 7.1+.
  • Loading Psalm and setting it up to execute static analysis checks during the test process.

With these changes, I was able to upload video files that ranged from 13-793MB large. Since the default PHP memory limit is 128MB, we're uploading in 100MB chunks, so anything larger than what I've tested with should function just as well.

Since this is a breaking change, this'll be tagged released as v3.0.

Resolves #168

@erunion erunion changed the title Replacing our Tus upload integration with ankitpokhrel/tus-php. Replacing our Tus upload integration with TusPHP. Nov 15, 2018
Copy link

@Onebrownsound Onebrownsound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice to see parity between the SDKs. One minor comment, but other than that gj!

'Upload-Offset: {placeholder}',
)
);
$base_url = str_replace($url_path, '', $url);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay for now, imo. But maybe mark this as a #TODO to export into it's own static function that takes in the path and returns all the necessary pieces, as a list or something. For testability and such.

@erunion erunion changed the base branch from master to v3-dev November 15, 2018 20:57
@erunion erunion merged commit 573c6a4 into v3-dev Nov 15, 2018
@erunion erunion deleted the better-tus-integration branch November 15, 2018 20:57
@erunion erunion mentioned this pull request Nov 15, 2018
Merged
3 tasks
@ghost
Copy link

ghost commented Jan 9, 2019

I'm using PHP 7.2 and Vimeo 3.0 but I can't upload big files using tus approach (PATCH).
I receive a message like this: SSL_write() returned SYSCALL, errno = 32
I can complete the upload only if I use very small videos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants