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

Add caching for VideoPress functions with meta queries #14803

Merged
merged 14 commits into from
Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/videopress/class.videopress-player.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ public function __construct( $guid, $maxwidth = 0, $options = array() ) {

if ( ! defined( 'WP_DEBUG' ) || WP_DEBUG !== true ) {
$expire = 3600;
if ( isset( $video->expires ) && is_int( $video->expires ) ) {
$expires_diff = time() - $video->expires;
if ( isset( $this->video->expires ) && is_int( $this->video->expires ) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With unset( $video ); above, it looks like isset( $video->expires ) will never be true

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch.

$expires_diff = time() - $this->video->expires;
if ( $expires_diff > 0 && $expires_diff < 86400 ) { // allowed range: 1 second to 1 day
$expire = $expires_diff;
}
Expand Down
73 changes: 67 additions & 6 deletions modules/videopress/utility-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ function videopress_get_video_details( $guid ) {
*
* Modified from https://wpscholar.com/blog/get-attachment-id-from-wp-image-url/
*
* @todo: Add some caching in here.
* @deprecated since 8.4.0
leogermani marked this conversation as resolved.
Show resolved Hide resolved
* @see videopress_get_post_id_by_guid()
*
* @param string $url
*
* @return int|bool Attachment ID on success, false on failure
*/
function videopress_get_attachment_id_by_url( $url ) {
_deprecated_function( __FUNCTION__, 'jetpack-8.4' );

$wp_upload_dir = wp_upload_dir();
// Strip out protocols, so it doesn't fail because searching for http: in https: dir.
$dir = set_url_scheme( trailingslashit( $wp_upload_dir['baseurl'] ), 'relative' );
Expand Down Expand Up @@ -619,7 +622,7 @@ function video_format_done( $info, $format ) {
*/
function video_image_url_by_guid( $guid, $format ) {

$post = video_get_post_by_guid( $guid );
$post = videopress_get_post_by_guid( $guid );

if ( is_wp_error( $post ) ) {
return null;
Expand All @@ -636,14 +639,67 @@ function video_image_url_by_guid( $guid, $format ) {
/**
* Using a GUID, find a post.
*
* @param string $guid
* @return WP_Post
* @param string $guid The post guid.
* @return WP_Post|false The post for that guid, or false if none is found.
*/
function videopress_get_post_by_guid( $guid ) {
$cache_key = 'get_post_by_guid_' . $guid;
$cache_group = 'videopress';
$cached_post = wp_cache_get( $cache_key, $cache_group );

if ( is_object( $cached_post ) && 'WP_Post' === get_class( $cached_post ) ) {
return $cached_post;
}

$post_id = videopress_get_post_id_by_guid( $guid );

if ( is_int( $post_id ) ) {
$post = get_post( $post_id );
wp_cache_set( $cache_key, $post, $cache_group, HOUR_IN_SECONDS );

return $post;
}

return false;
}

/**
* Using a GUID, find a post.
*
* Kept for backward compatibility. Use videopress_get_post_by_guid() instead.
*
* @deprecated since 8.4.0
* @see videopress_get_post_by_guid()
*
* @param string $guid The post guid.
* @return WP_Post|false The post for that guid, or false if none is found.
*/
function video_get_post_by_guid( $guid ) {
_deprecated_function( __FUNCTION__, 'jetpack-8.4' );
return videopress_get_post_by_guid( $guid );
}

/**
* Using a GUID, find the associated post ID.
*
* @since 8.4.0
* @param string $guid The guid to look for the post ID of.
* @return int|false The post ID for that guid, or false if none is found.
*/
function videopress_get_post_id_by_guid( $guid ) {
$cache_key = 'videopress_get_post_id_by_guid_' . $guid;
$cached_id = get_transient( $cache_key );

if ( is_int( $cached_id ) ) {
return $cached_id;
}

$args = array(
'post_type' => 'attachment',
'post_mime_type' => 'video/videopress',
'post_status' => 'inherit',
'no_found_rows' => true,
'fields' => 'ids',
'meta_query' => array(
leogermani marked this conversation as resolved.
Show resolved Hide resolved
array(
'key' => 'videopress_guid',
Expand All @@ -655,9 +711,14 @@ function video_get_post_by_guid( $guid ) {

$query = new WP_Query( $args );

$post = $query->next_post();
Copy link
Contributor Author

@kienstra kienstra Feb 26, 2020

Choose a reason for hiding this comment

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

There can be an error here if the query didn't find any posts.

if ( $query->have_posts() ) {
$post_id = $query->next_post();
set_transient( $cache_key, $post_id, HOUR_IN_SECONDS );

return $post;
return $post_id;
}

return false;
}

/**
Expand Down
181 changes: 181 additions & 0 deletions tests/php/modules/videopress/test_functions.utility-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,187 @@
*/
class WP_Test_Jetpack_VideoPress_Utility_Functions extends WP_UnitTestCase {

/**
* Tests a helper function to get the post by guid, when there is no post found.
*
* @covers ::videopress_get_post_by_guid
* @since 8.4.0
*/
public function test_no_post_found_videopress_get_post_by_guid() {
$this->assertFalse( videopress_get_post_by_guid( wp_generate_uuid4() ) );
}

/**
* Gets the test data for test_non_cached_videopress_get_post_by_guid().
*
* @since 8.4.0
*
* @return array The test data.
*/
public function get_data_test_video_non_cached() {
return array(
'external_object_cache_is_enabled' => array(
'wp_cache_get',
true,
'get_post_by_guid_',
'videopress',
),
'external_object_cache_is_not_enabled' => array(
'get_transient',
false,
'videopress_get_post_id_by_guid_',
),
);
}

/**
* Tests a helper function to get the post by guid, when there's initially no cached value.
*
* @dataProvider get_data_test_video_non_cached
* @covers ::videopress_get_post_by_guid
* @since 8.4.0
*
* @param callable $callback The callback to get the caching.
* @param bool $should_cache_object Whether the entire WP_Post should be cached, or simply the post ID.
* @param string $cache_key_base The base of the cache key.
* @param string|null $cache_group The cache group, if any.
*/
public function test_non_cached_videopress_get_post_by_guid( $callback, $should_cache_object, $cache_key_base, $cache_group = null ) {
$guid = wp_generate_uuid4();
$expected_id = videopress_create_new_media_item( 'Example', $guid );
$expected_post = get_post( $expected_id );
$actual_post = videopress_get_post_by_guid( $guid );

$this->assertEquals( $expected_post, $actual_post );

$caching_args = array( $cache_key_base . $guid );
if ( $cache_group ) {
$caching_args[] = $cache_group;
}
$expected_cached = $should_cache_object ? $expected_post : $expected_id;

// The function should have cached the value.
$this->assertEquals(
$expected_cached,
call_user_func_array( $callback, $caching_args )
);
}

/**
* Gets the test data for test_cached_videopress_get_post_by_guid().
*
* @since 8.4.0
*
* @return array The test data.
*/
public function get_data_test_video_cached() {
return array(
'post_should_be_stored_in_cache' => array(
'wp_cache_set',
true,
'videopress',
),
'post_id_should_be_stored_in_transient' => array(
'set_transient',
false,
),
);
}

/**
* Tests a helper function to get the post by guid, when there is a cached value.
*
* As long as there is a non-expired cache value,
* this should return that instead of instantiating WP_Query.
*
* @dataProvider get_data_test_video_cached
* @covers ::videopress_get_post_by_guid
* @since 8.4.0
*
* @param callable $callback The callback to set the caching.
* @param bool $should_cache_object Whether the entire WP_Post should be cached, or simply the post ID.
* @param string|null $cache_group The cache group, if any.
*/
public function test_cached_videopress_get_post_by_guid( $callback, $should_cache_object, $cache_group = null ) {
$guid = wp_generate_uuid4();
$attachment_id = videopress_create_new_media_item( 'Example Title', $guid );
$attachment_post = get_post( $attachment_id );
$post_to_cache = $should_cache_object ? $attachment_post : $attachment_id;
$caching_args = array( 'get_post_by_guid_' . $guid, $post_to_cache );

if ( $cache_group ) {
$caching_args[] = $cache_group;
}

call_user_func_array( $callback, $caching_args );

// This should always return the WP_Post, even though the post ID is stored in the transient.
$this->assertEquals(
$attachment_post,
videopress_get_post_by_guid( $guid )
);
}

/**
* Gets the test data for test_cached_invalid_videopress_get_post_by_guid().
*
* @since 8.4.0
*
* @return array The test data.
*/
public function get_data_cached_invalid() {
return array(
'non_post_object' => array( new stdClass() ),
'int_but_not_valid_post_id' => array( PHP_INT_MAX ),
'null' => array( null ),
'zero' => array( 0 ),
);
}

/**
* Tests invalid cached values that should be ignored.
*
* Unless the cached value is a WP_Post,
* the tested method should ignore it and query for the post.
*
* @dataProvider get_data_cached_invalid
* @covers ::videopress_get_post_by_guid
* @since 8.4.0
*
* @param mixed $invalid_cached_value A cached value that should be ignored.
*/
public function test_cached_invalid_videopress_get_post_by_guid( $invalid_cached_value ) {
$guid = wp_generate_uuid4();
$attachment_id = videopress_create_new_media_item( 'Example Title', $guid );

wp_cache_set( 'get_post_by_guid_' . $guid, $invalid_cached_value, 'videopress' );

$this->assertEquals(
get_post( $attachment_id ),
videopress_get_post_by_guid( $guid )
);
}

/**
* Tests a helper function to get the post id by guid.
*
* @covers ::videopress_get_post_id_by_guid
* @since 8.4.0
*/
public function test_non_cached_videopress_get_post_id_by_guid() {
$guid = wp_generate_uuid4();
$expected_id = videopress_create_new_media_item( 'Example', $guid );
$actual_post_id = videopress_get_post_id_by_guid( $guid );

$this->assertEquals( $expected_id, $actual_post_id );

// The function should have cached the value.
$this->assertEquals(
$expected_id,
get_transient( 'videopress_get_post_id_by_guid_' . $guid )
);
}

/**
* Tests the VideoPress Flash to oEmbedable URL filter.
*
Expand Down