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

9.0.1: admin/links/class-link-watcher.php calling apply_filter the_content outside of The Loop, without context setup, is probably still a bad practice #11395

Open
3 of 11 tasks
lkraav opened this issue Oct 24, 2018 · 2 comments

Comments

@lkraav
Copy link

lkraav commented Oct 24, 2018

  • I've read and understood the contribution guidelines.
  • I've searched for any related issues and avoided creating a duplicate issue.

Please give us a description of what happened.

Some plugins are reasonably expecting global $post context to be correctly set during executing the_content filter.

If this filter is executed outside of The Loop, you're likely to generate a ton of PHP Notices like $post->ID trying to get property of non-object.

This has been discussed a while ago https://core.trac.wordpress.org/ticket/24330#comment:3

Notable quote from @markjaquith reads:

WordPress core has, for years, relied on the convention that the_content filters are meant to be run inside a loop context with a valid global $post object. Multiple WordPress core functions already make that assumption, like gallery_shortcode() and prepend_attachment(), as well as all our embed handlers (which do caching by post ID). Any third party shortcode that touches post meta will be in the same boat, and won't work if $post isn't as expected.
It's not very clean, but it is expected and well-established behavior.

Please describe what you expected to happen and why.

There might be only one hypothetically low-cost improvement option:

Manually set up global $post while executing your apply_filters the_content call + clean it up after, to allow all "downstream" plugins to properly process incoming data.

Pseudo-code (or maybe real code?):

	private function process( $post_id, $content ) {
                global $post;
                $post = get_post( $post_id );
                setup_postdata( $post );
		// Apply the filters to have the same content as shown on the frontend.
		$content = apply_filters( 'the_content', $content );
		$content = str_replace( ']]>', ']]>', $content );
                wp_reset_postdata();

		$this->content_processor->process( $post_id, $content );
	}

Otherwise you're breaking your own function promise of // Apply the filters to have the same content as shown on the frontend., which simply cannot happen if all plugins involved in the chain don't have the core context setup they have a right to expect.

How can we reproduce this behavior?

  1. Install a legacy plugin like https://wordpress.org/plugins/q-and-a/ which noone should be using, but it demonstrates the problem well enough
  2. Take a look at a specific issue traceback like https://sentry.crdy.co/share/issue/a7b708920e5e4cf2a3b67d48235ef06f/
  3. Run wp_update_post() somewhere to trigger class-link-watcher.php

Technical info

  • WordPress version: 4.9.8
  • Yoast SEO version: 9.0.1
  • If relevant, which editor is affected (or editors):
  • Classic Editor
  • Gutenberg
  • Classic Editor plugin
  • Which browser is affected (or browsers):
  • IE11
  • Edge
  • Chrome
  • Firefox
  • Safari
  • WP-CLI
  • Relevant plugins in case of a bug:
  • Tested with theme:
@lkraav lkraav changed the title 9.0.1: admin/links/class-link-watcher.php calling apply_filter the_content outside of The Loop is probably still a bad practice 9.0.1: admin/links/class-link-watcher.php calling apply_filter the_content outside of The Loop, without context setup, is probably still a bad practice Oct 24, 2018
@moorscode
Copy link
Contributor

Hi @lkraav

Thanks for raising this issue.
I agree that this should be changed, we had not considered the WP-CLI implications on this functionality.

We would warmly welcome a patch for the suggested change.
It has been added to the list of items that will be implemented.

@leopoiroux
Copy link

+1

@IreneStr IreneStr added queue and removed next labels Jan 16, 2019
@Dieterrr Dieterrr self-assigned this Jan 17, 2019
@Dieterrr Dieterrr removed their assignment Jan 18, 2019
@Dieterrr Dieterrr self-assigned this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants