Open Graph: make sure transients are used to save image IDs. #6632

jeherve · 2017-03-10T16:55:16Z

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:

Open Graph: make sure transients are used to save image IDs.
Open Graph: make sure Site Icons are used as fallback Open Graph Image tags.

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.

samhotchkiss · 2017-03-27T16:29:18Z

LGTM-- @georgestephanis, do you mind giving it a spin?

georgestephanis · 2017-03-27T16:48:46Z

Will do.

georgestephanis

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.

@see

@see #6632 (review)

jeherve · 2017-03-27T17:50:57Z

Thanks for the feedback!

The diff here is showing what looks like some needless churn -- reordering of lines in both chunks, for example. Is this necessary?

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.

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.

Oh, I did not know that! Thanks! Fixed in b564cee

is it possible to extract the hash and prefixing to a seperate function or something?

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

Sorry for multiple change requests, went a bit more in depth this time.

georgestephanis · 2017-03-27T18:04:08Z

functions.opengraph.php

+
+			// 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 ) ) {


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.

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?

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.

georgestephanis · 2017-03-27T18:05:21Z

functions.opengraph.php

 				$image_id = attachment_url_to_postid( $image_url );
-				set_transient( 'jp_' . $image_url, $image_id );
+				set_transient( 'jp_' . $image_hash, $image_id );


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?

Good call. The conditional wasn't necessary since $image_url will always be set. I removed it.

georgestephanis · 2017-03-27T18:05:25Z

functions.opengraph.php

+			}
+
+			// Look for data in our transient. If nothing, let's get an attachment ID.
+			$cached_image_id = get_transient( 'jp_' . $image_hash );


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?

georgestephanis · 2017-03-27T18:08:20Z

functions.opengraph.php


+		$max_side = max( $width, $height );
+		$image_url = get_site_icon_url( $max_side );
+		$image_id = get_option( 'site_icon' );


Should we add a break or return or something if this returns nothing?

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.

@see

$image_hash will always exist, as `blavatar_url()` always returns something. @see https://github.com/Automattic/jetpack/pull/6632/files#r108238825

jeherve · 2017-03-28T10:12:40Z

@georgestephanis Thanks for all the feedback! I made some changes and replied to your comments.

zinigor

Looks good, works well in my tests!

@SInCE

* 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

jeherve added [Feature] Sharing Post sharing, sharing buttons [Pri] High [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Mar 10, 2017

jeherve self-assigned this Mar 10, 2017

matticbot added the Touches WP.com Files label Mar 10, 2017

Open Graph: remove transient for Site Icon fallback.

a30c365

We're already getting the site icon from the options table anyway, performance should be similar.

jeherve force-pushed the fix/missing-site-icon branch from e2d2d5a to a30c365 Compare March 10, 2017 21:01

samhotchkiss added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 27, 2017

georgestephanis requested changes Mar 27, 2017

View reviewed changes

Open Graph: use sha1 instead of md5 to create image URL hash.

b564cee

@see #6632 (review)

georgestephanis requested changes Mar 27, 2017

View reviewed changes

jeherve mentioned this pull request Mar 27, 2017

Sharing: Site Icon doesn't show when sharing to WhatsApp #6776

Closed

Open Graph: remove conditional when building $image_hash

4c25dd3

$image_hash will always exist, as `blavatar_url()` always returns something. @see https://github.com/Automattic/jetpack/pull/6632/files#r108238825

zinigor approved these changes Mar 28, 2017

View reviewed changes

jeherve added this to the 4.8 milestone Mar 28, 2017

georgestephanis approved these changes Mar 28, 2017

View reviewed changes

georgestephanis merged commit 93c0438 into master Mar 28, 2017

georgestephanis removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 28, 2017

jeherve deleted the fix/missing-site-icon branch March 28, 2017 21:35

jeherve added the [Status] Has Changelog label Mar 28, 2017

jeherve added a commit that referenced this pull request Mar 28, 2017

Changelog: add #6632

c515f05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Graph: make sure transients are used to save image IDs. #6632

Open Graph: make sure transients are used to save image IDs. #6632

jeherve commented Mar 10, 2017

samhotchkiss commented Mar 27, 2017

georgestephanis commented Mar 27, 2017

georgestephanis left a comment

jeherve commented Mar 27, 2017

georgestephanis left a comment

georgestephanis Mar 27, 2017

jeherve Mar 28, 2017

georgestephanis Mar 28, 2017

georgestephanis Mar 27, 2017

jeherve Mar 28, 2017 •

edited

Loading

georgestephanis Mar 27, 2017

georgestephanis Mar 27, 2017

jeherve Mar 28, 2017

jeherve commented Mar 28, 2017

zinigor left a comment

Open Graph: make sure transients are used to save image IDs. #6632

Open Graph: make sure transients are used to save image IDs. #6632

Conversation

jeherve commented Mar 10, 2017

Proposed changelog entry for your changes:

samhotchkiss commented Mar 27, 2017

georgestephanis commented Mar 27, 2017

georgestephanis left a comment

Choose a reason for hiding this comment

jeherve commented Mar 27, 2017

georgestephanis left a comment

Choose a reason for hiding this comment

georgestephanis Mar 27, 2017

Choose a reason for hiding this comment

jeherve Mar 28, 2017

Choose a reason for hiding this comment

georgestephanis Mar 28, 2017

Choose a reason for hiding this comment

georgestephanis Mar 27, 2017

Choose a reason for hiding this comment

jeherve Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

georgestephanis Mar 27, 2017

Choose a reason for hiding this comment

georgestephanis Mar 27, 2017

Choose a reason for hiding this comment

jeherve Mar 28, 2017

Choose a reason for hiding this comment

jeherve commented Mar 28, 2017

zinigor left a comment

Choose a reason for hiding this comment

jeherve Mar 28, 2017 •

edited

Loading