Shortcode contents don't display in non-standard BP 1.7 themes

[sensibleplugins]

BuddyPress does not display shortcode contents in theme parts when in theme compatibility mode.

We believe the bug is attributed to the following line of code in bp-core-theme-compatibility.php (line 525)

bp_remove_all_filters( 'the_content' );

The above line removes, amongst others, the ‘do_shortcode’ filter function from ‘the_content’ filter, and hence no shortcodes are ever executed.

[r-a-y]

Confirmed.

I'm not sure why we are wiping the filters so early. I think we should be doing this just before BP attempts to do its object buffering.

I've attached a quick patch, but could use some dev feedback.

Moving to 1.8.

[boonebgorges]

We're currently running bp_remove_all_filters() in bp_template_include_theme_compat(), which runs at 'template_include'. This is very late in the WP loading process, immediately before the templates begin to be loaded. It's possible that sensibleplugins is registering shortcodes after 'template_include' - for example, at 'wp_head', or maybe even inline in a template file. In contrast, WordPress adds its native shortcodes (such as [caption]) very early, as soon as wp-includes/media.php is included into the application. Therefore, I'd suggest that if you're loading your shortcodes after template_include, you're doing something wrong - in a theme, try 'after_setup_theme', and in a plugin, try 'init'.

That said, I'm still interested in r-a-y's question about why we are removing the filters as early as we are. It seems to me that moving the call to bp_remove_all_filters() to bp_buffer_template_part() would help with cases like sensibleplugins's. johnjamesjacoby, is there any reason why it couldn't be moved in this way?

[r-a-y]

attachment:5021.02.patch​ builds upon the approach from 01.patch.

bp_remove_all_filters() is now run on the 'loop_start' hook, which is the latest we can remove the filters before the_content() is run. Similarly, bp_restore_all_filters() runs on the 'loop_end' hook.

This will probably solve the issues that boonebgorges mentioned in comment:2.

Note: create_function() is used in my patch; if this isn't desired, we can create wrapper functions.

[r-a-y]

Updated to include better comments

[r-a-y]

Related: #4909.

[johnjamesjacoby]

Originally, the reason filters were removed from 'the_content' when they were, is because some functions in bbPress were using the 'the_content' filter in a way where it was possible to accidentally nest them. This is still currently possible with bbPress's shortcodes, which is why the output buffer start and end methods ​conveniently remove and add bbp_replace_the_content() from 'the_content' before putting out content to the output buffer.

Since we're not using 'the_content' anywhere else in BuddyPress at the moment, it's possible we can dial this into more narrow actions. I'd like to shy away from create_function() though, since I suspect we'll need to add more logic to these new functions over time.

A few things to consider here:

• Are there any issues with 5021.02.patch and loop_end and loop_start firing on all query loops?
• Does this accidentally trigger those actions on sidebars, etc?
• Are we accidentally running content through the 'the_content' filter twice? Or, are we skipping it on accident now?

[johnjamesjacoby]

Adds in_the_loop() calls

[johnjamesjacoby]

Remove is_main_query() calls

[johnjamesjacoby]

5021.3.patch incorporates the two helper functions I mention above, plus adds in_the_loop() checks to them to make sure they're only happening for the main loop. This ensures we're hitting the actual body content, and not other query loops.

This seems like the best approach to me, though I'm not actually sure if it fixes the bug that's initially reported here.

[johnjamesjacoby]

After looking at this a bit more, 5021.3.patch is the approach I'm going to take in bbPress for 2.4 also. These actions are the latest and earliest possible places to hook in and out of the loop content output, and we have sufficient knowledge of the current request to replace the_content in a way that maximizes compatibility with existing third party plugins.

Nice work tracking this down gents.

[johnjamesjacoby]

See ​[BB4972] for the exact change in bbPress. I'll commit this approach into BuddyPress momentarily, and if someone can confirm the issue is fixed, I'll let you close it.

[johnjamesjacoby]

In 7128:

Tweak theme compatibility to hook to loop_start and loop_end, to maximize compatibility with third party plugins and existing WordPress themes. Props r-a-y for the proof of concept. See #5021.

[r-a-y]

Add further conditions before removing all filters

[r-a-y]

JJJ, your comment (comment:5) made me think a bit more about when we should run BP's injection.

For example, what if there is a query_posts() loop before our main page loop in a theme? This could be a recent posts loop or a rotating slider, etc. (I know what you're thinking. Decent theme / plugin devs should always use a WP_Query post loop instead, but there are probably instances where query_posts() is being called.)

Because we don't do other checks for this, BP's injection will happen in that loop as well as the main page loop.

attachment:5021.4.patch​ checks to see if the current post loop has more than one post. If there is more than one post, then our injection shouldn't happen for that loop.

Also, I've added a marker after we've added our add_filter( 'the_content', 'bp_replace_the_content' ) hook because if we've already run our injection, then we have no need to do it again.

I might be overthinking this, but it's better to be safe!

Let me know what you think.

[boonebgorges]

Hey all - Thanks for your work on this so far.

r7128 does cause problems for plugins (or, at least, one of mine - BuddyPress Docs). Basically, because I'm using the_content() inside of my custom BP Docs loop, the BP theme compat filters are being run recursively, and my page loads with lots of nested Doc directories. This is not good :)

FWIW, I am *not* using query_posts(), but am invoking a new WP_Query object. I'm looking at the way that in_the_loop() works, and I don't think that it's a reliable method for testing whether we're in the main query - it seems to be run anytime you call the the_post() method on a WP_Query object.

I'm afraid that r-a-y's 5021.4.patch doesn't fix the issue for me. Not sure why.

ericlewis and I have spent the last hour or two messing around to figure out the cause. We don't have a complete grasp on the problem, but 5021.05.patch is what we had to do to fix the problem.

First, we manually remove the bp_theme_compat_main_loop_start from loop_start the first time it runs. This is a similar concept to r-a-y's "runonce" idea, but more direct. (It doesn't work to remove at loop_end, because the queries are nested - loop_end on the outer loop only runs *after* it's run on the inner loop.)

When we made this change alone, it reduced the infinite recursion to a single level of recursion: content was appearing only twice. We found that, by removing the re-adding of the bp_replace_the_content filter in bp_buffer_template_part(), the second level of recursion was avoided too. And really, there is no reason why we should be readding this filter in the first place: in what scenario would we want theme compat to run twice on the same page?

We don't totally understand why these changes fix the issue. The problems don't manifest with single post pages (like single Docs, or with bp-pages, which are single WP posts), but only with post type archives like those used in BP Docs directories. We're hoping that maybe JJJ will have some insight. In any case, the changes suggested in .05.patch seem in the spirit of r-a-y's .4.patch, but are more conservative and more guaranteed to work in all cases (at least, our experience is showing this).

[r-a-y]

The problems don't manifest with single post pages (like single Docs, or with bp-pages, which are single WP posts), but only with post type archives like those used in BP Docs directories.

This was the problem; I was only testing on single pages.

05.patch makes a lot of sense. My 4.patch's runonce idea isn't needed anymore since 05.patch removes the loop_start hook within bp_theme_compat_main_loop_start().

We found that, by removing the re-adding of the bp_replace_the_content filter in bp_buffer_template_part(), the second level of recursion was avoided too.

Yes, I missed this since we now add bp_replace_the_content in bp_theme_compat_main_loop_start(). Good catch!

---

After thinking about it more, we don't even need the 'loop_end' hook. We should just restore all filters in bp_buffer_template_part() after we've done our object buffering immediately.

06.patch implements these ideas as well as adds back the $wp_query->post_count check from 4.patch. I still think the post_count check is valuable for plugin or theme devs that might use query_posts() unnecessarily, but I'm open to removing this.

Let me know what you think.

[boonebgorges]

Hey r-a-y - 5021.06.patch looks mostly good to me. I agree that we don't need the loop_end stuff with the new technique.

I'm still not totally what the $wp_query->post_count check is intended to prevent. Can you give a specific example of what kind of thing it would prevent? I'm not sure I understand what query_posts() has to do with it - the truth is that (unless I'm missing something) we're not really doing any "is this the main loop" checks. if ( in_the_loop() ) will return true whenever we're inside of any WP_Query object, which we will always necessarily be at loop_start, even if it's not the *main* query.

What we're really doing with all of our logic is ensuring that only the _first_ WP_Query object constructed on a page load will trigger theme compat. 99.9% of the time, this will be the main query, and in those cases, the post count will always be 1 on a BP page (as long as we're using bp-pages). In the rare case where a plugin fires a WP_Query object _before_ the main query has been built (which is problematic in its own way - see ​https://core.trac.wordpress.org/ticket/20904), your post_count check will prevent theme compat from kicking in as long as that query returns more than one post. Is that what it's intended to do, or am I missing the larger picture? (Just to be clear - I don't think the post_count stuff is hurting anything, I just want to understand its purpose a bit better.)

[r-a-y]

I'm still not totally what the $wp_query->post_count check is intended to prevent. Can you give a specific example of what kind of thing it would prevent?

I'm thinking mostly from a theme / widget sidebar perspective.

If a widget added a post loop using query_posts() above the regular post loop like:
​http://pastebin.com/4yH2w2NZ (Twenty Twelve example)

You will see that without the post_count check, theme compat would run in that loop.

Also, I found that the_excerpt() seems to trigger theme compat as well due to the wp_trim_excerpt() function applying the_content filter.

Like I said in comment:13, I might just be playing it too safe!

if ( in_the_loop() ) will return true whenever we're inside of any WP_Query loop.

If you use a query_posts() loop, it will utilize the $wp_query global and ​in_the_loop() will return true because it is reliant on the $wp_query global. This is what I'm worried about.

If you use a WP_Query post loop, it doesn't use the $wp_query global. in_the_loop() will return false and we're safe from these instances.

Let me know if I'm not being clear enough!

[johnjamesjacoby]

5021.5.patch works around recursion by setting bp_set_theme_compat_active() to false when bp_replace_the_content() has started. bp_do_theme_compat() checks in_the_loop(), bp_is_theme_compat_active(), and a new function bp_is_template_included(), to make sure we're not accidentally running theme-compat the_content replacement on standard template files.

This should work with shortcodes, query_posts(), recursive calls to the_content filters, etc... without the added $wp_query->post_count logic.

[johnjamesjacoby]

After a bit more review, the main reason why everything was originally happening on template_redirect and template_include, was to prevent running all of WordPress's normal filters over the replaced content. bbPress trunk is having that issue now, with wpautop() running amok over the output, adding paragraph tags everywhere.

Looks like this will need a bit more tuning there too.

[johnjamesjacoby]

Remove _all_filters() juggling, for testing shortcodes and nested calls to 'the_content' filter

[johnjamesjacoby]

In 5021.6.patch, I obsoleted the need to juggle the filters that are added and removed to 'the_content'. This puts us in jeopardy of third party plugins that also plan on doing nefarious things with it, putting us at its mercy instead of having us stomp all over it. The plus side is that nested calls to the_content appear to work without a hitch.

[boonebgorges]

Thanks, johnjamesjacoby. This does seem to fix the recursion problem with nested queries.

Can you clarify the operative parts of the patch? I'm a little confused because there seems to be a bit of non-crucial refactoring going on too. You're now setting a different dummy post based on whether $wp_query->post isset. Is this the part that prevents theme compat from running on query_posts()? Or does it fix some other bug? It looks tro me like the bp_do_theme_compat() stuff is the important part of preventing recursion.

5021.07.patch removes the bbpress copypasta.

[johnjamesjacoby]

You're correct, that both bp_do_theme_compat() and setting bp_set_theme_compat_active( false ) early in bbp_replace_the_content() are what prevent the recursion.

You're also correct that the WP_Post stuff is unrelated to this specific bug; I can patch without it if it's distracting to the actual fixes.

[boonebgorges]

You're also correct that the WP_Post stuff is unrelated to this specific bug; I can patch without it if it's distracting to the actual fixes.

You know me - I like small changesets :) But really I just wanted some clarity. Thanks - and I think that these changes look good.

[johnjamesjacoby]

In 7131:

Iterate on theme compatibility content replacement order. Introduce bp_do_theme_compat() and other helper functions to avoid breaking on nested calls to the_content() or nested usages of 'the_content' filter.

Also, experiment with not adding/removing all_filters from 'the_content' for a bit, and see if any issues come up in testing with other plugins active.

See #5021.

[johnjamesjacoby]

In 7132:

Improvements to bp_theme_compat_reset_post() to use WP_Post class. See #5021.

[johnjamesjacoby]

The only thing to consider now is how specifically to handle shortcodes, and plugins that filter the content after priority 10:

add_filter('the_content', 'do_shortcode', 11); // AFTER wpautop()

Since we hook theme compat in on loop_start (and are now no longer removing all filters from the_content) we're hoping that plugins choose to behave and only use priority 11 or higher when it's absolutely necessary. Another side effect of this, is that any shortcodes that any user might try to post, will always get rendered. Shortcodes do not integrate with capabilities, and most shortcodes I've seen assume that the user is some kind of trusted blog author, which isn't the case in BuddyPress/bbPress.

I think we'll still need to juggle all of 'the_content' filters, and then decide in what scenarios we want to allow shortcodes to be rendered, and specifically hook those cases into their respective content areas.

[boonebgorges]

I think some good work has been done on this ticket, but I don't feel comfortable with what's been proposed to fix the specific shortcode issue (especially given jjj's good points about permissions in ​http://buddypress.trac.wordpress.org/ticket/5021#comment:24). Moving to Future Release.