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

Reverse old embed HTML if it has videopress URLs in it #14249

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented Dec 17, 2019

Older versions of VideoPress may have embedded HTML in a post in this format:

<p><embed src="http://v.wordpress.com/somevideoid" type="application/x-shockwave-flash" width="600" height="338"></embed></p>

Most browsers now block flash by default, so this old content cannot be viewed.

This patch fixes the issue in a minimal way by looking for embed codes with URLs matching v.videopress.com and extracting the ID from the URL.

It makes no attempt to extract the width and height. Nor does it "fix" your post by permanently changing the stored content. It simply intercepts the HTML during the_content and renders the shortcode, which then gets expanded to the full videopress tag at the default width and height.

Changes proposed in this Pull Request:

  • Convert <embed src="http://v.videpress... markup into [videpress SomEVideOiD] shortcodes

Testing instructions:

  • Create a VideoPress video and note the ID
  • Edit raw HTML and add some markup like this:
<p><embed src="http://v.wordpress.com/[YOUR VIDEO ID]" type="application/x-shockwave-flash" width="600" height="338"></p>
  • View the post, output should be the HTML5 video embed

Proposed changelog entry for your changes:

  • Automatically render old videopress flash embed code as HTML5 player

@gravityrail gravityrail requested a review from a team December 17, 2019 03:33
@gravityrail gravityrail added [Type] Bug When a feature is broken and / or not performing as intended [Feature] VideoPress A feature to help you upload and insert videos on your site. labels Dec 17, 2019
@jetpackbot
Copy link

jetpackbot commented Dec 17, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 52cb06f

@kraftbj
Copy link
Contributor

kraftbj commented Dec 17, 2019

What would you think about doing this same thing also when opening a post up for editing and/or on save? It'd organically fix older posts if/when they are ever edited while still catching those that aren't?

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.

I like Kraft's idea of fixing old posts, although I assume this may not be used much since I assume few folks end up editing old posts.

I do have some remarks below.

modules/videopress/shortcode.php Outdated Show resolved Hide resolved
modules/videopress/shortcode.php Outdated Show resolved Hide resolved
modules/videopress/shortcode.php Outdated Show resolved Hide resolved
@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Dec 17, 2019
@dereksmart dereksmart added this to the 8.1 milestone Dec 17, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Dec 17, 2019

Suggestion:

  • For this PR, convert the old embed to an oEmbed URL.
  • Filter it on the_content at some mutually agreed point. I personally like the idea of providing it wide and far (e.g. anytime Jetpack is active).
  • Lay aside for now my idea for converting it on editing/save/etc for a future PR in interest of not delaying implementation. Also, it allows us more time to research any other variations of the embed without having data change along the way.
  • Adding unit tests with an instance that matches. @jeherve what other tests to you suggest?

@kraftbj
Copy link
Contributor

kraftbj commented Dec 17, 2019

I've updated the PR to use oEmbed and filter the_content more broadly. No unit tests yet.

Though, my initial testing suggests Core's oEmbed for VideoPress URLs is not working as expected -- they aren't loading with Jetpack active unless the VideoPress module is enabled. I'd like to investigate that before landing this PR.

@kraftbj
Copy link
Contributor

kraftbj commented Dec 17, 2019

I'd like some extra eyes to make sure this works. I think the oEmbed cache was getting in the way due to trying to embed a VideoPress that hadn't fully encoded yet and this works fine as is.

@kraftbj kraftbj added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 18, 2019
@kraftbj kraftbj self-assigned this Dec 18, 2019
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.

I like this, it tests well and I think modules/videopress/utility-functions.php is just what we need here. This should be good to merge!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 18, 2019
@kraftbj kraftbj merged commit 3a0fb0b into master Dec 18, 2019
@kraftbj kraftbj deleted the try/fix-old-videopress-embeds branch December 18, 2019 17:15
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 18, 2019
@gravityrail
Copy link
Contributor Author

gravityrail commented Dec 18, 2019

Thank you @kraftbj and @jeherve for jumping on this and doing a great job.

jeherve added a commit that referenced this pull request Dec 20, 2019
zinigor added a commit that referenced this pull request Dec 30, 2019
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
zinigor added a commit that referenced this pull request Dec 30, 2019
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] VideoPress A feature to help you upload and insert videos on your site. [Type] Bug When a feature is broken and / or not performing as intended [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants