-
Notifications
You must be signed in to change notification settings - Fork 14
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
Issue-124: Add Support for Open Graph Tags on Posts & Pages (v2.0) #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, this looks great! I flagged a couple little things. 👍
src/features/class-open-graph.php
Outdated
get_post_types_by_support( 'open-graph' ), | ||
'wp_seo_open_graph_title', | ||
[ | ||
'sanitize_callback' => 'wp_kses_post', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably use sanitize_text_field
here. I don't think we allow/need any html, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Updated in 14ef3a6
src/features/class-open-graph.php
Outdated
get_post_types_by_support( 'open-graph' ), | ||
'wp_seo_open_graph_description', | ||
[ | ||
'sanitize_callback' => 'wp_kses_post', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 14ef3a6
src/features/class-open-graph.php
Outdated
get_post_types_by_support( 'open-graph' ), | ||
'wp_seo_open_graph_image', | ||
[ | ||
'sanitize_callback' => 'wp_kses_post', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably use intval
since it should always be a number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 14ef3a6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually went with absint
here instead to ensure positive integer and better fallback to 0. Updated in 14ef3a6
src/features/class-open-graph.php
Outdated
$open_graph_image = get_post_meta( $post_id, 'wp_seo_open_graph_image', true ); | ||
|
||
if ( ! empty( $open_graph_image ) && is_numeric( $open_graph_image ) ) { | ||
return wp_get_attachment_image_url( (int) $open_graph_image, 'full' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would probably be safest to get the url, then check if it's set and not a wp_error, then return it. The media could get deleted from the media library after being set in the meta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I've made that update in 4d80003
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional update in 4d9b484
tests/Feature/OpenGraphTest.php
Outdated
] | ||
) | ||
->create(); | ||
$this->assertEquals( 007, Open_Graph::get_image( $post_id ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is failing because get_image returns the url of the image (or false in this case because the image doesn't exist), not the id. There should be a way to load an image into the media library, then get its url. #cc_testing should be able to help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It took me a bit to sort out this but I think I came to a pretty good solution here: d123964. Post meta source is not identical but I think it works. I also figured out how to run phpunit tests locally in the process (much better than pushing to the remote branch 🤕 )
Summary
Fixes #124. This pull request introduces a Gutenberg-native solution for managing Open Graph tags on individual posts and pages.
Description of Changes
Additional Notes
Testing Instructions
Related Issue
#124