Opened 9 years ago
Closed 9 years ago
#6675 closed defect (bug) (fixed)
WP 4.4 deprecates wp_title()
Reported by: | mercime | Owned by: | imath |
---|---|---|---|
Milestone: | 2.4.3 | Priority: | normal |
Severity: | major | Version: | |
Component: | Core | Keywords: | has-patch commit |
Cc: |
Description
https://make.wordpress.org/core/2015/10/20/document-title-in-4-4/
This time, only Site Titles remain in <title>
, no more component slug nor e.g. member/group name, unlike in #6107 where issue was missing Site Titles.
Attachments (5)
Change History (29)
#2
@
9 years ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to 2.4
6675.patch is my suggestion to keep our title when WordPress 4.4 will be released. As beta 1 has just been published. I think we should consider including it into 2.4.0.
I've tested it in WP 4.3 & WP 4.4 and it seems to work fine.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#4
@
9 years ago
Tested the patch, for me it's only partially fixing the issue.
Trunk BuddyPress, this patch, theme 2015.
URL: http://trunk.bp/members/slaffik/activity/745/
Title before: Page 745 - BuddyPress Trunk
Title after: Activity - slaFFik - Page 745 - BuddyPress Trunk
Note: Page 745
could be removed, I think.
URL: http://trunk.bp/members/slaffik/
Before: BuddyPress Trunk
After: slaFFik - BuddyPress Trunk
Note: having component name like for single activity item would be nice.
URL: http://trunk.bp/members/slaffik/messages/view/84/
Before: Page 84 - BuddyPress Trunk
After: Activity - slaFFik - Page 84 - BuddyPress Trunk
Note: Page 84
could be removed, I think.
#5
@
9 years ago
Also SuperGroup > Manage > Settings
doesn't generate Settings
in the title with the patch attached.
Though slaFFik > Messages > Compose
does generate Compose - Messages - slaFFik - BuddyPress Trunk
title.
#6
@
9 years ago
As discussed yesterday in slack, i've just understood why slaFFik and i are not just viewing a notice error, but a wrong title on single item pages.
That's because we are testing using a theme that adds support for the title-tag
. In this case the wp_title()
function is not used in theme's header and bp_modify_page_title()
is simply never fired :)
- If you test using twentytwelve, you'll see the notice error with a good title because this theme doesn't add support for the
title-tag
. - If you test using twentyfifteen, you'll see no notice error and a "wrong" title for BuddyPress single item pages.
So the issue is more problematic for themes supporting the title-tag
feature. I'm going to keep on exploring
Also @slaFFik, about
Also SuperGroup > Manage > Settings doesn't generate Settings in the title with the patch attached.
I'm afraid this never existed in BuddyPress. See https://buddypress.trac.wordpress.org/browser/tags/2.1/src/bp-core/bp-core-filters.php line 463 to 466 - You only have the current single group nav and the group's name into the title.
That's because the Group navigation is a sub nav of the user main navigation (It's a subnav of Membership which uses the group's slug as its slug), meaning it has no $bp->bp_nav
entries. As the displayed user has $bp->bp_nav
entries, in your example you're getting 'Message' which is an item of $bp->bp_nav
and 'Compose' which is an item of $bp->bp_option_nav
. So this part: the Group's title tag is not a bug, it's more an enhancement we'll be able to work on as soon as #6534 will be achieved :)
So i'm going to test the patch in twentytwelve and twentyfifteen using WordPress 3.8 to 4.2.
#7
@
9 years ago
6675.02.patch is improving 6675.patch by:
- making sure to choose the right filter once the theme is set up.
- removing the
Page [Activity ID]
as i agree with slaFFIik, an activity ID is not a page and there's no reason to have this part in the title.
I've tested 6675.02.patch from WordPress 3.8 to trunk and it's working fine on each version with themes supporting or not the title-tag
When a theme is not supporting the title-tag
it's important to note that BuddyPress can't do anything about the notice error. It's theme's responsibility to use the WordPress way of generating the title tag as explained in the make article :
This latest change makes the new API (almost) feature complete and theme authors are discouraged from using wp_title() in the future
It's also important to note that even if the theme is still using wp_title()
the patch makes sure the title of the BuddyPress pages are still built the right way.
Finally, with this patch applied and with WordPress 4.4, it's also important to know that we could have a case where the active theme supports the title-tag
and the site's administrator has activated a breadcrumb plugin still using the wp_title()
function in this case the breadcrumb would get a notice error and a null value as the page title. If the breadcrumb is using wp_get_document_title()
then it will get no notice error and the BuddyPress page title.
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
9 years ago
#9
@
9 years ago
Quick feedback:
We should use the existing return filter, no need to duplicate that and change its name slightly (our functions' name isn't changing, only what it is hooked to). The other arguments look the same.
#10
@
9 years ago
- Milestone changed from 2.4 to 2.4.1
After discussion in dev chat starting from above mention, we're going to look at this for 2.4.1. Potential impact is very small at this point; we have enough time between our release and WordPress 4.4 to do that.
#11
@
9 years ago
Update: "Un-deprecate wp_title()" in https://core.trac.wordpress.org/changeset/35624
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#13
@
9 years ago
6675.03.patch
builds on the work imath has done, but with the following changes:
- Removes the
'bp_modify_document_title'
filter and adds a'bp_title_parts'
filter above the fold. I think filtering the BP title parts is better as it will work for both title filters. - Moves the older title tag compatibility stuff into the
// 'wp-title'
conditional block as it is only relevant there. - Changed
bp_do_document_title()
tobp_setup_document_title()
and did a tiny bit of refactoring. - I've added a
todo
note that is similar to DJPaul's concern about re-using the'bp_modify_page_title'
filter in our new WP 4.4 document title conditional block. The reason I did not implement it is the older'bp_modify_page_title'
filter includes the sitename. The place where I want to re-apply the'bp_modify_page_title'
filter will not have the sitename added since WP 4.4 will automatically add it inwp_get_document_title()
. I'm not sure if we should re-use this filter. Need some feedback here.
Tested with Twenty Twelve, Twenty Fifteen, Twenty Sixteen on both WP 4.4-bleeding and WP 4.3.1.
Let me know what you think!
#14
@
9 years ago
- Keywords dev-feedback added
First, thanks a lot @r-a-y for your patch, i've tested it and i confirm it's working fine. But i had another idea after a very good night of sleep :)
Actually thinking about the reply i have to give to @DJPaul about why using 2 filters in 02.patch made me change my mind. If i was using 2 different filters that was because :
when we filter document_title_parts
we are returning an array and when we are filtering wp_title
we are returning a string so this:
The other arguments look the same
is not exactly true :)
About this particulare issue, I finally strongly believe we shouldn't think OR but AND. All the patches we've built so far was thinking OR : filter wp_title
if WP < 4.4 or document_title_parts
if WP >= 4.4.
But what if we filter both no matter what WP version is in use ? Some contextual infos :
- Themes using
wp_title
are not supporting thewp-title
feature -> meaning thedocument_title_parts
filter won't be fired - Configs using WP < 4.4 won't fire the
document_title_parts
filter - Themes supporting the
wp-title
feature are not using thewp_title
function anymore -> meaning thewp_title
filter won't be fired - There's a good chance BreadCrumbs plugins are still using
wp_title
although the theme is supportingwp-title
If we include both filters at the same time: we are making sure all these points will be taken care of. And when wp_title
will finally be deprecated, we will be able to also deprecate bp_modify_page_title
(oops spoken too quick... only once our required version is 4.4 :) ).
In this spirit, 04.patch is :
- moving the part of
bp_modify_page_title
that was building the BuddyPress title parts in its own function :bp_get_title_parts()
- keeping the wp_title filter the way it was
- adding a new function
bp_modify_document_title_parts
to filterdocument_title_parts
- removing
_bp_strip_spans_from_title()
because what we need to do is to remove the span count class for title parts and BuddyPress dynamic nav menus, which is now achieved withbp_remove_span_count()
What is the extra benefit, other than it works: plugins will be able to use bp_get_title_parts()
directly to get BuddyPress title parts if needed.
What do you think ?
#15
@
9 years ago
What is the extra benefit, other than it works: plugins will be able to use bp_get_title_parts() directly to get BuddyPress title parts if needed.
I like 04.patch
. I was thinking about splitting the title parts into a separate function myself when I was working on 03.patch
. +1!
My only minor thing is the new bp_remove_span_count()
function; it's only used in two places, so I don't know if it is worth introducing a new function for this. We might also want to remove commas in there as well since it might be used in number localization.
#17
@
9 years ago
The latest patch removes _bp_strip_spans_from_title
-- we need to stub the function and move it to the deprecated file.
#18
@
9 years ago
Also agree with previous @r-a-y comment about bp_remove_span_count
.
Also agree with @imath who said somewhere we need to handle commas (etc) used as the number separator. Remember some locales uses dashes and periods for this.
#19
@
9 years ago
@DJPaul finally i think we should keep _bp_strip_spans_from_title
because it's actually removing the complete span (which is including the commas @r-a-y mentionned in a previous comment).
That's what i'm doing in .05.patch. If we all agree on the approach, i'll run some additionnal tests in WP 4.3 to 3.8 to make sure everything is ok.
Tell me what you think about it.
#20
@
9 years ago
Sorry I was unable to look at this this earlier today, imath. I haven't tested the 05 patch, but it looks good.
#21
@
9 years ago
- Keywords commit added; dev-feedback removed
I tested 05.patch
with WP 4.4 and Twenty Sixteen as well as with WP 3.8 and Twenty Twelve and everything works.
In the patch, there's a space character that needs to be removed on line 2884 of bp-core-template.php
, otherwise it's good to go!
Those new titles in WordPress 4.4 look very strange on a single activity page. Saying
Page 745 - [Site Title]
. Where 745 isbp_current_component()
- activity id.