OpenGraph: replace function getimagesize with standard WP function #5602

Merged
merged 1 commit into from Nov 11, 2016
@eliorivero
Contributor

Follow up to #3432

The PHP function getimagesize doesn't work in a server where allow_url_fopen=0 and throws a warning

Warning: getimagesize(): http:// wrapper is disabled in the server configuration by allow_url_fopen=0 in .../jetpack/functions.opengraph.php on line 353

Warning: getimagesize(http://xxx.xxx/wp-content/uploads/2016/05/cropped-pizza-1-270x270.png): failed to open stream: no suitable wrapper could be found in .../jetpack/functions.opengraph.php on line 353

Changes proposed in this Pull Request:

  • replace getimagesize and use a standard WP function instead.

Testing instructions:

  • make sure site icon, blavatar, everything works correctly and the sizes belong to the image used.
@eliorivero eliorivero OpenGraph: replace function getimagesize that doesn't work in an envi…
…ronment where allow_url_fopen=0. Use standard WP function instead.
c6db5f9
@eliorivero eliorivero added this to the 4.4 milestone Nov 11, 2016
@eliorivero eliorivero self-assigned this Nov 11, 2016
- $image_size = getimagesize( $image_url );
- $img_width = $image_size[0];
- $img_height = $image_size[1];
+ $max_side = max( $width, $height );
@dereksmart
dereksmart Nov 11, 2016 Member

Do you mean $max_size?

@eliorivero
eliorivero Nov 11, 2016 Contributor

No. I meant to say side, because it will get the larger side.

+ $image_id = attachment_url_to_postid( $image_url );
+ $image_size = wp_get_attachment_image_src( $image_id, $width >= 512
+ ? 'full'
+ : array( $width, $width ) );
@dereksmart
dereksmart Nov 11, 2016 Member

should this be $width, $height?

@eliorivero
eliorivero Nov 11, 2016 Contributor

no, it's to pass the same dimension passed when calling blavatar_url a few lines above, in line 324

@dereksmart
Member

Thanks Elio, looks good 👍 wp functions ftw

@dereksmart
Member

One note is that we are going to have to update wpcom files to use this as well. I don't see anything in here that is jetpack specific, so should be fine when it's all synced up.

@samhotchkiss samhotchkiss merged commit 878eb2b into master Nov 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samhotchkiss samhotchkiss deleted the fix/replace-getimagesize-wp-compat-function branch Nov 11, 2016
@zinigor zinigor added a commit that referenced this pull request Nov 14, 2016
@eliorivero @zinigor eliorivero + zinigor OpenGraph: replace function getimagesize that doesn't work in an envi…
…ronment where allow_url_fopen=0. Use standard WP function instead. (#5602)
14ab7ec
@zinigor
Member
zinigor commented Nov 14, 2016

Ported to branch-4.4 in 14ab7ec.

+
+ $img_width = '';
+ $img_height = '';
+ $image_id = attachment_url_to_postid( $image_url );
@mjangda
mjangda Jan 6, 2017 edited Member

@eliorivero Blavatars are hosted remotely (on Gravatar), so calling attachment_url_to_postid with a Blavatar URL will always fail.

@jeherve
jeherve Jan 9, 2017 Member

Since the code in this PR is already merged and released, I opened #6061 so we don't lose your comments. Let's continue the conversation there.

+
+ $img_width = '';
+ $img_height = '';
+ $image_id = attachment_url_to_postid( $image_url );
@mjangda
mjangda Jan 6, 2017 Member

@eliorivero Instead of doing a reverse lookup using attachment_url_to_postid which can be very slow on big sites, can we just call get_option( 'site_icon' ) to get the id?

@jeherve
jeherve Jan 9, 2017 Member

Since the code in this PR is already merged and released, I opened #6061 so we don't lose your comments. Let's continue the conversation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment