Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 9 years ago

#6675 closed defect (bug) (fixed)

WP 4.4 deprecates wp_title()

Reported by: mercime's profile mercime Owned by: imath's profile 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)

6675.patch (4.3 KB) - added by imath 9 years ago.
6675.02.patch (4.2 KB) - added by imath 9 years ago.
6675.03.patch (5.8 KB) - added by r-a-y 9 years ago.
6675.04.patch (15.0 KB) - added by imath 9 years ago.
6675.05.patch (15.1 KB) - added by imath 9 years ago.

Download all attachments as: .zip

Change History (29)

#1 @slaFFik
9 years ago

Those new titles in WordPress 4.4 look very strange on a single activity page. Saying Page 745 - [Site Title]. Where 745 is bp_current_component() - activity id.

#2 @imath
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.

@imath
9 years ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 years ago

#4 @slaFFik
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 @slaFFik
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.

Last edited 9 years ago by slaFFik (previous) (diff)

#6 @imath
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 @imath
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.

@imath
9 years ago

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


9 years ago

#9 @DJPaul
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 @DJPaul
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.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


9 years ago

#13 @r-a-y
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() to bp_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 in wp_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!

@r-a-y
9 years ago

#14 @imath
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 the wp-title feature -> meaning the document_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 the wp_title function anymore -> meaning the wp_title filter won't be fired
  • There's a good chance BreadCrumbs plugins are still using wp_title although the theme is supporting wp-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 filter document_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 with bp_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 ?

@imath
9 years ago

#15 @r-a-y
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.

#16 @DJPaul
9 years ago

  • Milestone changed from 2.4.1 to 2.4.2

#17 @DJPaul
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 @DJPaul
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 @imath
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.

@imath
9 years ago

#20 @DJPaul
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 @r-a-y
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!

#22 @imath
9 years ago

Thanks a lot for your feedback @r-a-y :) I will commit asap.

#23 @imath
9 years ago

In 10402:

Make sure document/page title tags are correctly generated for BuddyPress pages

  • Remove all the BuddyPress process to create BuddyPress title parts from bp_modify_page_title() and create the new function

bp_get_title_parts() and put this process here. This allowes plugin/theme to directly use it if needed.

  • Keep the filter on wp_title so that themes not supporting the title-tag feature or breadcrumb plugins can still easily get BuddyPress title parts.
  • Filter document_title_parts with a new function bp_modify_document_title_parts() to build the BuddyPress title parts if needed.
  • Both functions bp_modify_page_title() and bp_modify_document_title_parts() are using bp_get_title_parts().

Props slaFFik, mercime, r-a-y, DJPaul

See #6675 (branch 2.4)

#24 @imath
9 years ago

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

In 10403:

Make sure document/page title tags are correctly generated for BuddyPress pages

  • Remove all the BuddyPress process to create BuddyPress title parts from bp_modify_page_title() and create the new function bp_get_title_parts() and put this process here. This allowes plugin/theme to directly use it if needed.
  • Keep the filter on wp_title so that themes not supporting the title-tag feature or breadcrumb plugins can still easily get BuddyPress title parts.
  • Filter document_title_parts with a new function bp_modify_document_title_parts() to build the BuddyPress title parts if needed.
  • Both functions bp_modify_page_title() and bp_modify_document_title_parts() are using bp_get_title_parts().

Props slaFFik, mercime, r-a-y, DJPaul

Fixes #6675 (trunk)

Note: See TracTickets for help on using tickets.