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

[Open Graph] Use 'og:image:secure_url' for images via HTTPS #7240

Merged
merged 1 commit into from Jun 27, 2017

Conversation

@apfelbox
Copy link
Contributor

apfelbox commented Jun 12, 2017

Summary

This PR can be summarized in the following changelog entry:

  • [Open Graph] Use 'og:image:secure_url' for images via HTTPS

Relevant technical choices:

  • HTTPS only URLs won't work with just og:image, so use og:image:secure_url instead.

Defaulting to HTTPS is good for many reasons, even if there would be a HTTP version available.

Test instructions

This PR can be tested by following these steps:

  1. Install WordPress on an HTTPs website
  2. Load a post
  3. Look at the generated tag.
@@ -576,7 +576,10 @@ public function image( $image = false ) {
$opengraph_images = new WPSEO_OpenGraph_Image( $this->options, $image );

foreach ( $opengraph_images->get_images() as $img ) {
$this->og_tag( 'og:image', esc_url( $img ) );
$tag_suffix = 0 === strpos($img, 'https://')

This comment has been minimized.

Copy link
@andizer

andizer Jun 19, 2017

Contributor

Thank you for your pull request.

I have one point of feedback.

Can you make this line more readable. For example:

$og_image_tag = 'og:image';
if ( 0 === strpos( $img, 'https://' ) {
    $og_image_tag .= 'secure_url';
} 

$this->og_tag( $og_image_tag, esc_url( $img ) );

This comment has been minimized.

Copy link
@apfelbox

apfelbox Jun 19, 2017

Author Contributor

@andizer updated.

This comment has been minimized.

Copy link
@apfelbox

apfelbox Jun 19, 2017

Author Contributor

Also added some tests

@apfelbox apfelbox force-pushed the apfelbox:secure-image branch from 4c115ac to 06e7e6d Jun 19, 2017
$this->og_tag( 'og:image', esc_url( $img ) );
$og_image_tag = 'og:image';

if (0 === strpos($img, 'https://'))

This comment has been minimized.

Copy link
@andizer

andizer Jun 20, 2017

Contributor

If you can fix the codestyle in this line:
if ( 0 === strpos( $img, 'https://' ) ) {

This comment has been minimized.

Copy link
@andizer

andizer Jun 20, 2017

Contributor

You can setup the codestyle locally

composer install
composer config-yoastcs

You've to do these two steps one and you can run the codestyle check by doing this command: vendor/bin/phpcs

This comment has been minimized.

Copy link
@apfelbox

apfelbox Jun 20, 2017

Author Contributor

Thanks for the explanation on how to run the code style linter locally.

I looked for docs about how to run the test suite locally but couldn't find anything.

@apfelbox apfelbox force-pushed the apfelbox:secure-image branch from 2f3c865 to 92ba83b Jun 20, 2017
@moorscode

This comment has been minimized.

Copy link
Member

moorscode commented Jun 21, 2017

Hi @apfelbox

Thank you so much for this PR!

I have one thing that would need to change, as I understand the specifications from http://ogp.me/#structured the og:image tag should be present and the og:image:secure_url can be added when the URL is HTTPS.

Thus this would mean that an additional $this->og_tag would be required to add both meta fields.

Could you change the PR according to these specs?

@apfelbox

This comment has been minimized.

Copy link
Contributor Author

apfelbox commented Jun 21, 2017

@moorscode I was reading the spec too and it is quite unclear.

I can add both tags, but the issue is that if the image available via HTTPS only (which probably is or will soon be the standard) the regular tag would be a redirection or dead link.

Should I add it nevertheless?

@apfelbox apfelbox force-pushed the apfelbox:secure-image branch 3 times, most recently from 91f545c to 16842a2 Jun 26, 2017
@moorscode

This comment has been minimized.

Copy link
Member

moorscode commented Jun 26, 2017

Making a request to the actual file to see if it exists will cause problems on multiple environments, as well as cause performance issues, so we cannot verify if the link is valid or not.

Having the og:image:secure_url should be used before the normal one when supplied, so this should not cause any problems.

Please add it.

@apfelbox apfelbox force-pushed the apfelbox:secure-image branch from 16842a2 to 9355f38 Jun 26, 2017
@apfelbox

This comment has been minimized.

Copy link
Contributor Author

apfelbox commented Jun 26, 2017

Okay, so I just updated the PR.

In the spec, it is not explicitly stated whether og:image can contain a HTTPS URL or if it needs to be a HTTP one.
In the sharing docs of facebook (the biggest open graph consumer) for videos it is stated, that one should use a https url even in the regular tag. I know that Facebook doesn't correctly parse a HTTPS URL in the og:image tag, so I have no idea what the correct way for images is.

At least, this fixes the issues with the least amount of additional code as possible.

This PR is ready @moorscode @andizer

@moorscode

This comment has been minimized.

Copy link
Member

moorscode commented Jun 27, 2017

Tested and accepted, thank you!

@moorscode moorscode merged commit 23d33be into Yoast:trunk Jun 27, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@apfelbox apfelbox deleted the apfelbox:secure-image branch Jun 27, 2017
@calphaomega

This comment has been minimized.

Copy link

calphaomega commented Jul 6, 2017

Is working now only on desktop/laptop. On mobile if i put the link on fb messenger is not working.
If i share on my personal page the image is not loading. But on a company page is ok.

I think the issue is not solved with the new update. Because i already made the update to version 5.0 and is still the same problem.

Can i get some help here?

@kjy112

This comment has been minimized.

Copy link

kjy112 commented Nov 5, 2017

On some HTTPS websites, even with this update, FB is still not pulling in the image at first share. To fix this on one of my websites, I output the og:image:secure_url first and this fixed the issue.

if ( 0 === strpos( $img, 'https://' ) ) {
     $this->og_tag( 'og:image:secure_url', esc_url( $img ) );
}
$this->og_tag( 'og:image', esc_url( $img ) );

May want to strip the s for og:image to reflect none-SSL image link.

@julian-eckhardt

This comment has been minimized.

Copy link

julian-eckhardt commented Dec 1, 2017

@kjy112 haven't digged into Wordpress plugins/internals so far, but I'm having the same problem over here… Could you please tell me how to apply your fix?

@kjy112

This comment has been minimized.

Copy link

kjy112 commented Dec 1, 2017

@epdplus I can try to help please drop me a DM on twitter same handle as here @kjy112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.