WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34456 closed defect (bug) (invalid)

Add add_theme_support('title-tag') to all default WordPress themes as wp_title() is deprecated since WordPress 4.4

Reported by: Nikschavan Owned by: obenland
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Bundled Theme Keywords: has-patch has-screenshots
Focuses: Cc:
PR Number:

Description

Now that the wp_title() is deprecated since WordPress 4.4, all the default themes before should have add_theme_support('title-tag');

https://make.wordpress.org/core/2015/10/20/document-title-in-4-4/

http://i.imgur.com/vwsLzds.png

The screenshot is from twentythirteen theme with WP_Debug enabled.

Attachments (7)

34456.1.diff (1.6 KB) - added by Nikschavan 4 years ago.
The patch fixes only for twentythirteen theme, Once this is reviewed, I will create refresh the patch for all the themes as well.
34456.2.diff (1.6 KB) - added by Nikschavan 4 years ago.
adds add_theme_support('title-tag') required in post WordPress 4.1 versions
34456.3.diff (1.9 KB) - added by Nikschavan 4 years ago.
Refreshed the patch
34456.4.diff (2.2 KB) - added by Nikschavan 4 years ago.
I noticed this was adding title two tiles on WordPress 4.3.1, because add_filter( 'wp_title', 'twentythirteen_wp_title', 10, 2 ); was also being fired along with add_theme_support( 'title-tag' ); add_filter( 'wp_title', 'twentythirteen_wp_title', 10, 2 ); to compat check, will only be fired if _wp_render_title_tag does not exists.
34456.5.diff (6.8 KB) - added by Nikschavan 4 years ago.
34456.diff (672 bytes) - added by obenland 4 years ago.
34456.6.diff (799 bytes) - added by obenland 4 years ago.

Download all attachments as: .zip

Change History (35)

@Nikschavan
4 years ago

The patch fixes only for twentythirteen theme, Once this is reviewed, I will create refresh the patch for all the themes as well.

#1 @Nikschavan
4 years ago

  • Keywords has-patch reporter-feedback added

@Nikschavan
4 years ago

adds add_theme_support('title-tag') required in post WordPress 4.1 versions

#2 @Nikschavan
4 years ago

  • Keywords dev-feedback has-screenshots needs-testing added; reporter-feedback removed

#3 @Nikschavan
4 years ago

Title in WordPress 4.0.8 -

http://screenshots.sharkz.in/nik/2015/10/2015-10-27_01-49-42.png

Title in trunk version of WordPress -

http://screenshots.sharkz.in/nik/2015/10/2015-10-27_01-48-14.png

This ticket was mentioned in Slack in #core-themes by nik. View the logs.


4 years ago

#5 @pento
4 years ago

  • Milestone changed from Awaiting Review to 4.4

@obenland, could you please have a look at this? I just ran into the same problem in Twenty Eleven.

#6 @obenland
4 years ago

  • Owner set to obenland
  • Status changed from new to accepted

#7 @obenland
4 years ago

  • Keywords dev-feedback removed

Hi @Nikschavan, thanks for the ticket and the patch!

Some feedback:
The compat function would need an actual prefix and should be defined outside of twentythirteen_setup(). We should also do a positive check (if ( function_exists() ) rather than if ( ! function_exists() ).

@Nikschavan
4 years ago

Refreshed the patch

#8 @Nikschavan
4 years ago

Thank you @obenland for your feedback, I have refreshed the patch accordingly, I will probably need help in the doc for the new function that is added, though.

Also, As this needs to be updated in all the default themes from 2010-2014, should I create a new ticket for all the themes separately, combine all into one patch?

Last edited 4 years ago by Nikschavan (previous) (diff)

@Nikschavan
4 years ago

I noticed this was adding title two tiles on WordPress 4.3.1, because add_filter( 'wp_title', 'twentythirteen_wp_title', 10, 2 ); was also being fired along with add_theme_support( 'title-tag' ); add_filter( 'wp_title', 'twentythirteen_wp_title', 10, 2 ); to compat check, will only be fired if _wp_render_title_tag does not exists.

#9 @Nikschavan
4 years ago

In WordPress 4.3.1

Before -

http://goo.gl/9U4UoY

After -

http://goo.gl/bJl0It

The patch is tested on WP 4.0.8, 4.3.1 and trunk

This ticket was mentioned in Slack in #core-themes by obenland. View the logs.


4 years ago

#11 follow-up: @lancewillett
4 years ago

I don't think we should patch Twenty Ten or Eleven because it'd be difficult to avoid breaking all the sites running those themes. I think it's OK to focus on newer themes.

#12 @obenland
4 years ago

@Nikschavan 34456.4.diff looks good. Do you want to update it for 2012 and 2014 as well?

#13 @Nikschavan
4 years ago

Yes @obenland, I will update it today.

@Nikschavan
4 years ago

#14 @Nikschavan
4 years ago

34456.5.diff Adds the changes made in 34456.4.diff to themes 2012 and 2014 as well.

#15 in reply to: ↑ 11 @obenland
4 years ago

Replying to lancewillett:

I don't think we should patch Twenty Ten or Eleven because it'd be difficult to avoid breaking all the sites running those themes. I think it's OK to focus on newer themes.

We're essentially breaking back compat for all other themes too though, the 'wp_title' filter won't be run anymore.

@obenland
4 years ago

#16 @obenland
4 years ago

  • Keywords needs-testing removed

After giving it a bit more thought, I don't think we should update default themes at all. The reason being that we would break back compat for every site that has a filter hooked into wp_title, which is obviously not a good experience.

I added a patch that would make the notice less obtrusive and also stop hijacking the title on sites that have WP_DEBUG enabled in production, which I think could be a good compromise. If the deprecated_function_trigger_error filter had the function as a second argument we could even disable displaying the notice at all in these themes, which in this case might make sense because we know they will never get updated.

Thoughts?

#17 follow-up: @ocean90
4 years ago

I'm think it's okay to not update old default themes, but 34456.diff is meh. The function declaration should be outside of wp_title() otherwise it will lead to a "cannot redeclare" fatal error if wp_title() gets called twice.

Wasn't there a suggestion to make wp_title() a wrapper for wp_get_document_title() somewhere?


If the deprecated_function_trigger_error filter had the function as a second argument we could even disable displaying the notice at all in these themes

Related: #34183

#18 follow-up: @Nikschavan
4 years ago

I agree with @obenland on not updating the default themes concerning the backwards compatibility, Although I am not too sure if I like the deprecated notice being displayed on the front end to all the visitors of the site.

Maybe only show the notice to logged in/ admin users? or add the notice in error_log()?

Either way, as we are planning to not to patch the themes, user who has this theme installed will have only two options to get rid of this warning either to change the theme or downgrade WordPress both the options are not good.

#19 in reply to: ↑ 17 @obenland
4 years ago

Replying to ocean90:

The function declaration should be outside of wp_title() otherwise it will lead to a "cannot redeclare" fatal error if wp_title() gets called twice.

Good point, I shall update the patch.

Wasn't there a suggestion to make wp_title() a wrapper for wp_get_document_title() somewhere?

Maybe on the make/core post? I'm not sure it would help with maintaining back compat for the filters in there. We could maintain it for wp_title but probably not for wp_title_parts unless I'm missing something.

#20 in reply to: ↑ 18 @obenland
4 years ago

Replying to Nikschavan:

I am not too sure if I like the deprecated notice being displayed on the front end to all the visitors of the site.
user who has this theme installed will have only two options to get rid of this warning either to change the theme or downgrade WordPress both the options are not good.

The notice is only shown on sites that have WP_DEBUG enabled in production.

#21 follow-up: @DrewAPicture
4 years ago

First thoughts:

  • For some reason I was thinking nested functions would be treated as closures in 5.2, which doesn't actually seem to be true. But @ocean90 raises a good point about reuse of wp_title().
  • I think because of the deprecated notice and because we're talking about default themes, we need to update the defaults back at least a year, so Twenty Fourteen and Fifteen
Last edited 4 years ago by DrewAPicture (previous) (diff)

#22 in reply to: ↑ 21 ; follow-ups: @obenland
4 years ago

Replying to DrewAPicture:

  • I think because of the deprecated notice and because we're talking about default themes, we need to update the defaults back at least a year, so Twenty Fourteen and Fifteen

Twenty Fifteen already uses the new way. Again, if we updated Twenty Fourteen we would break backwards compatibility with everything that is hooked into wp_title_parts and wp_title on sites that use the theme.

#23 in reply to: ↑ 22 @ocean90
4 years ago

Replying to obenland:

everything that is hooked into wp_title_parts and wp_title on sites that use the theme.

We could check via has_filter() if something is hooked into one of the filters. If not, use the new title.

@obenland
4 years ago

#24 @obenland
4 years ago

I'm not sure if it should be a private function like _deprecated_function()?

#25 in reply to: ↑ 22 ; follow-up: @ocean90
4 years ago

Replying to obenland:

Twenty Fifteen already uses the new way.

Actually that's a back compat issue, because the new way was previoulsy _wp_render_title_tag()->wp_title() and thus the wp_title and wp_title_parts filters were available.

#26 in reply to: ↑ 25 @obenland
4 years ago

Replying to ocean90:

Actually that's a back compat issue, because the new way was previoulsy _wp_render_title_tag()->wp_title() and thus the wp_title and wp_title_parts filters were available.

The impact on backwards compatibility here should be limited. Chances that wp_title is being filtered for anything other than what wp_get_document_title() already accounts for are even more limited, especially for newer themes that would support title-tags. I also assume that the vast majority of themes has implemented theme support the way it was recommended on make/core, which would make this pretty much a non-concern.

In retrospect we probably should have kept theme support for title-tags visible in current_theme_supports(), it would have allowed plugins to account for themes that declare it, even though they still might temporarily call wp_title(). While it's not as ideal as I'd like it to be, I don't think it's going to pose a problem.

This ticket was mentioned in Slack in #core by obenland. View the logs.


4 years ago

#28 @obenland
4 years ago

  • Milestone 4.4 deleted
  • Resolution set to invalid
  • Status changed from accepted to closed

wp_title() was un-deprecated in [35624].

Note: See TracTickets for help on using tickets.