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

Use Jetpack_RelatedPosts::get_options() to get options for server-rendered Related Posts #14386

Conversation

westonruter
Copy link
Contributor

It was discovered by @jamesozzie that filtering options for rendering Related Posts does not work on AMP pages. For example, in response to a support forum topic he was looking to reduce the number of posts displayed from 3 to 2 with code like so:

function jetpackme_more_related_posts( $options ) {
    if ( function_exists('is_amp_endpoint') && is_amp_endpoint()) {
        $options['size'] = 2; //change the number for the amount required
    }
    return $options;
 }
add_filter( 'jetpack_relatedposts_filter_options', 'jetpackme_more_related_posts' );

This turned out not to work because AMP pages render the Related Posts using Jetpack_RelatedPosts::get_server_rendered_html() and this method was calling Jetpack_Options::get_option( 'relatedposts', array() ) rather than Jetpack_RelatedPosts::get_options(), and only the later method applies the jetpack_relatedposts_filter_options filter (as well as apply other normalizations).

This PR updates \Jetpack_RelatedPosts::get_server_rendered_html() to use Jetpack_RelatedPosts::get_options() rather than the lower-level Jetpack_Options::get_option( 'relatedposts', array() ).

Issue introduced in Jetpack 7.6 via #13028 for #9556.

Changes proposed in this Pull Request:

  • Ensure options for rendering Related Posts are filterable when rendering on AMP pages.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • If you're an Automattician, include a shortlink to the p2 discussion with Jetpack Product here.

Testing instructions:

  • Add a jetpackme_more_related_posts filter to change the size (see above).
  • Verify the size change is applied to AMP and non-AMP pages alike.

Proposed changelog entry for your changes:

  • Ensure options for rendering Related Posts are filterable when rendering on AMP pages. Use Jetpack_RelatedPosts::get_options() to get options for server-rendered Related Posts

…dered Related Posts

This ensures that options are normalized and that they can be filtered via jetpack_relatedposts_filter_options.
@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 44a310e

Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! LGTM

@gravityrail gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Bug When a feature is broken and / or not performing as intended AMP [Feature] Related Posts labels Jan 17, 2020
@jeherve jeherve added this to the 8.2 milestone Jan 20, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello westonruter! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37883-code before merging this PR. Thank you!

@jeherve jeherve merged commit 5e2bbbc into Automattic:master Jan 20, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 20, 2020
jeherve added a commit that referenced this pull request Jan 27, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP [Feature] Related Posts Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants