jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Implements the php TUS client to upload videos from the Media Library

Open leogermani opened this issue 1 year ago • 6 comments

Note: We are still considering whether this will live in the package or in the plugin, Not merging just yet.

Changes proposed in this Pull Request:

  • Implements the php TUS client to upload videos from the Media Library directly to the VideoPress servers

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?

Jetpack product discussion

1202184803119163-as-1202583706992777

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Setting up:

First of all, upload a video file to your Media Library

  • Go to Media > Add new and upload a video
  • Make sure you don't upload the video to VideoPress.
  • On the Media Library, click the new video and find out the attachment ID

Testing

In order to test it, we are going to use WP CLI

  • launch the interactive shell with jetpack docker wp shell

Let's follow the happy path:

  • Create a new instance of the Uploader: $u = new Automattic\Jetpack\VideoPress\Uploader(92); where 92 is the ID of your video attachment
  • Check the status: $u->check_status()
  • You should see something like:

Captura de tela de 2022-07-27 18-16-07

  • Now start the upload with $u->upload()

  • It will upload your file in chunks of 1MB. You should see a response like this: Captura de tela de 2022-07-27 18-17-03

  • Call $u->upload() as many times as needed to upload your video completely. Hopefully your video is not to big and it will get only a couple of times

  • Each time it will upload 1MB and increse the value of bytes_uploaded

  • When the video is complete, you should see something like this:

>>> $u->upload();
=> [
     "status" => "complete",
     "bytes_uploaded" => 8589783,
     "upload_key" => "s-204213934-v-92",
     "uploaded_details" => [
       "guid" => "XXXXYYYZZZ"
       "media_id" => "156",
       "upload_src" => "https://videos.files.wordpress.com/XXXXYYYZZZ/file-name.mp4",
     ],
   ]
  • Nice! it worked!
  • Back on your browser, go to your Media Library and confirm you have a new attachment and that this attachment is a videoPress video with the file you just uploaded
  • Back to the CLI, let's check some additional stuff:
  • $u->is_uploaded() should return true
  • $u->get_uploaded_attachment_id() should return the ID of the new attachment created.
  • $u->check_status() and $u->upload() should return the same thing. Info that the file was already uploaded.

Note that you can close the shell and create a new instance of the Uploader at any time and the results should be the same.

Some unhappy paths

  • Try to initialize the Uploader with an ID of a post that is not an attachment, or an attachment that is not a video, and confirm it throws

  • To see the specific tasks where the Asana app for GitHub is being used, see below:
    • 0-as-1202583706992777

leogermani avatar Jul 27 '22 21:07 leogermani

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :warning: All commits were linted before commit.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team review" label and ask someone from your team review the code. Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 30, 2022.

Videopress plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 29, 2022.

github-actions[bot] avatar Jul 27 '22 21:07 github-actions[bot]

A couple of open higher-level questions:

  • this requires PHP 7 while the Jetpack plugin requires 5.6 still. What's the best way to safeguard to ensure we don't crosswires and either put something into Jetpack that wouldn't fly on older systems or degrade the functionality for those folks?
  • let's ensure we audit the tus package for security.

Noting for future ref that it is MIT, which is GPL-compat, so no problems on that front. https://github.com/ankitpokhrel/tus-php/blob/main/LICENSE

kraftbj avatar Aug 01 '22 16:08 kraftbj

hey @kraftbj thanks for jumping in. I'm about to post a p2 to centralize this conversation, but to answer your questions:

. The Uploader class has a "is_supported" method. The idea is to only enable this feature in sites that support it. And that lib will only be loaded in those cases (https://github.com/Automattic/jetpack/pull/25302/files#diff-d023f18f671b4132f279bf23ed098c3e7f2e85710c9036374e4253fbf9cde4d9R67) . We are still not sure if the new block that uses it will be shipped on Jetpack or only in VideoPress stand-alone plugin. This decision can change the approach a bit . Since it's MIT, we also have the option to incorporate the code and modify it to support PHP 5.6

let's ensure we audit the tus package for security.

Good point

leogermani avatar Aug 01 '22 17:08 leogermani

@kraftbj pe4Cmx-92-p2

leogermani avatar Aug 01 '22 17:08 leogermani

Regarding b3a33ff and 2d78db3, we've always allowlisted vendor/** directories that we needed in plugins, which seems to go back a long ways. With the ability to production-exclude anything that we really don't need and --no-dev working, it seems to me to be a solution without a problem that causes headaches to the development process.

Since it's early in the release cycle, we can check the production repo after merge and see the impact, then go from there.

kraftbj avatar Aug 10 '22 18:08 kraftbj

@kraftbj as per pe4Cmx-92-p2#comment-91 I removed all external dependencies, so I reverted tha changes to .gitattributes from this PR. Letting you know in case you want to do this in another PR

leogermani avatar Aug 12 '22 19:08 leogermani

I've tested the PR and it works pretty well! Nice work 🚀 Maybe getting another ✅ from other folks won't hurt.

retrofox avatar Aug 22 '22 20:08 retrofox