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

Related Posts: Remove the call to remove_filter( 'the_content', ...) #13804

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

kbrown9
Copy link
Member

@kbrown9 kbrown9 commented Oct 22, 2019

Fixes #13775

The filter filter_add_target_to_dom() removes itself from the the_content
hook when the site is in AMP mode. When a filter removes itself from a hook, a bug in Core causes the filter with the next highest priority to be skipped.

For more information on the Core bug see: https://core.trac.wordpress.org/ticket/9968

To avoid this bug, remove the call to remove_filter( 'the_content', array( $this, 'filter_add_target_to_dom'), ...). Instead, just rely on conditionals in the
filter_add_target_to_dom() method to control when related posts are added to the
content.

Related posts should not be added to the content when any of these three conditions are true:

  1. The post contains a Related Posts block.
  2. The post contains a jetpack-related-posts shortcode.
  3. The get_the_excerpt hook is being executed. This hook is executed when the related
    posts are being generated, and related posts should not be added to the related post
    content.

These conditions are already handled by the filter_add_target_to_dom() method. However, when the call to remove_filter( 'the_content', array( $this, 'filter_add_target_to_dom'), ...)is removed, the legacy related posts are added to posts that contain a Related Posts block. Fix this by removing the $content parameter from has_block('jetpack/related-posts', $content). Then, the has_block() method will determine whether the global $post contains a Related Posts block.

Changes proposed in this Pull Request:

  • Remove the call to remove_filter( 'the_content', array( $this, 'filter_add_target_to_dom'), ...).
  • Remove the $content parameter from has_block('jetpack/related-posts', $content).

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

  • Fixes an existing feature.

Testing instructions:

  1. Set up the test site:

    • The Jetpack and AMP plugins must be installed and activated.
    • AMP should be set to Standard mode.
    • The Related Posts module must be activated.
    • The site must have enough posts with related content, tags, and categories that related post are generated.
    • At least one post must use legacy related posts.
    • At least one post must use a Related Posts block.
  2. Add this code to a snipped plugin or your theme's functions.php:

add_filter( 'the_content', 'test_filter', 41 );

function test_filter( $content ) {
	return $content . ' TEST ';
}
  1. Navigate to a post that uses legacy related posts. Note that the 'TEST' text (which should display after the related posts) is missing.

  2. Apply this branch.

  3. Refresh the post with legacy related posts. Verify that the 'TEST' text is now displayed under the related posts.

  4. Navigate to a post that uses the Related Posts block. Verify that the legacy related posts are not displayed and that the 'TEST' text is displayed at the bottom of the post content.

Proposed changelog entry for your changes:

  • Fix a bug in Related Posts that causes the filter with the next highest priority on the 'the_content' hook to be skipped while in AMP mode.

Fixes #13775

The filter 'filter_add_target_to_dom()' removes itself from the 'the_content'
hook. Then, a bug in Core causes the filter with the next highest priority to
be skipped.

For more information on the Core bug see: https://core.trac.wordpress.org/ticket/9968

To avoid this bug, remove the call to "remove_filter( 'the_content', array( $this,
'filter_add_target_to_dom'), ...)". Instead, just rely on conditionals in the
'filter_add_target_to_dom()' method to control when related posts are added to the
content.

Related posts should not be added to the content under these three condtions:
1. The post contains a Related Posts block.
2. The post contains a 'jetpack-related-posts' shortcode.
3. The 'get_the_excerpt' hook is being executed. This hook is executed when the related
   posts are being generated, and related posts should not be added to the related post
   content.

These conditions are already handled by the 'filter_add_target_to_dom()' method. However,
legacy related posts are currently added to posts that contain a Related Posts block. Fix this
by removing the $content parameter from "has_block('jetpack/related-posts', $content)".
@kbrown9 kbrown9 added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Related Posts [Status] Needs Review To request a review from Crew. Label will be renamed soon. AMP labels Oct 22, 2019
@kbrown9 kbrown9 requested a review from a team as a code owner October 22, 2019 18:06
@kbrown9 kbrown9 self-assigned this Oct 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 D34334-code before merging this PR. Thank you!

@jetpackbot
Copy link

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: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against efc254c

@jeherve jeherve added this to the 7.9 milestone Oct 23, 2019
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 seems to work well in my tests. 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 Crew. Label will be renamed soon. labels Oct 23, 2019
@jeherve jeherve merged commit 5bed937 into master Oct 23, 2019
@jeherve jeherve deleted the fix/related_posts_fix_content_filter branch October 23, 2019 08:10
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
@mdawaffe
Copy link
Member

When a filter removes itself from a hook, a bug in Core causes the filter with the next highest priority to be skipped.

Thanks for fixing! I thought this core issue had been addressed years ago… :)

jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
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.

Related Posts on AMP: Lower-priority filter hooks are removed
5 participants