-
Notifications
You must be signed in to change notification settings - Fork 800
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
MU WPCOM: Disable the Open Graph Tags according to the privacy of the site #39012
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
dfd3ef7
to
a272555
Compare
a272555
to
ea68063
Compare
projects/packages/jetpack-mu-wpcom/src/features/blog-privacy/blog-privacy.php
Outdated
Show resolved
Hide resolved
Coming soon:
Public & Prevent third-party sharing:
|
|
Maybe we should merge this first? Not sure if it's a dependency |
I found you have to enable either |
It should be resolved by 4fe84c6 |
0c3aae8
to
960f993
Compare
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.
Coming soon:
- ✅ Without Yoast SEO - No OG tags.
- ✅ With Yoast SEO - No OG tags.
Public & Prevent third-party sharing:
- ✅ Without Yoast SEO - No OG tags.
- ✅ With Yoast SEO - No OG tags.
960f993
to
085b381
Compare
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 wonder if wpcom_public_coming_soon
has other implications that Jetpack should be aware of or take into account itself. I don't think that's a blocker for this PR.
I think you should get a review from someone on a Jetpack team for this, though. It's been a while since I've been well-informed enough to have a trustworthy opinion :)
* Disable the Open Graph Tags based on the value of either wpcom_public_coming_soon and wpcom_data_sharing_opt_out option. | ||
*/ | ||
function remove_og_tags() { | ||
if ( ! (bool) get_option( 'wpcom_public_coming_soon' ) && ! (bool) get_option( 'wpcom_data_sharing_opt_out' ) ) { |
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 don't think wpcom_data_sharing_opt_out
is the right thing to check here. We don't, for example, turn off search engine access or social media expanded links if that setting is true.
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.
The value of wpcom_data_sharing_opt_out
is true when the Prevent third-party sharing for your site
is checked. As a result, I think it would be better to avoid sharing on social media as well.
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.
The third parties relevant for this option are Automattic's partners, not social media sites users share their content to. As described in the UI:
This option will prevent this [public] site’s content from being shared with our licensed network of content and research partners, including those that train AI models.
So turning off OG tags for public sites is outside of the original intent of this setting. I don't think the intent of this setting should expand to include social sharing. If you disagree, could you please make the case in a P2 discussion to get more perspectives involved?
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.
Will limit the changes to the Coming Soon 🙂
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.
Cool - thanks! Looks like the comment in the lines above needs updating too.
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.
Oh thanks! Updated 👍
I think we can simply focus on the OG tags in this PR to limit the scope 🙂 |
085b381
to
365f8da
Compare
if ( function_exists( 'jetpack_og_tags' ) ) { | ||
// @phan-suppress-next-line PhanUndeclaredFunctionInCallable | ||
remove_action( 'wp_head', 'jetpack_og_tags' ); | ||
} | ||
|
||
// Avoid calling check_open_graph as it registers the jetpack_og_tags function when running wp_head action. | ||
// @phan-suppress-next-line PhanUndeclaredClassInCallable | ||
if ( class_exists( '\Jetpack', false ) && is_callable( 'Jetpack::init' ) ) { | ||
// @phan-suppress-next-line PhanUndeclaredClassMethod | ||
$jetpack = \Jetpack::init(); | ||
remove_action( 'wp_head', array( $jetpack, 'check_open_graph' ), 1 ); | ||
} |
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 it be an option to simplify this logic and just force the Open Graph off with the jetpack_enable_open_graph
filter?
I haven't tested but I would expect something like this to work (maybe with some priority changes?)
if ( function_exists( 'jetpack_og_tags' ) ) { | |
// @phan-suppress-next-line PhanUndeclaredFunctionInCallable | |
remove_action( 'wp_head', 'jetpack_og_tags' ); | |
} | |
// Avoid calling check_open_graph as it registers the jetpack_og_tags function when running wp_head action. | |
// @phan-suppress-next-line PhanUndeclaredClassInCallable | |
if ( class_exists( '\Jetpack', false ) && is_callable( 'Jetpack::init' ) ) { | |
// @phan-suppress-next-line PhanUndeclaredClassMethod | |
$jetpack = \Jetpack::init(); | |
remove_action( 'wp_head', array( $jetpack, 'check_open_graph' ), 1 ); | |
} | |
add_filter( 'jetpack_enable_open_graph', '__return_false' ); |
If that works, I wouldn't be surprised if Yoast SEO offered a similar filter allowing one to shortcircuit their implementation.
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.
The check_open_graph
function would register the hook to turn on the open graph if certain modules are active so I'm not sure whether it's okay to do that.
However, after re-reading the code, the function also adds the following filter to turn off the open graph if there is any conflict plugin. So, I think I can simply add the same filter.
add_filter( 'jetpack_enable_open_graph', '__return_false', 99 );
If that works, I wouldn't be surprised if Yoast SEO offered a similar filter allowing one to shortcircuit their implementation.
The current way is recommended by https://developer.yoast.com/customization/yoast-seo/disabling-yoast-seo/. We can simply remove all actions for wpseo_head
but I think it would be better to follow the official suggestion. Does it make sense?
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.
but I think it would be better to follow the official suggestion
Yeah, definitely!
5b3452a
to
9d379fa
Compare
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.
Re-tested and LGTM 👍
… site (#39012) * MU WPCOM: Disable the Open Graph Tags according to the privacy of the site * changelog * Fix lint * Fix typo * Remove Jetpack OG only when the feature is enabled * Fix Jetpack OG * Fix lint * Remove OG tags only when the site is configured to Coming Soon * Update comment * Add filter to turn of Jetpack Open Graph Tags
Fixes Automattic/wp-calypso#92385
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: