-
Notifications
You must be signed in to change notification settings - Fork 384
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
Remove handling of Jetpack shortcodes #3678
Changes from 7 commits
9436ac2
73b8b10
5ceb39b
26a22c7
92d9de1
a98f88c
85077be
b64392b
3e0e649
d0b73eb
0192ff5
93cfaba
fcda766
302d611
822ca4b
a4bcd1b
5d74750
00b2e2c
ec5257e
6f6d3f7
e2e2d5c
d53df79
bac7db1
672997e
e168813
1a4b700
9f9c2ff
2d5ae27
5097040
b300e9d
fdb463e
2d71b6b
ba51034
6ae3419
4726922
19e05de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,13 @@ | |
*/ | ||
class AMP_SoundCloud_Embed_Handler extends AMP_Base_Embed_Handler { | ||
|
||
/** | ||
* The tag (name) of the shortcode. | ||
* | ||
* @var string | ||
*/ | ||
const SHORTCODE_TAG = 'soundcloud'; | ||
|
||
/** | ||
* Default height. | ||
* | ||
|
@@ -23,9 +30,8 @@ class AMP_SoundCloud_Embed_Handler extends AMP_Base_Embed_Handler { | |
* Register embed. | ||
*/ | ||
public function register_embed() { | ||
if ( function_exists( 'soundcloud_shortcode' ) ) { | ||
// @todo Move this to Jetpack. | ||
add_shortcode( 'soundcloud', [ $this, 'shortcode' ] ); | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
add_shortcode( self::SHORTCODE_TAG, [ $this, 'shortcode' ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please mark the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, 3e0e649 does that. |
||
} | ||
add_filter( 'embed_oembed_html', [ $this, 'filter_embed_oembed_html' ], 10, 2 ); | ||
} | ||
|
@@ -34,9 +40,8 @@ public function register_embed() { | |
* Unregister embed. | ||
*/ | ||
public function unregister_embed() { | ||
if ( function_exists( 'soundcloud_shortcode' ) ) { | ||
// @todo Move this to Jetpack. | ||
remove_shortcode( 'soundcloud' ); | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
remove_shortcode( self::SHORTCODE_TAG ); | ||
} | ||
remove_filter( 'embed_oembed_html', [ $this, 'filter_embed_oembed_html' ], 10 ); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,13 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { | |
*/ | ||
const URL_PATTERN_TIMELINE = '#https?:\/\/twitter\.com(?:\/\#\!\/|\/)(?P<username>[a-zA-Z0-9_]{1,20})(?:$|\/(?P<type>likes|lists)(\/(?P<id>[a-zA-Z0-9_-]+))?)#i'; | ||
|
||
/** | ||
* The tag (name) of the shortcode. | ||
* | ||
* @var string | ||
*/ | ||
const SHORTCODE_TAG = 'vimeo'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in ec5257e. It turns out that the constant wasn't needed at all 😄 |
||
|
||
/** | ||
* Tag. | ||
* | ||
|
@@ -46,7 +53,9 @@ class AMP_Twitter_Embed_Handler extends AMP_Base_Embed_Handler { | |
* Registers embed. | ||
*/ | ||
public function register_embed() { | ||
add_shortcode( 'tweet', [ $this, 'shortcode' ] ); // Note: This is a Jetpack shortcode. | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See notes above for Soundcloud. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, applied in b64392b |
||
add_shortcode( self::SHORTCODE_TAG, [ $this, 'shortcode' ] ); | ||
} | ||
wp_embed_register_handler( 'amp-twitter', self::URL_PATTERN, [ $this, 'oembed' ], -1 ); | ||
wp_embed_register_handler( 'amp-twitter-timeline', self::URL_PATTERN_TIMELINE, [ $this, 'oembed_timeline' ], -1 ); | ||
} | ||
|
@@ -55,7 +64,9 @@ public function register_embed() { | |
* Unregisters embed. | ||
*/ | ||
public function unregister_embed() { | ||
remove_shortcode( 'tweet' ); // Note: This is a Jetpack shortcode. | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
remove_shortcode( self::SHORTCODE_TAG ); | ||
} | ||
wp_embed_unregister_handler( 'amp-twitter', -1 ); | ||
wp_embed_unregister_handler( 'amp-twitter-timeline', -1 ); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,27 @@ | |
*/ | ||
class AMP_Vimeo_Embed_Handler extends AMP_Base_Embed_Handler { | ||
|
||
/** | ||
* The embed URL pattern. | ||
* | ||
* @var string | ||
*/ | ||
const URL_PATTERN = '#https?:\/\/(www\.)?vimeo\.com\/.*#i'; | ||
|
||
/** | ||
* The aspect ratio. | ||
* | ||
* @var float | ||
*/ | ||
const RATIO = 0.5625; | ||
|
||
/** | ||
* The tag (name) of the shortcode. | ||
* | ||
* @var string | ||
*/ | ||
const SHORTCODE_TAG = 'vimeo'; | ||
|
||
/** | ||
* Default width. | ||
* | ||
|
@@ -50,7 +67,9 @@ public function __construct( $args = [] ) { | |
*/ | ||
public function register_embed() { | ||
wp_embed_register_handler( 'amp-vimeo', self::URL_PATTERN, [ $this, 'oembed' ], -1 ); | ||
add_shortcode( 'vimeo', [ $this, 'shortcode' ] ); | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See notes above for Soundcloud. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, b64392b changes this |
||
add_shortcode( self::SHORTCODE_TAG, [ $this, 'shortcode' ] ); | ||
} | ||
add_filter( 'wp_video_shortcode_override', [ $this, 'video_override' ], 10, 2 ); | ||
} | ||
|
||
|
@@ -59,7 +78,9 @@ public function register_embed() { | |
*/ | ||
public function unregister_embed() { | ||
wp_embed_unregister_handler( 'amp-vimeo', -1 ); | ||
remove_shortcode( 'vimeo' ); | ||
if ( ! $this->is_amp_shortcode_available_in_jetpack( self::SHORTCODE_TAG ) ) { | ||
remove_shortcode( self::SHORTCODE_TAG ); | ||
} | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,15 @@ | |
*/ | ||
class AMP_Vimeo_Embed_Test extends WP_UnitTestCase { | ||
|
||
/** | ||
* Tears down the environment after each test. | ||
* | ||
* @inheritDoc | ||
*/ | ||
public function tearDown() { | ||
remove_all_filters( 'do_shortcode_tag' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this isn't necessary because filters get reset after each test anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, sounds good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3e0e649#diff-58db4548e89d6f4290514ec825c906ecL15 removes this |
||
} | ||
|
||
/** | ||
* Get conversion data. | ||
* | ||
|
@@ -109,4 +118,23 @@ public function test__get_scripts( $source, $expected ) { | |
|
||
$this->assertEquals( $expected, $scripts ); | ||
} | ||
|
||
/** | ||
* Test is_amp_shortcode_available_in_jetpack. | ||
* | ||
* @covers AMP_Base_Embed_Handler::is_amp_shortcode_available_in_jetpack() | ||
*/ | ||
public function test_is_amp_shortcode_available_in_jetpack() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test may not be necessary. I think we can rather just check if the |
||
remove_all_filters( 'do_shortcode_tag' ); | ||
$embed = new AMP_Vimeo_Embed_Handler(); | ||
|
||
// With the filter not added, this filter should return false. | ||
$this->assertFalse( $embed->is_amp_shortcode_available_in_jetpack( 'vimeo' ) ); | ||
|
||
add_filter( 'do_shortcode_tag', [ 'Jetpack_AMP_Vimeo_Shortcode', 'filter_shortcode' ] ); | ||
|
||
// With the filter added, this filter should return false. | ||
$this->assertTrue( $embed->is_amp_shortcode_available_in_jetpack( 'vimeo' ) ); | ||
$this->assertFalse( $embed->is_amp_shortcode_available_in_jetpack( 'wrongshortcode' ) ); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this method, wouldn't it be the same to just do
! class_exists( 'Jetpack_AMP_Soundcloud_Shortcode' )
? This code will quickly become dead code that should be removed entirely from the AMP plugin, once a new version of Jetpack has been released, so we don't need to worry too much about keeping this code around too long.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, b64392b checks whether the class exists.