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.

Sign up for GitHub

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

Jump to bottom

Related Posts: Fix bug that prevents related posts from displaying in AMP view #13273

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Related Posts: Fix bug that prevents related posts from displaying in AMP view #13273

merged 1 commit into from
Aug 22, 2019

Conversation

kbrown9
Copy link
Member

@kbrown9 kbrown9 commented Aug 20, 2019

Fixes #13260

This fixes a bug that prevents related posts from displaying in AMP view when the Sharing or Publicize modules are enabled. This bug is also present when the Yoast SEO plugin is activated.

In AMP view, the related posts are displayed by using a filter, filter_add_target_to_dom(), on the the_content hook here.

That filter is removed in the render_block function So, filter_add_target_to_dom() will only be called the first time that the_content filters are applied.

In the Publicize and Sharing modules, filters are applied to the get_the_excerpt hook early, and those filters apply the the_content filters. This causes the filter_add_target_to_dom() filter to be applied to the the_content hook before the content is actually being prepared. Then the filter is removed, so the filter isn't applied when it's needed.

Changes proposed in this Pull Request:

  • Fix this issue by preventing filter_add_target_to_dom() from calling render_block() when get_the_excerpt is in $wp_current_filter.

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

  • This fixes a bug in the existing Related Posts module.

Testing instructions:

Test on a site that has related posts.

  1. Enable the Related Posts module.
  2. Install and activate the AMP plugin.
  3. In the AMP settings, select 'Transitional'.
  4. Look at a post that has related posts in AMP view by adding ?amp to the URL. Observe that the related posts are displayed.
  5. Enable the Sharing module.
  6. Reload the post from step 4 and observe that the related posts are still displayed.
  7. Enable the Publicize module.
  8. Reload the post from step 4 and observe that the related posts are still displayed.
  9. Install and activate the Yoast SEO plugin.
  10. Reload the posts from step 4 and observe that the related posts are still displayed.

Steps 3 to 10 should be repeated with the Standard AMP mode.

Proposed changelog entry for your changes:

  • Fix a bug that prevents Related Posts from displaying in AMP view.

@kbrown9 kbrown9 requested a review from a team August 20, 2019 16:41
@kbrown9 kbrown9 added the [Type] Bug When a feature is broken and / or not performing as intended label Aug 20, 2019
@jetpackbot
Copy link

jetpackbot commented Aug 20, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against 9fa6348

@kbrown9 kbrown9 added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Aug 20, 2019
@jeherve jeherve added this to the 7.7 milestone Aug 21, 2019
jeherve
jeherve previously approved these changes Aug 21, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This makes sense to me. 👍

@htdat Do you think you could give that patch a spin, since you had a test site where you were running into the issue?

Thank you!

@jeherve
Copy link
Member

jeherve commented Aug 21, 2019

cc @westonruter who may be interested in this as well.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 21, 2019
@kbrown9
Related Posts: Do not add related posts to excerpts
9fa6348 
Fixes #13273

Check the $wp_current_filter array for 'get_the_excerpt' using
doing_filter() to ensure that related posts aren't added to excerpts.
@kbrown9 kbrown9 force-pushed the fix/dont-add-related-posts-to-excerpts branch from 4335d9b to 9fa6348 Compare August 21, 2019 15:21
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Aug 21, 2019
@htdat
Copy link
Member

htdat commented Aug 22, 2019 •
edited
Loading

@htdat Do you think you could give that patch a spin, since you had a test site where you were running into the issue?

I've tested this. It is working very well for my site whether or not I activate Sharing and/or Publicize modules.

jeherve
jeherve approved these changes Aug 22, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well for me too. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 22, 2019
@matticbot
Copy link
Contributor

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

@jeherve jeherve merged commit babdf02 into master Aug 22, 2019
@jeherve jeherve deleted the fix/dont-add-related-posts-to-excerpts branch August 22, 2019 11:09
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 22, 2019
jeherve added a commit that referenced this pull request Aug 23, 2019
@jeherve
Changelog: add #13273
1fdcd1a
jeherve added a commit that referenced this pull request Aug 27, 2019
@jeherve
7.7 changelog: initial set of changes (#13190)
190301b 
* 7.7 changelog: initial set of changes

* Changelog: add #13154

* Changelog: add #13134

* Changelog: add #12699 and many others

* Changelog: add #13127

* Changelog: add #13167

* Changelog: add #13225

* Changelog: add #13179

* Changelog: add #13173

* Changelog: add #13232

* Changelog: add #13137

* Changelog: add #13172

* Changelog: add #13182

* Changelog: add #13200

* Changelog: add #13244

* Changelog: add #13267

* Changelog: add #13204

* changelog: add #13205

* Changelog: add #13183

* Changelog: add #13278

* Changelog: add #13162

* Changelog: add #13268

* Changelog: add #13286

* Changelog: add #13273

* Changelog: add #12474

* Changelog: add #13085

* Changelog: add #13266

* Changelog: add #13306

* Changelog: add #13311

* Changelog: add #13302

* Changelog: add #13196

* Changelog: add #12733

* Changelog: add #13261

* Changelog: add #13322

* Changelog: add #13333

* Changelog: add #13335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@kbrown9 kbrown9

Labels
AMP [Feature] Related Posts [Pri] Normal Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
7.7
Development

Successfully merging this pull request may close these issues.

Related Posts / AMP: Related posts do not display when Sharing or Publicize are enabled.
6 participants
@kbrown9 @jetpackbot @jeherve @htdat @matticbot @westonruter