Open Graph: make sure transients are used to save image IDs.#6632
Open Graph: make sure transients are used to save image IDs.#6632georgestephanis merged 4 commits intomasterfrom
Conversation
In #6297, we added transients to cache some of the fallback images in jetpack_og_get_image(). However, transients can only be 45 chars maximum. To work around that limitation, we now create a 44-char string using md5, based on the image URL. Also added back changes to Site Icon brought in #6303, and accidentally reverted in #6297.
We're already getting the site icon from the options table anyway, performance should be similar.
e2d2d5a to
a30c365
Compare
|
LGTM-- @georgestephanis, do you mind giving it a spin? |
|
Will do. |
georgestephanis
left a comment
There was a problem hiding this comment.
The diff here is showing what looks like some needless churn -- reordering of lines in both chunks, for example. Is this necessary?
Also, is it possible to extract the hash and prefixing to a seperate function or something?
Finally, it's not necessary to substring the result of calling md5, as md5s are only 32 characters long -- it'll never hit the 41 char you're allotting. Also, SHA-1 is 40 characters long -- I think either is likely fine, as engineered collisions are low-risk in this scenario.
|
Thanks for the feedback!
I think it makes the code more consistent and easier to read, especially when going through each fallback method, one at a time. I'd like to leave this as is.
Oh, I did not know that! Thanks! Fixed in b564cee
The hash method is unique in this file so far, but I'll definitely keep this in mind if we ever extend it and reuse this later on! |
georgestephanis
left a comment
There was a problem hiding this comment.
Sorry for multiple change requests, went a bit more in depth this time.
|
|
||
| // Look for data in our transient. If nothing, let's get an attachment ID. | ||
| $cached_image_id = get_transient( 'jp_' . $image_hash ); | ||
| if ( ! is_int( $cached_image_id ) ) { |
There was a problem hiding this comment.
Would the returned value from get_transient() be an int type, or a string type? I know is_numeric() would include integers stored as strings, but unless we're casting it, I'm not sure if is_int() would.
There was a problem hiding this comment.
attachment_url_to_postid() casts its result as an integer so get_transient() should return an integer as well. If it doesn't, it means the value was modified and we probably shouldn't use it.
Since we use that transient in wp_get_attachment_image_src() later, and since wp_get_attachment_image_src() expects an integer, I think it's best to check for is_int() before to use the value.
Or do you think I should use is_numeric() and allow for third-parties to overwrite the transients saved by Jetpack and save strings instead?
There was a problem hiding this comment.
Honestly, I'm unsure. If it works now, good enough, we can reevaluate later or third-parties can cast to int before saving.
I just wasn't sure if the number would get cast to a string when saved in the db. Just being cautious.
| if ( ! is_int( $cached_image_id ) ) { | ||
| $image_id = attachment_url_to_postid( $image_url ); | ||
| set_transient( 'jp_' . $image_url, $image_id ); | ||
| set_transient( 'jp_' . $image_hash, $image_id ); |
There was a problem hiding this comment.
You're using $image_hash here, but it's only set conditionally above. So could there be a situation where $image_url evaluates to falsey, so $image_hash never gets declared?
There was a problem hiding this comment.
Good call. The conditional wasn't necessary since $image_url will always be set. I removed it.
| } | ||
|
|
||
| // Look for data in our transient. If nothing, let's get an attachment ID. | ||
| $cached_image_id = get_transient( 'jp_' . $image_hash ); |
There was a problem hiding this comment.
You're using $image_hash here, but it's only set conditionally above. So could there be a situation where $image_url evaluates to falsey, so $image_hash never gets declared?
|
|
||
| $max_side = max( $width, $height ); | ||
| $image_url = get_site_icon_url( $max_side ); | ||
| $image_id = get_option( 'site_icon' ); |
There was a problem hiding this comment.
Should we add a break or return or something if this returns nothing?
There was a problem hiding this comment.
Since we check for has_site_icon() before to start the whole process, I don't think that's necessary. For has_site_icon() to return true, get_option( 'site_icon' ) has to return an ID.
$image_hash will always exist, as `blavatar_url()` always returns something. @see https://github.com/Automattic/jetpack/pull/6632/files#r108238825
|
@georgestephanis Thanks for all the feedback! I made some changes and replied to your comments. |
zinigor
left a comment
There was a problem hiding this comment.
Looks good, works well in my tests!
* Readme: remove old release and add skeleton for 4.8. * Changelog: add #6572 * Changelog: add #6567 * Changelog: add #6542 * Changelog: add #6527 * Changelog: add #6508 * Changelog: add #6478 * Changelog: add #6477 * Changelog: add #6249 * Update stable version and remove old version from readme. * Changelog: add 4.7.1 to changelog. * Readme: add new contributor. * Sync: update docblock @SInCE version. Related: #6053 * Changelog: add release post. * changelog: add #6053 * Changelog: add #6413 * Changelog: add #6482 * Changelog: add #6584 * Changelog add #6603 * Changelog: add #6606 * Changelog: add #6611 * Changelog: add #6635 * Changelog: add #6639 * Changelog: add #6684 * Changelog: add #6710 * Changelog: add #6711 * Changelog: add #5461 * Testing list: update Settings UI feedback prompt. Props @MichaelArestad * Changelog: add #6789 * Changelog: add #6778 * Changelog: add #6777 * Changelog: add #6775 * Changelog: add #6755 * Changelog: add #6731 * Changelog: add #6721 * Changelog: add #6705 * Changelog: add #6702 * Changelog: add #6671 * Changelog: add #6637 * Changelog: add #6582 * Changelog: add #6566 * Changelog: add #6555 * Changelog: add #6529 * Changelog: add #6344 * Changelog: add #5763 * Changelog: add #5503 * Changelog: update #6637 changelog. @see 40e115c#commitcomment-21523982 * Changelog: add #6699 * Changelog: add #6632 * Changelog: add #6769 * Changelog: add #6707 * Changelog: add #6590
In #6297, we added transients to cache some of the fallback images in jetpack_og_get_image().
However, transients can only be 45 chars maximum.
To work around that limitation, we now create a 44-char string using md5, based on the image URL.
Also added back changes to Site Icon brought in #6303, and accidentally reverted in #6297.
Fixes 144-gh-jpop-issues
Proposed changelog entry for your changes: