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

Cache preview images of docs locally & Fix showing doc preview issues #6

Open
wants to merge 5 commits into
base: custom-oembed-provider
Choose a base branch
from

Conversation

itowhid06
Copy link

No description provided.

templates/embeds/google-drive-file.php Outdated Show resolved Hide resolved
includes/classes/class-rtcamp-google-embeds.php Outdated Show resolved Hide resolved
templates/embeds/google-drive-file.php Outdated Show resolved Hide resolved
templates/embeds/google-drive-file.php Outdated Show resolved Hide resolved
@itowhid06 itowhid06 changed the title Cache preview images of docs locally Cache preview images of docs locally & Fix showing doc preview issues May 5, 2020
@juhi123 juhi123 requested a review from SID177 June 4, 2020 06:55
@juhi123
Copy link
Contributor

juhi123 commented Jun 4, 2020

@SID177 Whenever you are available, please review this PR

Copy link
Contributor

@SID177 SID177 left a comment

Choose a reason for hiding this comment

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

Check comments @itowhid06

includes/classes/class-rtcamp-google-embeds.php Outdated Show resolved Hide resolved
includes/classes/class-rtcamp-google-embeds.php Outdated Show resolved Hide resolved
includes/classes/class-rtcamp-google-embeds.php Outdated Show resolved Hide resolved
includes/classes/class-rtcamp-google-embeds.php Outdated Show resolved Hide resolved
templates/embeds/google-drive-file.php Outdated Show resolved Hide resolved
// Check if retrieved http code is 200.
$status_code = wp_remote_retrieve_response_code( $response );
if ( 200 === $status_code ) {
// Save the thumbnail.
$this->save_thumbnail( $file_id, $contents );
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to cache image which is being fetched on line 322 of this file too. And test once if it's working fine even after token is expired

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try implementing this? Current save_thumbnail call will only work for public files, not for private files. In order for this to work with private files, you'll need to do what I've suggested above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also get_thumbnail_url function should return cached URL if network request fails. If all the network requests fails, then at the end you can check whether the file of this ID is already cached, if it is then send that URL.

Copy link
Contributor

@SID177 SID177 left a comment

Choose a reason for hiding this comment

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

Please check suggestions, also check the previous suggestions as well.

$cached_url = esc_url_raw( $upload_dir['baseurl'] . "/cache/wp-google-drive/{$file_id}.png" );

var_dump( $cached_file );
var_dump( $cached_url );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there are var_dump here?


preg_match( '/[-\w]{25,}/', $url, $matches );
if ( ! empty( $matches[0] ) ) {
$file_id = $matches[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if the matches are empty then it shouldn't go ahead.

$matches = array();
$file_id = '';
$basedir = '';
$baseurl = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

PHPCS indentations

$basedir = '';
$baseurl = '';

preg_match( '/[-\w]{25,}/', $url, $matches );
Copy link
Contributor

Choose a reason for hiding this comment

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

Use get_file_id_from_url existing function instead.

// Check if retrieved http code is 200.
$status_code = wp_remote_retrieve_response_code( $response );
if ( 200 === $status_code ) {
// Save the thumbnail.
$this->save_thumbnail( $file_id, $contents );
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try implementing this? Current save_thumbnail call will only work for public files, not for private files. In order for this to work with private files, you'll need to do what I've suggested above.

// Check if retrieved http code is 200.
$status_code = wp_remote_retrieve_response_code( $response );
if ( 200 === $status_code ) {
// Save the thumbnail.
$this->save_thumbnail( $file_id, $contents );
Copy link
Contributor

Choose a reason for hiding this comment

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

Also get_thumbnail_url function should return cached URL if network request fails. If all the network requests fails, then at the end you can check whether the file of this ID is already cached, if it is then send that URL.

</a>
<img src="<?php echo esc_url( $thumbnail_url ); ?>" alt="<?php esc_attr_e( 'Shared Document Preview', 'rt-google-embeds' ); ?>" style="border: 1px solid #eee; margin: 15px auto; display: block;" />
</div>
if ( file_exists( $cached_file ) ) : ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

There won't be a cached_file URL here, as if the network request fails, get_thumbnail_url function itself will return cached file URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding https://github.com/rtCamp/wp-google-drive/pull/6/files#diff-59c6e676df5dade9eec1de91f5c5b44fR252-R253 lines, you'll only use the URL retrieved from get_thumbnail_url function, that function will decide whether to use cached image or network image URL.

Whenever network request is successful, you update the cached image. And when network request fails, you return the cached image URL.

@itowhid06 itowhid06 force-pushed the cache-preview-image branch from 3e1164b to 165a371 Compare August 17, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants