Skip to:
Content

bbPress.org

Opened 5 years ago

Closed 17 months ago

#2607 closed defect (fixed)

bbp_has_replies/topics broken on lastest WP Trunk

Reported by: thebrandonallen Owned by: thebrandonallen
Milestone: 2.6 Priority: high
Severity: major Version: 2.5.3
Component: Component - Search Keywords: needs-patch
Cc:

Description

[WP28623] breaks bbp_has_replies/topics. Since we set 's' in the $defaults array, the new change in trunk always thinks we're doing a search. So, when we're not actually doing a search, no results are returned.

Unsure about who's in the wrong here. It could be considered a backward incompatible change, but maybe it's just exposing the we're _doing_it_wrong. Figured I'd start here, and we can push upstream if necessary.

Attachments (2)

2607.01.patch (1.8 KB) - added by thebrandonallen 5 years ago.
Work around with back-compat
2607.02.patch (2.4 KB) - added by thebrandonallen 5 years ago.
Remove unused code

Download all attachments as: .zip

Change History (20)

#1 @netweb
5 years ago

  • Priority changed from normal to high

Thanks, came across this the other day with bbpress.org/buddypress.org and hadn't started looking at what upstream changeset it was that broke things.

Related: #WP11330 [WP28623] [WP28612]

#2 @thebrandonallen
5 years ago

  • Keywords has-patch added
  • Owner set to thebrandonallen
  • Status changed from new to assigned

Attached patch fixes the issue. Turns out, the code that was causing the problem has never actually been used. It was introduced with the initial version of the bbPress plugin, but it has never been used. I'm providing two versions of a patch.

The first patch keeps the original code more or less intact, but works around the change to WP. The only reason this would be necessary is if there were a plugins making use of the ts and rs parameters. Since bbPress never even made use of these, I doubt that there are any plugins using them. The second patch removes the unused code, which, simultaneously, fixes the change to WP.

@thebrandonallen
5 years ago

Work around with back-compat

@thebrandonallen
5 years ago

Remove unused code

#3 @thebrandonallen
5 years ago

  • Keywords dev-feedback added

The original issue has been fixed as of [WP28804], however, the "fix" in 2607.02.patch is still valid. As it stands, the 'ts' (for topics) and 'rs' (for replies) array keys will never be anything but empty. This means we're setting a variable for every call to bbp_has_topics and bbp_has_replies, but has not purpose. Leaving open for @netweb or @johnjamesjacoby to decide the fate of this ticket ;)

#4 @johnjamesjacoby
5 years ago

In 5467:

Only add the s argument to queries if rs or ts are set in their respective topics & replies queries.

See #2607, #WP11330, [WP28623] [WP28612], [WP28804].

#5 @johnjamesjacoby
5 years ago

In 5468:

Only add the s argument to global forum search queries if search-terms exist.

See #2607, #WP11330, [WP28623] [WP28612], [WP28804].

#6 @johnjamesjacoby
5 years ago

  • Component changed from Topics to Search
  • Milestone changed from Awaiting Review to 2.6

These should be fixed now, along with a third query for the global search.

Going to mark as closed. Please reopen as needed.

#7 @thebrandonallen
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#8 @tollmanz
5 years ago

One little issue slipped into the r5467. L147 uses $default_topic_search which is no longer defined. This can lead to PHP notices in certain conditions.

#9 @thebrandonallen
5 years ago

This was fixed in [5477].

#10 @coreymckrill4ttf
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The changesets related to this bug, [5467], [5468], and [5477] do not seem to be in the latest distributed version of bbPress (2.5.8 at the moment). Is this an oversight, or were they pulled out for some reason?

#11 @thebrandonallen
4 years ago

The changesets you mentioned were all for trunk, and were never applied to the 2.5 branch. Are you having an issue related to this?

#12 @coreymckrill4ttf
4 years ago

Ah, gotcha. Yeah, this bug breaks our whole forum. I am currently manually applying the changes every time I update to the latest 2.5.x.

#13 @thebrandonallen
4 years ago

You shouldn't have any issues. The initial report was based off of WP trunk, at the time, and it was fixed before an official WP release came out. I haven't seen anyone else report this. Could you give more details, so we can see if this is something we need to fix in the 2.5 branch?

#14 @coreymckrill4ttf
4 years ago

Thanks, @thebrandonallen

Basically, always including the 's' parameter in the query args for topics and replies causes is_search() to always return true, even when it's just showing a topic/reply thread, and therefore should be false.

Our theme uses is_search to conditionally do some other funky stuff to the query args, which then breaks the forum. So, as it turns out, it's really a problem with our theme, though it seems like getting a false positive on is_search() is still a significant bug...

#15 @thebrandonallen
3 years ago

  • Keywords has-patch dev-feedback removed
  • Resolution set to fixed
  • Status changed from reopened to closed

#16 @Cybr
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm afraid this bug is back. Tested in version 2.5.14.

Use case: https://wordpress.org/support/topic/conflict-with-bbpress-13/#post-9721655

#17 @johnjamesjacoby
22 months ago

  • Keywords needs-patch added
  • Milestone changed from 2.6 to 2.5.15

This is fixed in trunk (for 2.6) and wasn't ported to the 2.5 branch.

#18 @johnjamesjacoby
17 months ago

  • Milestone changed from 2.5.15 to 2.6
  • Resolution set to fixed
  • Status changed from reopened to closed

Just moving this back into 2.6 to mark as fixed.

Note: See TracTickets for help on using tickets.