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

Youtube Shortcode: Updates regex #10224

Merged
merged 3 commits into from
Dec 3, 2018
Merged

Youtube Shortcode: Updates regex #10224

merged 3 commits into from
Dec 3, 2018

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Sep 26, 2018

Brings upstream changes from wp.com discussed in D17568-code and D17868-code.

Changes proposed in this Pull Request:

  • Updates the regex used in the Youtube embed sniffing to allow for some cases previously missed.

Testing instructions:

  • See D17568-code

Proposed changelog entry for your changes:

  • Updates YouTube regex for converting object era-embeds to a shortcode.

@kraftbj kraftbj added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 26, 2018
@kraftbj kraftbj added this to the 6.7 milestone Sep 26, 2018
@kraftbj kraftbj requested a review from a team as a code owner September 26, 2018 18:13
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D18859-code. (newly created revision)

@jetpackbot
Copy link

jetpackbot commented Sep 26, 2018 •
edited
Loading

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 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS

@kraftbj kraftbj added the DO NOT MERGE don't merge it! label Sep 26, 2018
@kraftbj
Copy link
Contributor Author

kraftbj commented Sep 26, 2018 •
edited by brbrr
Loading

Convo in p1537995658000100-slack-jetpack-crew , but before merging, we should run plenty of performance testing.

@kraftbj kraftbj self-assigned this Sep 26, 2018
@kraftbj kraftbj removed this from the 6.7 milestone Oct 25, 2018
georgestephanis
georgestephanis reviewed Nov 29, 2018
View reviewed changes
modules/shortcodes/youtube.php
$params = $match[1];

if ( 'ifr_regexp_ent' == $reg ) {
if ( in_array( $reg, array( 'ifr_regexp_ent', 'regexp_ent' ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be simpler for this to do a wp_endswith( $reg, '_ent' ) ?

@westi
Copy link
Contributor

westi commented Nov 30, 2018

There is a new version of code in D21652-code once that is reviewed we should sync this PR with the new code.

@dereksmart dereksmart added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 30, 2018
kraftbj and others added 2 commits December 3, 2018 13:42
@kraftbj @jeherve
Youtube Shortcode: Updates regex
eb9b2cb 
Brings upstream changes from wp.com discussed in D17568-code and D17868-code.
@westi @jeherve
Improve the matching of the youtube shortcode reversal again.
e4c966d 
Merges r184358-wpcom

In D17568-code and D17868-code we improved this code.
Those changes accidentally got reversed in D18511-code
I was reviewing #10224 which would merge the original
changes into Jetpack and noted the 's' modifier.
Unfortunately we can't include that here because it means on long but malformed post content we go into an
infinite loop because of the .*? in the RegEx.
This means we can't match objects/mangled objects which have a newline within them but we can match more
that we were

Diff: D21652-code
@kraftbj kraftbj requested a review from a team December 3, 2018 12:51
@matticbot
Copy link
Contributor

D21699-code. (newly created revision)

@jeherve
Copy link
Member

jeherve commented Dec 3, 2018

once that is reviewed we should sync this PR with the new code.

Took care of that in e4c966d

@pablinos
Copy link
Contributor

pablinos commented Dec 3, 2018

You're right @andfinally, they should be escaped. It does currently match:

<object data="https://www youtubescom/v/caWbt0IOOZk" type="application/x-shockwave-flash" width="400" height="300"><param name="movie" value="https://www.youtube.com/v/caWbt0IOOZk" /><param name="quality" value="high" /><param name="allowFullScreen" value="true" /><!-- Fallback content --><a href="https://www.youtube.com/watch?v=caWbt0IOOZk"><img height="300" src="https://img.youtube.com/vi/caWbt0IOOZk/0.jpg" width="400" />YouTube Video</a></object>

Although the resulting shortcode would correct this as it only uses the video ID, it's definitely worth fixing.

@jeherve
Copy link
Member

jeherve commented Dec 3, 2018

👍 Made the change in d3bd4ef

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed DO NOT MERGE don't merge it! [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 3, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 3, 2018
jeherve
jeherve approved these changes Dec 3, 2018
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.

🚢 Merging this.

@jeherve jeherve merged commit 89f6290 into master Dec 3, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 3, 2018
@jeherve jeherve deleted the sync/youtube branch December 3, 2018 19:07
jeherve added a commit that referenced this pull request Dec 19, 2018
@jeherve
changelog: add #10224
3ac1ff8
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
changelog: add #10224
a58e248
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Jetpack 6.9: Changelog and testing list (#10722)
8d91603 
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgestephanis georgestephanis georgestephanis left review comments

@westi westi westi approved these changes

@jeherve jeherve jeherve approved these changes

@pablinos pablinos Awaiting requested review from pablinos

@andfinally andfinally Awaiting requested review from andfinally

Assignees

@kraftbj kraftbj

Labels
[Feature] Shortcodes / Embeds Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
6.9
Development

Successfully merging this pull request may close these issues.
9 participants
@kraftbj @matticbot @jetpackbot @westi @jeherve @pablinos @georgestephanis @andfinally @dereksmart