Skip to:
Content

BuddyPress.org

Opened 14 years ago

Closed 14 years ago

#2598 closed defect (bug) (fixed)

[patch] Forum topic title should be included in bp_get_page_title()

Reported by: r-a-y's profile r-a-y Owned by: djpaul's profile DJPaul
Milestone: 1.5 Priority: normal
Severity: Version:
Component: Forums Keywords: has-patch, needs-testing
Cc:

Description

There's some code by Boone in an old forum topic that could easily be added to address this issue:
http://buddypress.org/community/groups/how-to-and-troubleshooting/forum/topic/editing-group-and-forum-page-title-tags/#post-36358

If there's no time for this, feel free to bump to 1.3.

Attachments (5)

2598.diff (1.3 KB) - added by boonebgorges 14 years ago.
2598-2.patch (1.2 KB) - added by boonebgorges 14 years ago.
2598-3.patch (659 bytes) - added by boonebgorges 14 years ago.
2598-4.patch (872 bytes) - added by boonebgorges 14 years ago.
2598-5.patch (884 bytes) - added by DJPaul 14 years ago.

Download all attachments as: .zip

Change History (37)

#1 @hnla
14 years ago

FWIW if this doesn't get added I will be having to preempt what I know will be a query raised by the other developers on a BP implementation I'm working on; their SEO*sigh* chap will leap on this as a major issue and be asking me to correct it, I would rather not let the apparent flaw be commented on so will have to implement Boone's solution in functions.php

#2 @boonebgorges
14 years ago

  • Component changed from Core to Forums

Boy, nothing comes easy, does it?

That code doesn't work anymore as written. Something changed around 1.2.3ish so that the $forum_template global is no longer populated around the time when BP is creating the page title. (I seem to remember some priorities changing around that time, anyway.) That means that bp_get_the_topic_title() was returning empty.

The attached patch takes a different swing at things, by putting some information about the current topic into the $bp global. It's not ideal, because it requires pulling up the topic an extra time, and because it relies on a bbPress function call. But it's more efficient (probably) then calling up the entire $forum_template twice, at least.

@boonebgorges
14 years ago

#3 @boonebgorges
14 years ago

  • Keywords has-patch needs-testing added
  • Summary changed from Forum topic title should be included in bp_get_page_title() to Forum topic title should be included in bp_get_page_title() (has patch)

#4 @r-a-y
14 years ago

I don't know about you... but the old code is still working for me fine on BP 1.2.5.2!
http://pastebin.com/1zdN7C7K

Can someone confirm? Plop in functions.php.

#5 @pisanojm
14 years ago

Ray,

I added your pastebin directly above and it works fine. The order was reversed in the topic title. I.E. after your patch it is now XXXX/Group/Site and was originally Site/Group/XXXX.

Using Single Install, 1.2.5.2 and WP 3.01

#6 @hnla
14 years ago

Boone that is odd that bp_get_the_topic_title() returns empty I have tested it in a function running from functions.php on 1.2.5.2 and it works perfectly

#7 @boonebgorges
14 years ago

Ugh, I know what was happening. I took out the bp_has_forum_topic_posts() check, and bbpress_init therefore wasn't firing. That's why bp_get_the_topic_title() was returning false. I suck. Here's another patch based on what r-a-y wrote (based on what I wrote).

#8 @boonebgorges
14 years ago

I should probably go eat lunch before I attempt any more patching :-D Correct patch attached as -3.patch.

#9 @hnla
14 years ago

:lol: perhaps I should have patched this one :p

#10 @r-a-y
14 years ago

Haha, it happens to the best of us, Boone!

Re: http://trac.buddypress.org/ticket/2598#comment:5

As an addendum, perhaps we should look at reversing the order for the entire page title in BP 1.3. Makes sense to me from an SEO standpoint.

#11 @boonebgorges
14 years ago

r-a-y - Agreed on rethinking the page title order for 1.3. For this patch I kept with the default BP component order.

#12 @hnla
14 years ago

Good point re reversing the order it should be the opposite way round which I forgot and have just changed. topic title if considered important needs to be first.

#13 @hnla
14 years ago

:( no I haven't changed

#14 @paulhastings0
14 years ago

  • Keywords changed from has-patch needs-testing to has-patch, needs-testing
  • Summary changed from Forum topic title should be included in bp_get_page_title() (has patch) to [patch] Forum topic title should be included in bp_get_page_title()

#15 @hnla
14 years ago

For reference:

Blog category tags displayed as slug? i.e hyphans not removed in words

If bp_page_title() is given a clean up in 1.3 then this thread perhaps ought to be checked:
http://buddypress.org/community/groups/requests-feedback/forum/topic/problem-with-title-tag-in-category-and-tag/#post-69554

#16 @johnjamesjacoby
14 years ago

What's the consensus boys? To patch or not to patch? :)

#17 @hnla
14 years ago

My Opinion? Patch to Boone's 2958.3 patch (no justification in this holding things up)

This addresses the main point of no topic title which is the real issue and gives us:

Site Name | Groups | Group Name | Forum | Topic title - for title tag string

For me I would say it's perhaps too verbose and prefer a variation on Boon's forum thread example and will possibly still extract the function and add to it:

$title = str_replace( ' | Groups', , $title );
$title = str_replace( ' | Forum', '| Topic', $title );

But this is I think a matter of choice? up to the individual to decide if they wish or prefer something a little different.

Hopefully we will start building a library of snippets functions people can use when the new codex is in place.

As for my previous comment not sure whether this bp_page_title() should be added to 1.3 as an enhancement ticket with a view to checking through thoroughly? The question of the hyphen in post category titles is one I think will have to be tackled with a user function for time being, shouldn't be difficult once the correct cat object 'name' or 'nice name' is found, did have a quick go but hadn't really time to hunt through WP codex.

#18 @hnla
14 years ago

That ought to have read 2598-3 (I've become lexdysic :( )

#19 @johnjamesjacoby
14 years ago

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

(In [3221]) Fixes #2598 props boonebgorges

#20 @hnla
14 years ago

  • Priority changed from normal to minor
  • Resolution fixed deleted
  • Status changed from closed to reopened

With a view to hyphenated (slug) category name a quick? fix is to add:

$title = str_replace('-', ' ', $title);

`} else if ( is_category() ) {

$title = ( 'Blog | Categories | ' . ucwords( $wp_query->query_varscategory_name? ), 'buddypress' );

$title = str_replace('-', ' ', $title);`

This, imho, is trivial issue that ought to be looked at for 1.3 but mentioned by a user, ticket re-opened but ought to be moved to 1.3? Please feel free to close at will :)

#21 @johnjamesjacoby
14 years ago

  • Milestone changed from 1.2.6 to 1.3

Bumping to 1.3. Don't think we want to modify user entered strings if we can help it.

#22 @hnla
14 years ago

Agreed best bumped to 1.3

'Modifying user string' it's not a question of modifying user string, is it? We are taking the slug and spitting it out to browser where text should be; in human readable / seo / assistive devise terms we expect 'This is a title' rather than 'this-is-a-title'

#23 @johnjamesjacoby
14 years ago

For localization purposes we can't rely 100% on the slug translating to the description.

I agree that this does need some tlc though.

#24 @hnla
14 years ago

Take your point but had thought that simply preg_replace /str_replace / ereg_ or whatever on the hyphens to white space wouldn't have caused a localization issue.

But regardless yes a bit of tlc when appropriate in 1.3. This isn't a huge big issue but one that could be irritating to users and not great from SE description perspective.

#25 @boonebgorges
14 years ago

2598-4.patch is a less hackish solution for category archive pages. The only downside is that it means an additional query. FWIW WP itself does something similar in wp_title().

Thoughts?

@DJPaul
14 years ago

#26 @DJPaul
14 years ago

2598-5.patch allows for localisation. The entire function needs to be re-written in the same way.

#27 @DJPaul
14 years ago

Have a read of this -- http://daringfireball.net/2010/12/title_junk

I agree with Mr Gruber's "Source: Headline" preference.

#28 @r-a-y
14 years ago

Kind of off-topic, but I totally agree with that article.

It's a throwback to using the <title> tag for what it was originally intended; though I do prefer "Headline - Source" myself.

I say this because if all listings on Google were like "Source: Headline" and you're making a search on one domain only (eg. "site:domain.com" on Google), it would make it particularly hard to find the page you want to view.

#29 @DJPaul
14 years ago

  • Owner set to DJPaul
  • Priority changed from minor to normal
  • Status changed from reopened to assigned

#30 @DJPaul
14 years ago

  • Milestone changed from 1.3 to Future Release

I'd personally love to change the page title around, but punting this to a future release

#31 @DJPaul
14 years ago

  • Milestone changed from Future Release to 1.3

#32 @djpaul
14 years ago

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

(In [4536]) Rework page <title> generation to use WP's wp_title(). Fixes #2598

Note: See TracTickets for help on using tickets.