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

Add caching for VideoPress functions with meta queries #14803

Merged
merged 14 commits into from
Mar 23, 2020
Merged

Add caching for VideoPress functions with meta queries #14803

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9608b4b
Add caching for VideoPress functions that call WP_Query
kienstra Feb 25, 2020
f91ef7c
Change the since tag to 8.4.0
kienstra Feb 26, 2020
96a565a
Revert changes to videopress_get_attachment_id_by_url()
kienstra Feb 27, 2020
67df908
Deprecate videopress_get_attachment_id_by_url()
kienstra Feb 28, 2020
85b14a0
Store the videopress ID in a transient if no persistent object caching
kienstra Mar 17, 2020
21dc2cd
Remove needless helper method
kienstra Mar 17, 2020
073e096
Add a new function, using Leo's suggestion
kienstra Mar 19, 2020
69447b8
Move the new function's test lower in the file
kienstra Mar 19, 2020
331f265
Set the transient expiration to an hour
kienstra Mar 19, 2020
5c05e33
Add a see tag, correct DocBlock summary
kienstra Mar 19, 2020
64918b1
Change prefix of new function from video_ to videopress_
kienstra Mar 19, 2020
e630175
Add since tags to new functions
kienstra Mar 19, 2020
1e163aa
Remove needless comment
kienstra Mar 19, 2020
c13c629
Rename function to keep consistency
leogermani Mar 20, 2020
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
View file
Open in desktop
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
49 changes: 38 additions & 11 deletions modules/videopress/utility-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ 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.
*
* @param string $url
*
* @return int|bool Attachment ID on success, false on failure
Expand All @@ -87,13 +85,21 @@ function videopress_get_attachment_id_by_url( $url ) {
// Is URL in uploads directory?
if ( false !== strpos( $url, $dir ) ) {

$file = basename( $url );
$file = basename( $url );
$cache_key = 'videopress_get_attachment_id_by_url_' . md5( $url );
$cache_group = 'videopress';
$cached_id = wp_cache_get( $cache_key, $cache_group );
Copy link
Member

Choose a reason for hiding this comment

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

Quick question; did you consider using transients instead of object caching? Would it be better for performance on sites that do not use caching plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'll look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I could add a transient here if you'd like.

But since the attachment ID is from a database query (WP_Query), I'm not sure that using a transient to store it in the database (wp_options) would be the best approach.

It would probably improve performance for sites without persistent object caching. But it could fill the wp_options table.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kienstra Expiring transients aren't autoloaded and are deleted once they expire.

Do you think we need to worry about polluting wp_options under these circumstances?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kienstra Also, is videopress_get_attachment_id_by_url ever called?

I think the original usage was in videopress_shortcode_override_for_core_shortcode (771097d), but has been superseded by 32dbcd7, but the method itself has never been removed.

I ran git log -p -S videopress_get_attachment_id_by_url to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Use transients, but store only the post ID
  2. We could use the object cache to store the post object, avoiding multiple calls to get_post() if we want.

Thanks, what do you think about the latest commits, which apply those 2 points?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And to wrap things up, I'd get the post ID in a separate method, which would be a direct replacement for the method we are deprecating :) .

Hm, did you have in mind something like:

/**
 * Using a GUID, find a post ID.
 *
 * @param string $guid The guid to use to find the post ID.
 * @return int|false The post ID.
 */
function video_get_post_id_by_guid( $guid ) {
	$post = video_get_post_by_guid( $guid );

	if ( is_object( $post ) && 'WP_Post' === get_class( $post ) ) {
		return $post->ID;
	}

	return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, what do you think about the latest commits, which apply those 2 points?

What I thought is that we can use both transients and wp_cache at the same time, each one for a purpose. Transients to store the post ID, so we avoid making the costly SQL SELECT many times though several requests. Object Caching to avoid calling get_post() several times within the same request.

Hm, did you have in mind something like:

Not exactly. I would actually leave the WP_Query call in video_get_post_id_by_guid, adding the 'fields' => 'ids'. I would call it from video_get_post_by_guid and then call get_post. So I would have the same behavior every time. (Always fetch the ID and then call get_post)

Looks a bit cleaner to me. Each method doing just one thing, and avoiding things like $cached_post sometimes holding the actual post, and sometimes holding just the post id.

Also it's important to add an expiration time for the transient.

This is how I see it. (Please consider this just as an opinion!)

function video_get_post_by_guid( $guid ) {
	// ...
	$cached_post = wp_cache_get( $cache_key, $cache_group );

	if ( $cached_post ) {
		return $cached_post;
	}

	$post_id = $this->video_get_post_id_by_guid( $guid );

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

}

function video_get_post_id_by_guid( $guid ) {
	// ... 
	$cached_id = get_transient( $cache_key );
	if ( $cached_id ) {
		return $cached_id;
	}

	// do the query
	set_transient( $cache_key, $id, HOUR_IN_SECONDS );
	return $id;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, and sorry for the delay. I'm working on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this look? It mainly uses your snippet above.

if ( false !== $cached_id ) {
return $cached_id;
}

$query_args = array(
'post_type' => 'attachment',
'post_status' => 'inherit',
'fields' => 'ids',
'meta_query' => array(
'post_type' => 'attachment',
'post_status' => 'inherit',
'fields' => 'ids',
'no_found_rows' => true,
'update_post_term_cache' => false,
'meta_query' => array(
array(
'key' => '_wp_attachment_metadata',
'compare' => 'LIKE',
Expand All @@ -111,7 +117,9 @@ function videopress_get_attachment_id_by_url( $url ) {
$cropped_files = wp_list_pluck( $meta['sizes'], 'file' );

if ( $original_file === $file || in_array( $file, $cropped_files ) ) {
return (int) $attachment_id;
$attachment_id = (int) $attachment_id;
wp_cache_set( $cache_key, $attachment_id, $cache_group, HOUR_IN_SECONDS );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the cache expiration time should be different

return $attachment_id;
}
}
}
Expand Down Expand Up @@ -637,13 +645,28 @@ function video_image_url_by_guid( $guid, $format ) {
* Using a GUID, find a post.
*
* @param string $guid
* @return WP_Post
* @return WP_Post|false The post for that guid, or false if none is found.
*/
function video_get_post_by_guid( $guid ) {
$cache_key = 'video_get_post_by_guid_' . $guid;
$cache_group = 'videopress';
$cached_post_id = wp_cache_get( $cache_key, $cache_group );

if ( false !== $cached_post_id ) {
$cached_post = get_post( $cached_post_id );
if ( $cached_post ) {
return $cached_post;
} else {
// The cached post ID doesn't belong to a post, so delete it.
wp_cache_delete( $cache_key, $cache_group );
}
}

$args = array(
'post_type' => 'attachment',
'post_mime_type' => 'video/videopress',
'post_status' => 'inherit',
'no_found_rows' => true,
'meta_query' => array(
leogermani marked this conversation as resolved.
Show resolved Hide resolved
array(
'key' => 'videopress_guid',
Expand All @@ -655,9 +678,13 @@ 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 •
edited
Loading

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 = $query->next_post();
wp_cache_set( $cache_key, $post->ID, $cache_group, HOUR_IN_SECONDS );
return $post;
}

return $post;
return false;
}

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

/**
* Tests a helper function to get the attachment ID, when there's no cached value.
*
* @covers ::videopress_get_attachment_id_by_url
* @since 8.3.0
*/
public function test_non_cached_videopress_get_attachment_id_by_url() {
$expected_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/test-image.jpg', 0 );
$url = wp_get_attachment_url( $expected_id );

$this->assertEquals( $expected_id, videopress_get_attachment_id_by_url( $url ) );
}

/**
* Tests a helper function to get the attachment ID, when there is a cached value.
*
* @covers ::videopress_get_attachment_id_by_url
* @since 8.3.0
*/
public function test_cached_videopress_get_attachment_id_by_url() {
$cached_id = 512351;
self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/test-image.jpg', 0 );
$upload_dir = wp_get_upload_dir();
$url = trailingslashit( $upload_dir['url'] ) . 'random.jpg';
wp_cache_set( 'videopress_get_attachment_id_by_url_' . md5( $url ), $cached_id, 'videopress' );

$this->assertEquals( $cached_id, videopress_get_attachment_id_by_url( $url ) );
}

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

/**
* Tests a helper function to get the post by guid, when there's no cached value.
*
* @covers ::video_get_post_by_guid
* @since 8.3.0
*/
public function test_non_cached_video_get_post_by_guid() {
$guid = wp_generate_uuid4();
$expected_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/test-image.jpg', 0 );
wp_insert_attachment(
array(
'ID' => $expected_id,
'post_mime_type' => 'video/videopress',
)
);
add_post_meta( $expected_id, 'videopress_guid', $guid );

$actual_post = video_get_post_by_guid( $guid );
$this->assertEquals( $expected_id, $actual_post->ID );
}

/**
* 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.
*
* @covers ::video_get_post_by_guid
* @since 8.3.0
jeherve marked this conversation as resolved.
Show resolved Hide resolved
*/
public function test_cached_video_get_post_by_guid() {
$attachment_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/test-image.jpg', 0 );
wp_insert_attachment(
array(
'ID' => $attachment_id,
'post_mime_type' => 'video/videopress',
)
);
add_post_meta( $attachment_id, 'videopress_guid', wp_generate_uuid4() );

$cached_guid = wp_generate_uuid4();
$cached_post_id = $this->factory()->post->create();
wp_cache_set( 'video_get_post_by_guid_' . $cached_guid, $cached_post_id, 'videopress' );

$actual_post = video_get_post_by_guid( $cached_guid );
$this->assertEquals( $cached_post_id, $actual_post->ID );
}

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