Opened 9 years ago
Closed 9 years ago
#6107 closed defect (bug) (fixed)
Missing site titles for BP pages in Twenty Fifteen and new themes which add support to the new title-tag feature
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 2.3 | Priority: | normal |
| Severity: | major | Version: | |
| Component: | Core | Keywords: | has-patch |
| Cc: |
Description
What shows up in browser tab and source code:
`Site-Wide Activity | ` instead of `Site-Wide Activity | Devsite Title ` or `admin | Profile | ` instead of `admin | Profile | Devsite Title ` and so forth.
WordPress 4.1 introduced add_theme_support( 'title-tag' ) which has been added to the functions.php file of Twenty Fifteen. The wp_title tag which BP directly hooks into in bp-core-filters.php via the bp_modify_page_title function is no longer present in said theme's <head>.
Adding theme support for title-tag in after_setup_theme is the recommended way for new themes to display page titles moving forward and we should expect new themes to follow suit.
Attachments (9)
Change History (70)
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
#4
@
9 years ago
What do you think of this plan ?
I think wp_title() (and bp_title() by association) are poor representations of what most people expect from title tags these days, and we should move towards not supporting enhancements to it going forward.
Patch and approach look fine to me. Works as expected, and moves us closer to dropping support for wp_title().
Note: I hate that we have to do all this, but once it's done, we can pretty much leave it alone.
#5
@
9 years ago
This missed RC (which is tagged but not announced). Objections if I punt it to 2.3 or a 2.2.1? I don't care about title tags on a (currently) very small subset of themes.
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
#8
@
9 years ago
- Milestone changed from 2.3 to 2.2.2
#6279 was closed as a duplicate. It looks like we should consider this for 2.2.2.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#11
@
9 years ago
@imath The patch looks good so far in testing with possible issues though with member user account screens.
On the staging site running Yoast with Yoast deactivated the BP new function bp_modify_page_title() looks to work as intended and on directory component and user account screens returns the component names and user names or 'your profile' for the logged in user.
With Yoast re-activated and where pages are manually configured to use placeholders i.e %%title%% | %%sitename%% these are correctly honoured and override the BP titles for directories.
On the user account screens I'm not getting any output though for username or logged in user string with Yoast re-activated; I'll look at that further to try and establish why, at this preliminary stage it may be something I've done.
I'll also try and find the various support threads on title issues so we can test those as well plus #6729 in the morning.
#12
@
9 years ago
May I suggest that we write some unit tests that demonstrate the various ways that plugins might interact with page titles - Yoast's technique being one of them - so that we have reliable ways of doing consistent tests? My head is spinning a bit just from thinking about all the plugin toggling and F5ing that hnla describes above :)
#13
@
9 years ago
Re-testing on a separate install - WP 4.1.1 / BP bleeding - twentyfifteen with Yoast installed:
Main issue we have is the lack of titles on the member user account screens with Yoast activated.
Yoast appears to hook at a late priority which upsets our filters.
add_filter( 'wp_title', 'bp_modify_page_title', 10, 3 );
The 'wp_title' filter needs to have a priority of '20' then the BP strings kick into life.
Sadly we can't simply leave it at this late priority it seems as when Yoast not activated that is too late a priority, WP has already run the output.
Obvious answer I guess is test for Yoast and change the filter priority but adjusting core for specific third party plugins can't be a good idea.
@boonebgorges Agree, clear tests are needed, this is one of those somewhat confusing ones to test and I've been going back and fourth trying to establish what we, WP, and Yoast are all doing in this game of title tag output and it does get confusing.
I'm fairly happy though that taking Yoast out of the picture and just running the updated function for title-tags appears to be working perfectly.
#14
@
9 years ago
@hnla if you are testing the patch with WP >= 4.0, then the filter 'wp_title' is never used by BuddyPress, it uses 'wp_title_parts' instead in order to avoid a mess with the 'title-tag' support. see my comment above
In other words, the patch is only using 'wp_title' if WP < 4.0 as 'wp_title_parts' was introduced in 4.0
So it can explain changing the priority of the filter like you did had no effect when testing on 4.1.1 :)
using 'wp_title_parts' is fixing the 'title-tag' trouble, i've left the 'wp_title' filter for backcompat. But it seems, unfortunately, Yoast is breaking our fix :(
#15
@
9 years ago
I have an idea!
if you look at the patch, there's this part:
if ( 'wp_title_parts' == current_filter() ) {
$return_array = true;
// Remove the wp_title filter as we don't need it in this case
remove_filter( 'wp_title', 'bp_modify_page_title', 10, 3 );
}
Which explains my above comment. But we could also force Yoast to consider BuddyPress title parts by moving up the checks that are below this part to be sure we are in a BuddyPress area and checking if has_filter( 'wp_title' ) != 'bp_modify_page_title' and if so:
- remove all filters on wp_title,
- use a _doing_it_wrong() with a message explaining: "if you want to filter the BuddyPress title please use: 'bp_modify_page_title_parts'"
What do you think ?
#16
@
9 years ago
forget my last comment, WordPress is filtering wp_title to sanitize it! That was a bad idea :(
#17
@
9 years ago
@imath Yep thought the 'wp_title' was simply fallback, it's the way it read to me so was a little surprised when the priority change affected my test case as at the moment this is simply testing for the newer title-tag functionality.
So it can explain changing the priority of the filter like you did had no effect when testing on 4.1.1 :)
The reverse was true though it did have an affect which surprised me so was planning on looking at just what WP does now, had to go out for a while so wanted to leave the note about filter priorities and Yoast.
#18
@
9 years ago
@hnla i think 6107.03.patch will definitively fix the problem
In this version of the patch, i keep the wp_title filter, but use a global if we already got the title_parts using wp_title_parts. This way, even if by default we will pass 2 times (because of Yoast !!) the title should be ok with a minimum of extra code.
Could you test it ?
#20
@
9 years ago
I don't totally understand the issue here (IMO if Yoast is doing something extremely rude that overwrites our own filters, then it should be Yoast's responsibility not to wipe out our content - either that, or they should stop filtering on BP pages). That being said, just from a readability point of view: using argument overloading in bp_modify_page_title(), and using the same function for both filters, makes the logic a bit hard to follow. Could we convert to having two separate callbacks - one for 'wp_title' and one for 'wp_title_parts' - and each would be a wrapper for the central logic of bp_modify_page_title()?
#21
@
9 years ago
@boone It may be rude but Yoast says to users construct your own page title tag parts so in this respect BP has to bow to Yoast's superior SEO handling and step aside and let it override what we have set. Only on member account screens does this not work as Yoast has no knowledge of them thus it's left to BP to provide for those.
Issue then becomes Yoast running it's filter later than ours, great for the main BP directories bad for our member account screens which get stripped out by Yoast.
@imath I think we have gone too far the other way now as we are running back and ensuring our filters are set so now stripping out Yoasts title tags strings in favour of BP ones.
We need to ensure we only run the filter aggressively where we return bp_is_user()?
#22
@
9 years ago
The problem with catering too much to Yoast's plugin is that it's just one plugin (popular though it may be). In what sense does Yoast have "no knowledge" of member account screens? Why is this not also true for group screens? Why is it not a problem on directories? Because we're using WP pages for directories? We should only juggle bp_is_user() etc if we are confident that this is going to be a problem with any such plugin; I don't want to make large architectural changes based solely on what Yoast is doing.
#23
@
9 years ago
I don't want to make large architectural changes based solely on what Yoast is doing.
This except s/Yoast/pretty much any plugin or theme/
#24
@
9 years ago
@boonebgorges
Why is this not also true for group screens?
Apologies, this is true of group single or members single - it's the nature of our groups and profiles not being 'real' pages or CPT's. Yoast provides ability to set pages,posts, cpt, taxonomies etc with their own user set title strings but isn't aware of our screens and it's filter simply strips out without anything to replace with, unless we run our filter on a later priority.
I agree and understand about not pandering to individual plugins in core, however I would guestimate that of half serious sites one in three are using Yoast, it's an extremely popular plugin.
More the point here might be that we(BP) are manipulating a WP function and other plugins/themes have licence to do that as well so it behoves BP to do this in such a way that is flexible and not aggressively dictating?
#25
@
9 years ago
More the point here might be that we(BP) are manipulating a WP function and other plugins/themes have licence to do that as well so it behoves BP to do this in such a way that is flexible and not aggressively dictating?
Sure, this sounds right to me. I just want to be sure that we're making the decisions based on these considerations, rather than how we interact with one popular plugin. Carry on :)
#26
@
9 years ago
That being said, just from a readability point of view: using argument overloading in bp_modify_page_title(), and using the same function for both filters, makes the logic a bit hard to follow. Could we convert to having two separate callbacks - one for 'wp_title' and one for 'wp_title_parts' - and each would be a wrapper for the central logic of bp_modify_page_title()?
My first idea was to stop using the 'wp_title' filter to let WordPress build the title for us using the 'wp_title_parts' filter instead, it appears it's not yet a good idea, because :
- 'wp_title_parts' was introduced in 4.0 and we support back to 3.6
- using a "mix" of both filters where we would use 'wp_title_parts' for the sites using WP > 4.0 , and eventually fallback to 'wp_title' for the sites using 3.6 < WP < 4.0, as @boonebgorges said, is confusing and hard to follow
- There are plugins that are filtering 'wp_title' without taking in account the filter BuddyPress is providing for its "pages" (in particular Yoast). This means our efforts to respect the WordPress way of building title using the 'wp_title_parts' filter would anyway & inevitably be cancelled by these plugins.
So 6170.04.patch is simplifying everything as it carries on filtering 'wp_title', changing the priority to 20. I think we can act this late because we are taking care of checking the page that is loading is a BuddyPress page.
I've tested it with twentyfifteen (which supports the title-tag feature) & twentytwelve (which is not supporting it and uses twentytwelve_wp_title() to filter 'wp_title') on WordPress trunk.
Then i've switched to 4.0 branch and tested twentytwelve again.
Patch seems to fix the issue and i have good hope, it will also be the case, when a plugin is filtering 'wp_title' before 20.
#27
@
9 years ago
There are plugins that are filtering 'wp_title' without taking in account the filter BuddyPress is providing for its "pages" (in particular Yoast). This means our efforts to respect the WordPress way of building title using the 'wp_title_parts' filter would anyway & inevitably be cancelled by these plugins.
Which appears our principle issue.
I was finding issues though on previous patch with the late priority on wp_title as it was overridding other plugins(Yoast) attempts to write title tags.
I'll run another series of tests with .04
One thought did occur to me yesterday running through things, which was whether we need to filter in such a global manner? Our real target for this filtering is to construct meaningful title parts for our single groups and member screens, perhaps we only run our filter for those instances and leave either WP or third party plugin to handle pages i.e our directories?
#28
@
9 years ago
Testing on WP 4.1.1, alternating themes between twentyfifteen & twentyfourteen for wp_title() & 'title-tag' implementations.
Patch 6107.04 checks out to work as expected and is better for the simplification.
The issue does remain with the necessary later priority though on the filter, this still acts to override anything a thirdparty plugin (Yoast) may have constructed and filtered in where BP component directories and single group/members are concerned.
As I still see this as the concern in that BP is dictating how the title tag reads and preventing alternative choices I propose a revision in:
Patch 6107.05.patch
I have added to the initial check for bp_is_blog_page() bp_is_directory() we then bail out and return $title allowing either WP to run the title tag string or thirdparty plugin.
For single groups or single members we still run out constructed strings, these work well and are where we really need to do the work.
It's a compromise in a sense, as our constructed strings for component directories are a little better than the WP version which is simply the page title, but anyone needing them to be better has the option of using Yoast or similar to beef them up.
Yoast provides two approaches to managing title tag strings as global for post/pages tax etc or individually for a given page or post via a meta entry thus users can construct quite specific strings for say the Activity directory page.
#29
@
9 years ago
I understand your point. I'm fine with it.
One remark. If we are to do this, i think we should take care of all BuddyPress pages. I think bp_is_directory() is set by the bp_update_is_directory() function eg:
bp_update_is_directory( true, 'groups' );
The register page for example, should be out of your check as :
bp_update_is_directory( false, 'register' );
#30
@
9 years ago
In other words 'register' is not a directory. I did consider 'register' & 'activate' but figured in reality what we set read ok and in reality title tags or SEO for these two BP screens was less important - perhaps that's a bad way of looking at it though.
If we include those two screens then in the bail out, in effect saying only manipulate title tags for single groups, single members, we can simply add bp_is_register_page() & activate to that early check removing the later $title_parts?
With the Group Create & Blog Create screens not being Pages I would suggest they were treated as the single groups/members are being, we'll manipulate the strings.
Can we get a consensus amongst the core devs then as to this notion that what we'll now end up doing is simply writing title tags for those screens that don't exist as pages and leave the others to WP or thirdparty plugins to handle
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
9 years ago
#32
@
9 years ago
Following on from a brief discussion with Boone in slack ( https://wordpress.slack.com/archives/buddypress/p1425666311000571)
I'm making the decision that we do let BP handle all title tags for BP screens as per Imath's patch, our later priority on the wp_title filter will overwrite any others.
In attempting to please everyone I was over complicating the issue.
We have corrected the problems outlined in this ticket and in #6279 which is the priority.
I am recommending we close out and commit imaths patch 6107.04.patch '.05' is now considered redundant.
#33
@
9 years ago
6107.06.patch builds off math's patch a few different ways:
- Adds a
is_buddypress()check at the beginning. This is basically the opposite ofbp_is_blog_page()but, I thinkis_buddypress()is a more oft suggested approach, and since it has its own filter is worth including regardless. - Removes the anonymous function for PHP 5.2 combat
- Refreshes for BuddyPress 2.2 branch
- Fakes a
current_theme_supports( 'title-tag' )check with a direct touch to the$_wp_theme_featuresglobal, and bystrstr()ing the$titleforblogname()to see if it was previously included. This maintains compatibility with core themes that are doing-it-both right and wrong (if you believewp_title()is ever right.)
We should probably write some tests for all this. Lots of plugins use wp_title() and lots of users get really mad when something narfs them up. Plus we've all spent a decent amount of time on this already, and tests will help ensure we can spend less time on this later.
#34
@
9 years ago
Here is (one reason) why this is a big problem:
- Since WordPress 4.1, themes that support the
title-tagfeature will automatically include the blog name, plus the description on the home page and pagination on paginated results. - TwentyTen does not filter
wp_titledirectly, and instead has the above additions hard-coded. - TwentyEleven does not filter
wp_titledirectly, and instead has the above additions hard-coded. - TwentyTwelve filters
wp_titleand adds the above additions via a filter at priority 10. - TwentyThirteen filters
wp_titleand adds the above additions via a filter at priority 10. - TwentyFourteen filters
wp_titleand adds the above additions via a filter at priority 10. - TwentyFifteen supports the
title-tagfeature, which means by the time we filterwp_titlethe above additions have been made by WordPress core. - Underscores just switched to using
title-tagwhich means all previous derivatives will be filteringwp_titleand all future ones will be usingtitle-tag.
There are 3 permutations in core themes, and I think we can safely assume any other themes will be using all of these approaches for years to come, and I have a hunch the number of Underscores forks out there is probably pretty high.
I'm confident my patch considers all of these permutations, but less confident it actually fixes issues with Yoast's SEO plugin, or any other plugin goofing around with wp_title. Like Boone said, I agree that we should probably own the <title> contents for our own pages, and any plugins wanting to improve them should do so purposely, and in such a way that accidentally breaking them is not possible without doing something egregious.
#35
@
9 years ago
A few other considerations I just noticed:
- As of WordPress 4.1, core accidentally broke (or maybe silently deprecated) the ability to have
leftseplocation. This is because the blog name, description, & pagination are always appended at the end, resulting in strings like| 2014 | JulyWordPress Developfor date archives. This should be reported upstream; see [WP30074] and #WP18548 for years worth of cringing. - We should probably add our own
$seplocationcheck above our blog name/description/pagination dance, to ensure we prepend or affix a$sepbefore or after it if necessary. This also mirrors whatwp_titleis already doing that we are stomping.
What a mess.
#36
@
9 years ago
Tested 6107.06.patch and results posted in spreadsheet.
https://docs.google.com/spreadsheets/d/11SqwCqyuapgENELaMGMnVpxyS-yhznGumAxTrzgvwAc/edit?usp=sharing [edit - 1st 2 columns are fixed changed to top 3 rows fixed]
- Fixes #6107 Missing site titles for BP pages in Twenty Fifteen and new themes which add support to the new title-tag feature
- Fixes #6279 Title page removed users and groups per report in https://buddypress.org/support/topic/title-page-removed-users-and-groups/
#38
@
9 years ago
but less confident it actually fixes issues with Yoast's SEO plugin, or any other plugin goofing around with wp_title. Like Boone said, I agree that we should probably own the <title> contents for our own pages,
Fixing the issue with Yoast, is less the issue now, given the acceptance that we have BP manipulate all the title strings for BP screens on a later priority than Yoast is filtering on.
The problem as raised in #6207 was in Yoast filtering wp_title but returning empty for BP single groups/single members, this is corrected by BP now filtering on priority '20' and saying in effect sorry Yoast for BP screens we'll be managing the title tag.
#39
@
9 years ago
@boonebgorges - I just needed to check what was what and where now.
@johnjamesjacoby Expanded test coverage of 6107.07.patch to include Twenty Ten and posted the results in the Google Spreadsheet linked to above.
Twenty Fourteen and Twenty Fifteen pass.
For Twenty Ten, there's a missing separator and spaces before the Site Title for default install. If Yoast SEO is activated, the separator and spaces show up but the Site Title shows up twice.
For fun, I tested BP Default theme and results were exactly the same as that for Twenty Ten.
#40
@
9 years ago
For Twenty Ten, there's a missing separator and spaces before the Site Title for default install. If Yoast SEO is activated, the separator and spaces show up but the Site Title shows up twice.
This seems like it's normal Twenty Ten behavior, even without a patch attached. And I suspect bp-default was last updated based on those changes to Twenty Ten.
Maybe my title_tag_compat logic needs tweaking, or maybe this just a genuine flaw with wp_title and WordPress 4.1?
@
9 years ago
Same as 6107.07.patch, but refactored _bp_strip_spans_from_title usage and removed home/frontpage logic from title-tag compatibility check
#42
@
9 years ago
6107.08.patch has been tested with the latest versions of all Twenty* themes and BP Default, and yields acceptable results for all of them.
mercime - can you verify:
- Twenty Ten -
Version: 1.8from latest trunk - Twenty Fourteen -
Version: 1.3from latest trunk - Twenty Fifteen -
Version: 1.0from latest trunk
#43
@
9 years ago
JJJ - Trunk versions of Twenty Ten, Twenty Fourteen, and Twenty Fifteen on new test installation.
6107.08.patch with no SEO plugin
- Fixes Twenty Ten, Twenty Fourteen, Twenty Fifteen, and BP Default themes.
6107.08.patch with Yoast WP SEO activated
- Fixes Twenty Fourteen and Twenty Fifteen themes. But for Twenty Ten and BP Default - same as the other two themes except duplicate site titles.
New spreadsheet https://docs.google.com/spreadsheets/d/1ww95Q184EAHG0ztI9MtkdjpA2pALnMY0L-R7FA6QqGc/edit#gid=0 Added the same data in old spreadsheet starting at row 39 for comparison.
#44
@
9 years ago
6107.09.patch includes an awful hack of a work-around to play nicely with Twenty Ten & Twenty Eleven, title-tag or not, and also with Yoast SEO activated or deactivated.
This patch pretty much only exists so I can say I figured out how to make it work with every core theme & permutation of Yoast, and not because it's the ideal solution.
In my opinion, 6107.09.patch completely sucks, and 6107.08.patch is silly, though somewhat less stupid.
#45
@
9 years ago
Confirming 6107.09.patch fixes page titles of Twenty Fifteen, Twenty Fourteen, Twenty Ten, and BP Default Themes whether WP SEO plugin is activated or not.
The latest spreadsheet has been updated and positive results from 6107.09.patch are highlighted in light orange. https://docs.google.com/spreadsheets/d/1ww95Q184EAHG0ztI9MtkdjpA2pALnMY0L-R7FA6QqGc/edit#gid=0
#46
@
9 years ago
Just in case it's not apparent and as it may skew results & testing just going to highlight the fact that Yoast WP SEO causes duplicated site title in twentyten & twentyeleven, confirmed with BP deactivated so we need to bear that in mind as we deal with the site title aspect so that we're not trying to 'mend' plugins.
I see the issue in that on our single members/groups screens tag that WP SEO doesn't handle 'ostensibly' we do see duplicate site titles, but this is an issue of a third party plugin rather than something BP is causing?
I can understand the issues we have but worrying it's getting a tad complicated. Going to have a good test of latest patch.
#47
@
9 years ago
Just in case it's not apparent and as it may skew results & testing just going to highlight the fact that Yoast WP SEO causes duplicated site title in twentyten & twentyeleven
@hnla Yes, that was noted in spreadsheets and comments above. Sorry if that was not made clear. When you open the latest spreadsheet, please scroll to the top and then scroll ~ 2 or 3 more columns to the right to see that the test results show the duplicate site titles when WP SEO is activated and results when latest patch is applied. Having said that, additional testing is always welcome.
#48
@
9 years ago
@mercime Yep sorry just wanted to emphasise the fact that WP SEO with or without BP activated has an issue.
#49
@
9 years ago
Sanity check please that latest Yoast WP SEO upgrade to 1.7.4 to fix "blind-sql-injection-vulnerability" borked fixes in patch. If it didn't, then fine. Otherwise, :(
#51
@
9 years ago
@mercime Just re-tested on twentyten and twentyfifteen - re-activated the now updated WP SEO plugin V 1.7.4 and get the same title tag results as the patch was/is producing for Dir & Members single screens, so think things are as before the Yoast update and as the last patch corrected to, but this was a speedy check.
#52
@
9 years ago
What's the verdict here y'all?
I think we can run with the 08 patch, and that 09 is too insane for core.
#53
@
9 years ago
A final comparison then between 09 and 08 and decision by end of weekend to lay this one to rest and be in a position to commit so we can push out for 2.2.2.
I'll run over both patches. Tend to agree though that 08, if cleaner and more maintainable, is the preferred choice; code that deals with the now rather than attempts to deal with all possible issues past.
I'll manually patch a client dev site which has a heap of custom functionality to deal with title-tags which will be a good test.
#54
@
9 years ago
I reverted out back to 08 to run a final tests and to update a staging site merging with it's existing customizations and all tested to work just fine, also ran through twenty* themes with Yoast activated/deactivated just for safety.
Not sure if 09 is too insane, think it cleverly works to resolve an issue but that this isn't really an issue that BP should worry about and adds complexity to an already complex function for a simple end result of title tags.
I recommend that we run with 08 as suggested and commit.
One final note in the 08 patch do we need both bp_is_blog_page() & is_buddypress I find 'bp_is_blog_page' confusingly named and doesn't 'is_buddypress' cover all bp screens for the requirement of bailing out?
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#58
@
9 years ago
- Milestone changed from 2.2.2 to 2.3
- Resolution fixed deleted
- Status changed from closed to reopened
Re-opening because the patch isn't in trunk and because of an error introduced - see #6369.
#59
@
9 years ago
When 08 is committed to trunk, please also make the change in r9741 (moving the nested function out into global scope).
Checking
current_theme_supports( 'title-tag' )seems impossible! it looks like it's only defined at wp_head with a priority of 1, very strange. So i gave up and choose another way as this support is really new in WordPress and might evolved.Since WordPress 4.0, there's an interesting new filter that can save us :)
'wp_title_parts'. So as we support WordPress down to 3.6, here's the plan i suggest :filter
'wp_title_parts'and'wp_title', most of the time (i hope so) we'll get'wp_title_parts', in this case we simply have toremove_filter( 'wp_title' ). In the first case, we need to return an array, if we are in the second case, we just need tojoin()this array with the$sep.This is what i have done in 6107.patch. I've tested it in TwentyFifteen and Twentytwelve and it works fine. In twentytwelve, i've removed the filter to
'wp_title_parts'in order to check it was still working, and it's the case :)What do you think of this plan ?