Skip to:
Content

Opened 16 months ago

Closed 15 months ago

Last modified 15 months ago

#5301 closed defect (bug) (fixed)

Theme compat: Reset post arguments should not set 'is_archive' to true

Reported by: michaello13 Owned by: r-a-y
Milestone: 1.9.2 Priority: highest
Severity: normal Version: 1.7
Component: Component - Core Keywords: commit
Cc:

Description

Dear BuddyPress team,

I just installed BuddyPress 1.9 and it broke links and images on the site (BuddyPress related).
Is there any fix for that?

Example: http://humancolony.org/members/

Thank you

Attachments (1)

5301.01.patch (3.7 KB) - added by r-a-y 15 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 @johnjamesjacoby16 months ago

Strange that both your images and CSS are broken. Your activity stream looks okay, but your register page is also broken. Can you confirm that going back to BuddyPress 1.8 fixes everything?

comment:2 @michaello1316 months ago

Yes, I have another site and I just updated from 1.8 and there is the same problem.

comment:3 @boonebgorges16 months ago

Confirmed. This appears to be a bug in Atahualpa. Was broken with [7386]. Investigating further.

comment:4 @michaello1316 months ago

Okay, thank you

comment:5 @michaello1316 months ago

So I downgraded and it works now. If you find/develop fix please let me know. Thank you.

comment:6 @boonebgorges16 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

OK, here's the situation.

Atahualpa is not using WP's native template loading logic. Its sole template is index.php, and then it uses custom logic to decide what should be shown on a given page. The relevant function for the purpose of BuddyPress content is bfa_post_bodycopy(). This function decides whether to display the entire post content (the_content()) or just an excerpt (the_excerpt()) based on a number of conditions. BP's theme compatibility is designed to work with the_content(), but strictly speaking, it mostly works with the_excerpt() too (because of some shared underpinnings).

The problem is with stripped HTML tags. Prior to 1.9, BuddyPress echoed its buffered content, effectively killing anything that came after it. So, it skipped any custom filters on stripped tags, such as those that appear in bfa_wp_trim_excerpt(). In #5156, we discussed this behavior and decided it was a bug; [7386] fixes it. So now, in the case of Atahualpa, we have an instance where (a) the theme is awkwardly deciding to use the_excerpt() instead of the_content(), *and* (b) it's stripping HTML tags from the excerpt as modified by BP's theme compat system, which results in a markup-free block of text.

As an immediate workaround, I recommend that Atahualpa users go to Dashboard > Appearance > Atahualpa Theme Options > Configure EXCERPTS, and change the value of "Don't strip these tags" to <form><ul><li><label><select><option><img><span><div><a><p>. This allows the necessary HTML tags through Atahualpa's excerpt filters.

The "correct" fix is probably for Atahualpa to use the_content() for BuddyPress pages, probably by another conditional switch in bfa_post_bodycopy() (or maybe via filter). I have no idea whether the Atahualpa devs would be open to such a fix; in the past they have ignored my patches, so I'm not eager to try.

I don't think there's anything for us to fix in BuddyPress, because this issue is really due to Atahualpa doing a number of odd things in a row. Closing as wontfix. If someone would like to post the "don't strip these tags" fix to the Codex, that'd be great.

comment:7 @johnjamesjacoby16 months ago

I'm okay with this outcome. The original theme compatibility approach was tested with bbPress and Atahualpa, and I made the decision to echo VS return as a brute force approach to circumvent these issues.

Other themes may have similar functionalities, so it's plausible we'll see similar tickets like this one for them. Let's keep a mental note of this ticket, just in case.

Last edited 16 months ago by johnjamesjacoby (previous) (diff)

comment:9 @johnjamesjacoby15 months ago

Commented on the support topic. I'm inclined to call this one a wontfix, even if it is really, really annoying. The only thing we can do is revert [7386], which is a step backwards for anyone trying to use this code correctly.

comment:10 @boonebgorges15 months ago

Does anyone have a copy of Thesis so we can see if the cause is exactly the same? (In the case of Atahualpa, it's HTML tag stripping from the_excerpt(); it'd be quite a coincidence if Thesis were doing the exact same odd thing.)

comment:11 @johnjamesjacoby15 months ago

You're right that it's worth confirming. I might have an old copy somewhere. If I do, I'll share it someplace more appropriate than here.

Where these issues intersect is in (originally) having theme compatibility completely stomp the_content and circumvent any additional processing VS (now) having it intercept the_content and letting filters and code continue to run its course without any further intervention from BuddyPress or theme compatibility.

comment:12 follow-up: @r-a-y15 months ago

  • Milestone set to 1.9.2
  • Priority changed from normal to highest
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from New buddypress 1.9 bug to Theme compat: Reset post arguments should not set 'is_archive' to true
  • Version changed from 1.9 to 1.7

Reopening ticket.

This problem never came up in the past because we were echoing the_content code immediately (see #5156, which fixed this behavior).

It turns out that BP is the problem.

The themes that have some type of template loader logic are all checking against what type of post the current page is.

When BP runs its theme-compat code, BP resets the post so 'is_archive' is set to true. For themes with template loader logic, they will load up their archive template and the archive template historically uses the_excerpt() causing all formatting to be stripped out.

I've attached a simple patch against 1.9-branch that switches out all of our reset post code so 'is_page' is set to true and removes the 'is_archive' portion.

Test this with the Attitude theme, which a user mentioned on the BP forums (props to him):
http://wordpress.org/themes/attitude

This should fix all the other themes like Thesis as well, but let's get some people testing the patch immediately.

Last edited 15 months ago by r-a-y (previous) (diff)

@r-a-y15 months ago

comment:13 in reply to: ↑ 12 @imath15 months ago

Replying to r-a-y:

Test this with the Attitude theme, which a user mentioned on the BP forums (props to him):
http://wordpress.org/themes/attitude

Hi r-a-y, i've just tested with Atahualpa and i confirm your patch fixed the trouble with this theme

comment:14 follow-up: @boonebgorges15 months ago

See also #5307.

I can't think of any negative side effects of this, but it would be nice for jjj to chime in on the switch to is_page (and the original justification for is_archive).

comment:15 in reply to: ↑ 14 @johnjamesjacoby15 months ago

Replying to boonebgorges:

See also #5307.

I can't think of any negative side effects of this, but it would be nice for jjj to chime in on the switch to is_page (and the original justification for is_archive).

My original thought was to have the arguments better match WordPress's paradigm, to have is_archive set for our directories, and is_single set for single groups, members, etc... Ironically, that never happened, and they're all set to is_archive, likely from copy/pasting, and in testing, it still worked in themes that behaved correctly.

Thinking back, this is probably a case of good intentions gone bad. Since so much in WordPress relies on the main query and its parameters (including the way we calculate theme compatibility) it does seem proper to set is_page to true in all cases, and introduce our own extra arguments as they are needed (for things like rewrite rules.)

I'm okay with this fix, and I'll do the same for bbPress in the next few days.

Nice work tracking this down.

comment:16 @johnjamesjacoby15 months ago

  • Keywords commit added

comment:17 @r-a-y15 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from reopened to closed

In 7735:

Theme compat: When resetting post, set 'is_page' to true.

Previously, when resetting the post, 'is_archive' was set to true.

As a consequence, certain themes with some form of conditional
template logic (like Atahualpa and Attitude) were loading their
archive template because of the 'is_archive' flag. This caused
formatting display issues because the archive template historically
uses the_excerpt() causing all formatting to be stripped out.

This commit changes all reset post calls so the 'is_page' parameter is
set to true. This should correctly use the regular page template for
theme compatibility for themes with conditional template logic.

Fixes #5301. (1.9-branch)

comment:18 @r-a-y15 months ago

In 7736:

Theme compat: When resetting post, set 'is_page' to true.

Previously, when resetting the post, 'is_archive' was set to true.

As a consequence, certain themes with some form of conditional
template logic (like Atahualpa and Attitude) were loading their
archive template because of the 'is_archive' flag. This caused
formatting display issues because the archive template historically
uses the_excerpt() causing all formatting to be stripped out.

This commit changes all reset post calls so the 'is_page' parameter is
set to true. This should correctly use the regular page template for
theme compatibility for themes with conditional template logic.

See #5301.

comment:19 @r-a-y15 months ago

Thanks for the feedback everyone!

I've left a reply on #5307, which should also address that ticket as well.

comment:20 @hnla15 months ago

I tested and closed #5307 as 'resolved' with a comment that we may think again about adding the patch in #5204 to add the body class 'page' ?

Note: See TracTickets for help on using tickets.