Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

kienstra · 2019-11-11T03:46:50Z

Summary

This moves the AMP handling of the [youtube] shortcode from the AMP plugin to Jetpack. Very similar to #13921, but for [youtube] instead of [vimeo].

Fixes ampproject/amp-wp#3309

Changes proposed in this Pull Request:

Apply the [youtube] shortcode handling from the AMP plugin
Only refactor existing non-AMP [youtube] shortcode logic for reuse with the AMP shortcode, no intended change to it

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a new feature for Jetpack, but moved from the AMP plugin

Testing instructions:

Ensure that wp-config.php has define( 'JETPACK_DEV_DEBUG', true);
In /wp-admin, in the Jetpack 'Settings' page, and in the 'Writing' tab, ensure this is toggled on:

Fetch this PR's branch
Clone the AMP plugin

$ git clone --recursive https://github.com/ampproject/amp-wp.git amp && cd amp

Fetch this PR of the AMP plugin, which deactivates the AMP plugin's [youtube] shortcode callback: Remove handling of Jetpack shortcodes ampproject/amp-wp#3678
Do $ npm install && composer install
Activate the AMP plugin
Create a new post with a Shortcode block
Add a shortcode, like:

[youtube https://www.youtube.com/watch?v=_Oh12ROTQCE]

Preview the front-end of the AMP URL, like by adding &amp or ?ampto the URL, and look at how the Vimeo shortcode looks
checkout the master branch of Jetpack, not this PR's branch
Preview the front-end of the AMP URL, and notice how the Vimeo shortcode looks
Expected: The shortcode should look the same, whether using this PR's branch, or the Jetpack plugin's master branch
Test more shortcodes under the More Examples heading here

Before

After

Proposed changelog entry for your changes:

Migrate AMP-conversion of [youtube] shortcode to Jetpack from the AMP plugin

In continuation of the [vimeo] shortcode AMP migration. The intent is to not change how the rendering worked in the AMP plugin, only to migrate it to Jetpack.

To follow coding standards, ensure the array() ends with a ,

jetpackbot · 2019-11-11T03:49:08Z

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: November 19, 2019.
Scheduled code freeze: November 12, 2019

Generated by 🚫 dangerJS against 3a67936

This should actually be '', as that is the behavior of the AMP plugin before.

kienstra · 2019-11-11T04:24:35Z

Separate PRs?

Hi @jeherve,
Hope you had a great weekend.

Is it alright to open a separate PR for each shortcodes to migrate to Jetpack?

If it's alright, there are 4 more AMP shortcodes after this that we'd like to migrate to Jetpack.

Though this isn't urgent, I know reviewing them can be a lot of work.

Also, let me know if you'd like me to fix the Code Climate issues.

Thanks, Jeremy!

jeherve · 2019-11-11T14:24:52Z

Is it alright to open a separate PR for each shortcodes to migrate to Jetpack?

That's absolutely fine, yes. In fact, separate PRs are better I think, they're easier to review and to work on.

modules/shortcodes/youtube.php

As Jeremy mentioned, jetpack_get_youtube_id() should work in place of the custom function get_amp_youtube_id_from_url(). It actually passes the unit test: test_get_amp_youtube_id_from_url().

modules/shortcodes/youtube.php

As Jeremy mentioned, the <a> with the fallback URL didn't have a closing </a> before.

As Jeremy mentioned, this logic was run in 2 functions. So move this to the top of youtube_shortcode().

As Jeremy mentioned, it doesn't only get query args.

As Jeremy mentioned, if there's no $id, there's a comment with an error message. So do something similar if the $args are empty.

jeherve

Nice edits, we are getting there I think. I only have one remark right now.

modules/shortcodes/youtube.php

Co-Authored-By: Jeremy Herve <jeremy@tagada.hu>

Also, pass another argument to sprintf(). And update a few tests.

jeherve

This looks good to me. I'll merge on Monday after testing on WordPress.com as well. Thank you!

matticbot · 2019-11-15T18:43:21Z

Caution: This PR has changes that must be merged to WordPress.com
Hello kienstra! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D35551-code before merging this PR. Thank you!

kienstra · 2019-11-15T21:41:52Z

Thanks, Jeremy! Have a great weekend.

WordPress.com does not run the latest version of the AMP plugin yet.

matticbot · 2019-11-18T11:46:12Z

kienstra, Your synced wpcom patch D35551-code has been updated.

* 8.0 Release: running changelog * Changelog: add #13921 * Changelog: add #13980 * Changelog: add #13905 * Changelog: add #13971 * Changelog: add #13984 * Changelog: add #14009 * Changelog: add #13620 * Remove things that will ship in 7.9.1 * Changelog: add 7.9.1 release (#14044) * Changelog: add base for 7.9.1 release * Update release date and post link * Changelog: add #14066 * Update changelog for 7.9.1 * Changelog: add #13405 * Changelog: add #13841 * Changelog: add #13924 * Changelog: add #13986 * Changelog: add #14010, #14028, #14053, #14055. * Changelog: add #14054 * Changelog: add #14031 * Changelog: add #14039 * Changelog: add #14050 * Changelog: add #14070 * Changelog: add #14082 * Changelog: add #14084 * Changelog: add #14111 * Changelog: add #13961 * Changelog: add #14047 * Changelog: add #14091 * Changelog: add #14108 * Changelog: add #14121

Move the AMP rendering of [youtube] shortcode to Jetpack

80aaf25

In continuation of the [vimeo] shortcode AMP migration. The intent is to not change how the rendering worked in the AMP plugin, only to migrate it to Jetpack.

kienstra requested a review from a team November 11, 2019 03:46

matticbot added the Touches WP.com Files label Nov 11, 2019

Add a trailing , to end of array()

8775440

To follow coding standards, ensure the array() ends with a ,

Correct failed unit tests for 'no_attributes'

1594536

This should actually be '', as that is the behavior of the AMP plugin before.

jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it AMP [Feature] Shortcodes / Embeds labels Nov 11, 2019

jeherve assigned jeherve and kienstra Nov 11, 2019

jeherve added this to the 8.0 milestone Nov 11, 2019

jeherve requested changes Nov 11, 2019

View reviewed changes

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

jeherve 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 11, 2019

Remove custom YouTube ID function in favor of existing one

d7c0564

As Jeremy mentioned, jetpack_get_youtube_id() should work in place of the custom function get_amp_youtube_id_from_url(). It actually passes the unit test: test_get_amp_youtube_id_from_url().

jeherve 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 Nov 12, 2019

jeherve requested changes Nov 12, 2019

View reviewed changes

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

jeherve 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 12, 2019

kienstra added 4 commits November 12, 2019 14:45

Close the fallback link

08c760b

As Jeremy mentioned, the <a> with the fallback URL didn't have a closing </a> before.

Only run URL logic once

51a5eb7

As Jeremy mentioned, this logic was run in 2 functions. So move this to the top of youtube_shortcode().

Rename jetpack_shortcode_youtube_query_args()

eb7c5e7

As Jeremy mentioned, it doesn't only get query args.

Return an error comment if the $args are empty, like above

bed7e2c

As Jeremy mentioned, if there's no $id, there's a comment with an error message. So do something similar if the $args are empty.

jeherve requested changes Nov 14, 2019

View reviewed changes

modules/shortcodes/youtube.php Outdated Show resolved Hide resolved

Commit Jeremy's suggestion to add the URL as a child of the <a>

54be87b

Co-Authored-By: Jeremy Herve <jeremy@tagada.hu>

Update unit tests for the previous commit

131840b

Also, pass another argument to sprintf(). And update a few tests.

kienstra mentioned this pull request Nov 14, 2019

Migrate Jetpack shortcode handling out of AMP plugin and into Jetpack ampproject/amp-wp#3309

Closed

jeherve 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 Nov 15, 2019

jeherve previously approved these changes Nov 15, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 15, 2019

Skip AMP tests on WordPress.com

3a67936

WordPress.com does not run the latest version of the AMP plugin yet.

jeherve dismissed their stale review via 3a67936 November 18, 2019 11:45

jeherve approved these changes Nov 18, 2019

View reviewed changes

jeherve merged commit 2d311f3 into Automattic:master Nov 18, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 18, 2019

jeherve mentioned this pull request Nov 18, 2019

Shortcodes: simplify YouTube AMP fallback link tag generation #14061

Merged

kienstra deleted the add/youtube-shortcode-amp branch November 19, 2019 03:32

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Nov 25, 2019

jeherve added a commit that referenced this pull request Nov 25, 2019

Changelog: add #14010, #14028, #14053, #14055.

1f5efa5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

kienstra commented Nov 11, 2019

jetpackbot commented Nov 11, 2019 •

edited

Loading

kienstra commented Nov 11, 2019

jeherve commented Nov 11, 2019

jeherve left a comment

jeherve left a comment

matticbot commented Nov 15, 2019

kienstra commented Nov 15, 2019

matticbot commented Nov 18, 2019

Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

Move AMP rendering of [youtube] shortcode to Jetpack from AMP plugin #14010

Conversation

kienstra commented Nov 11, 2019

Summary

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Before

After

Proposed changelog entry for your changes:

jetpackbot commented Nov 11, 2019 • edited Loading

kienstra commented Nov 11, 2019

jeherve commented Nov 11, 2019

jeherve left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

matticbot commented Nov 15, 2019

kienstra commented Nov 15, 2019

matticbot commented Nov 18, 2019

jetpackbot commented Nov 11, 2019 •

edited

Loading