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.

Sign up for GitHub

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

Jump to bottom

Remove videopress local files #6590

Merged
merged 5 commits into from
Mar 28, 2017
Merged

Remove videopress local files #6590

merged 5 commits into from
Mar 28, 2017

Conversation

dbtlr
Copy link
Contributor

@dbtlr dbtlr commented Mar 7, 2017 •
edited
Loading

This PR removes the use of locally downloaded (sideloaded) video files from the user's server and instead uses an attachment URL that points to the processed file for any video headers.

Fixes #6588

To test

  1. Upload a video
  2. Test that it can be embedded in a post
  3. Verify that it can be added as the Video header for the 2017 theme.

@dbtlr dbtlr added [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Mar 7, 2017
@dbtlr dbtlr requested a review from jeherve March 7, 2017 14:57
zinigor
zinigor requested changes Mar 8, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Tested, found one strange problem: when I upload a video, shortly after it's done, but before it's processed, I get false as the video URL. Is that related to this PR?
Plus there are two comments in the code.

modules/videopress/class.jetpack-videopress.php Outdated
*
* TODO: Fix this so that it will return a VideoPress process url, to ensure that it is in MP4 format.
* This is most a action proxy to the videopress_get_attachment_url() utility function.
Copy link
Member

Choose a reason for hiding this comment

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

I think this sentence could use some rephrasing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

modules/videopress/class.videopress-xmlrpc.php
// Get the attached file and if there isn't one, then let's update it with the one from the server.
$file = get_attached_file( $id );
if ( ! $file && is_string( $info['original'] ) ) {
videopress_download_video( $info['original'], $id );
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can remove the videopress_download_video function after this? There will be no code calling it, would there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this for @georgestephanis, since he's a big proponent of downloaded videos. I'm not so sure about them, though. I'll delete as we can always pull this back out of source control later if needed.

@dbtlr dbtlr force-pushed the fix/remove-videopress-local-files branch from ca7dc92 to f2ff5ee Compare March 10, 2017 02:29
@dbtlr
Copy link
Contributor Author

dbtlr commented Mar 10, 2017

@zinigor I added a check and now use the original file when transcoding isn't completed and this condition occurs.

@jeherve jeherve added [Feature] VideoPress A feature to help you upload and insert videos on your site. [Pri] High labels Mar 10, 2017
@dbtlr
Copy link
Contributor Author

dbtlr commented Mar 28, 2017

@zingor @jeherve: Can I get a final review and merge?

@zingor: Your false issue is a no. All of this is triggered as part of the backend transcode jobs. Not as part of your query.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 28, 2017
zinigor
zinigor approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Thanks for the additional edits, I have tested again, works well! This time I didn't experience that problem with the false, but since it's not related to this PR, I'm going to test some more for the beta and create a separate issue if it occurs again.

@rcoll
Copy link
Member

rcoll commented Mar 28, 2017

@dbtlr This LGTM as-is but needs a rebase. Once that's done I'll give it a quick test and get it merged :-)

@jeherve jeherve force-pushed the fix/remove-videopress-local-files branch from 9259dc2 to 4d1966d Compare March 28, 2017 21:28
jeherve
jeherve approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well in my tests! I rebased, and will merge as soon as Travis completes.

@jeherve jeherve merged commit 7ac48d6 into master Mar 28, 2017
@jeherve jeherve deleted the fix/remove-videopress-local-files branch March 28, 2017 21:41
@jeherve jeherve removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 28, 2017
jeherve added a commit that referenced this pull request Mar 28, 2017
@jeherve
Changelog: add #6590
a6bddd9
samhotchkiss pushed a commit that referenced this pull request Mar 29, 2017
@jeherve @samhotchkiss
Jetpack 4.8: changelog. (#6613)
0d49aa5 
* Readme: remove old release and add skeleton for 4.8.

* Changelog: add #6572

* Changelog: add #6567

* Changelog: add #6542

* Changelog: add #6527

* Changelog: add #6508

* Changelog: add #6478

* Changelog: add #6477

* Changelog: add #6249

* Update stable version and remove old version from readme.

* Changelog: add 4.7.1 to changelog.

* Readme: add new contributor.

* Sync: update docblock @SInCE version.

Related: #6053

* Changelog: add release post.

* changelog: add #6053

* Changelog: add #6413

* Changelog: add #6482

* Changelog: add #6584

* Changelog add #6603

* Changelog: add #6606

* Changelog: add #6611

* Changelog: add #6635

* Changelog: add #6639

* Changelog: add #6684

* Changelog: add #6710

* Changelog: add #6711

* Changelog: add #5461

* Testing list: update Settings UI feedback prompt.

Props @MichaelArestad

* Changelog: add #6789

* Changelog: add #6778

* Changelog: add #6777

* Changelog: add #6775

* Changelog: add #6755

* Changelog: add #6731

* Changelog: add #6721

* Changelog: add #6705

* Changelog: add #6702

* Changelog: add #6671

* Changelog: add #6637

* Changelog: add #6582

* Changelog: add #6566

* Changelog: add #6555

* Changelog: add #6529

* Changelog: add #6344

* Changelog: add #5763

* Changelog: add #5503

* Changelog: update #6637 changelog.

@see 40e115c#commitcomment-21523982

* Changelog: add #6699

* Changelog: add #6632

* Changelog: add #6769

* Changelog: add #6707

* Changelog: add #6590
@jeherve jeherve mentioned this pull request Apr 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@dbtlr dbtlr

Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Pri] High [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dbtlr @rcoll @zinigor @jeherve