WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#31078 closed enhancement (fixed)

Continue the better option for <title> tags

Reported by: obenland Owned by: obenland
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.1
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

Step 1 was done in #18548, setting up the infrastructure and encouraging theme authors to use the new theme-support system.

Step 2, which this ticket is for, will be replacing wp_title() in the _wp_render_title_tag() callback with a more meaningful output, and deprecating wp_title().

Step 3 would be a good case for a feature plugin, creating a UI to allow users to specify how their titles are rendered.

Attachments (11)

31078.diff (12.5 KB) - added by obenland 5 years ago.
31078.2.diff (13.5 KB) - added by obenland 4 years ago.
31078.3.diff (14.3 KB) - added by obenland 4 years ago.
31078.4.diff (14.3 KB) - added by obenland 4 years ago.
31078.5.diff (14.6 KB) - added by obenland 4 years ago.
31078.6.diff (19.7 KB) - added by obenland 4 years ago.
Adds some unit tests
31078.7.diff (21.6 KB) - added by obenland 4 years ago.
Finalized unit tests.
31078.8.diff (31.7 KB) - added by obenland 4 years ago.
wp_title_rss and documentation and usage updates.
31078.9.diff (31.8 KB) - added by obenland 4 years ago.
31078.10.diff (6.8 KB) - added by obenland 4 years ago.
31078.11.patch (715 bytes) - added by Fab1en 4 years ago.
Display deprecation notice outsite <title> tag

Download all attachments as: .zip

Change History (53)

@obenland
5 years ago

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


5 years ago

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


5 years ago

#3 @Fab1en
5 years ago

Some ideas for Step 3 :

creating a UI to allow users to specify how their titles are rendered.

The Wordpress SEO by Yoast plugin has a great user interface to define site title formatting. This plugin is generally part of all my WordPress installs only for this feature.

You can format titles independently for each "page type" : home, articles, pages, medias, taxonomies, archives, ... There are some keywords (like %%title%% for the post title and %%sitename%% for the site title). You can even select the right separator (short dash, long dash, pipe, ...) !

Would this subject deserve a new ticket ?

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


5 years ago

#5 @obenland
5 years ago

  • Milestone changed from 4.2 to Future Release

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


4 years ago

@obenland
4 years ago

#7 @joostdevalk
4 years ago

Ok I love this, I think it can go in as is in its latest iteration.

The only thing I'd change is the default separator, making it - instead of |.

#8 @SergeyBiryukov
4 years ago

Not sure if array_reverse() is needed for is_rtl(). wp_title() didn't do that, nor did any of the default themes. Looks good to me otherwise.

#9 @obenland
4 years ago

  • Milestone changed from Future Release to 4.3
  • Owner set to obenland
  • Status changed from new to accepted

#10 @obenland
4 years ago

  • Milestone changed from 4.3 to Future Release

Upon further consideration, this is not ready for 4.3 yet.
We'd also need to remove checks from current_theme_supports() functions and it hasn't been tested enough yet.

#11 @joostdevalk
4 years ago

Can we get the pre filter in early perhaps? :)

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


4 years ago

#13 @johnbillion
4 years ago

  • Keywords has-patch needs-testing added

#14 @obenland
4 years ago

  • Milestone changed from Future Release to 4.4

@obenland
4 years ago

#15 @obenland
4 years ago

  • Keywords dev-feedback added

Refreshed patch that updates all version mentions to 4.4.0, improves some filter documentation, and removes theme support check hacks.

This is pretty much ready to go and should probably go in early in the cycle. We should just be on the same page about whether to deprecate wp_title() or not. Thoughts?

#16 @wonderboymusic
4 years ago

@helen @nacin - can we get some big guns feedback on this when you get a spare moment?

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


4 years ago

#18 @obenland
4 years ago

@helen @nacin: I propose deprecating wp_title() to encourage theme authors to add title-tag theme support, to go in-line with the WPTRT requirement of not using wp_title() anymore, and to make it easier for plugins to filter the title output going forward. I'd love some feedback on it, when you get a chance.

#19 follow-up: @nacin
4 years ago

Solid work here. Thoughts:

  • Not against deprecating wp_title().
  • Should $title inside <title> be esc_html()? I honestly can't remember.
  • It's necessary to keep the crazy $wp_locale code inside the is_month() conditional. Translating a php.net/date string is valid when dealing with numbers, but it falls apart when you try to use abbreviations, or in this case, a full month name (F). Translating F will not end up with a translated month name. You'll end up with F, which will then only turn into a translated month name when PHP is operating under a different locale. Maybe I'm missing something.
  • I like that this patch requires spaces on each side of the separator, and keeps the spaces out of the title_tag_separator filter.
  • I agree with @SergeyBiryukov that is_rtl() shouldn't need an array_reverse().
  • Where did the wp_title filter go? Is there any reason for it to vanish? Switching to add_theme_support() is the choice of the theme, but wp_title is going to be used in plugins. (Wouldn't be against deprecating the filter.)
  • I like the associative array $title used to build the parts. However, there are some confusing pieces.
  • I like that $title['site'] is used, but I don't like how it's overloaded with the site description, or how the site title is moved to $title['page']. This shift makes it harder for tweaks to happen. I would probably go with $title['site'] and $title['description'] (or $title['tagline'], which I think is clearer here, funny enough) and have $title['page'] empty here. (I don't love this either, more in a moment...)
  • $title['page'] to me says page number. But that is actually $title['number']. I would call these $parts['title'] and $parts['number'].
  • Thus: $parts['title'] (if not home page), $parts['site'], $parts['tagline'] (if home page), $parts['page'] (if paged).
  • I think this makes certain things easier, but also makes it a bit easier to screw it up in some other ways, like by trying to assign $parts['title'] but failing to realize it's set in $parts['site']. Open to ideas. I'm thinking $parts['title'] overrides $parts['site'] if present. As in, treat them as synonyms, but keep them separate so it's easier to figure out what you receive through the filter. On the other hand, it makes it harder to remove the site title from your <title>... Perhaps we stick with what's in the patch, but some kind of $parts['home'] = true indicator?

#20 in reply to: ↑ 19 ; follow-up: @obenland
4 years ago

  • Keywords dev-feedback removed

Replying to nacin:
Thanks so much for your feedback!

Translating F will not end up with a translated month name. You'll end up with F, which will then only turn into a translated month name when PHP is operating under a different locale.

Yes, we'd assume that PHP *is* operating under a different locale. There are a number of instances where we already make that assumption, namely in get_the_archive_title().

  • Where did the wp_title filter go? Is there any reason for it to vanish?

One of the main reasons for doing this API change in the first place was that the wp_title is unreliable for plugins because it doesn't necessarily allow for filtering the entire document title. I thought it'd make sense to let it die together with wp_title() and continue with a clean slate.

  • I like the associative array $title used to build the parts. However, there are some confusing pieces.

Your suggestions make sense, especially the revised key names. In my upcoming patch I ended up with
$parts['title'], $parts['site'] (if not home page), $parts['tagline'] (if home page), $parts['page'] (if paged).
That way 'title' is constant, which is probably what is most important in that context.

@obenland
4 years ago

@obenland
4 years ago

#21 in reply to: ↑ 20 ; follow-up: @nacin
4 years ago

Replying to obenland:

Yes, we'd assume that PHP *is* operating under a different locale. There are a number of instances where we already make that assumption, namely in get_the_archive_title().

I think this is a bad assumption and one we should not make, whether in get_the_archive_title(), here, or anywhere else. If I reviewed get_the_archive_title(), and I think I did, then I simply missed it there and in past its incarnations in Twenty Eleven et al.

date() isn't even locale-aware, according to the documentation: http://php.net/manual/en/function.date.php. Even if it were, I would venture to guess that the locale is almost never *installed*, let alone configured (WordPress doesn't use setlocale()).

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

Replying to nacin:

date() isn't even locale-aware, according to the documentation: http://php.net/manual/en/function.date.php. Even if it were, I would venture to guess that the locale is almost never *installed*, let alone configured (WordPress doesn't use setlocale()).

But mysql2date(), which is used by get_the_date(), returns by default (third argument) a translated date.

@obenland
4 years ago

Adds some unit tests

@obenland
4 years ago

Finalized unit tests.

#23 @ocean90
4 years ago

1) Tests_Feed_RSS2::test_rss
Unexpected deprecated notice for wp_title

/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:262
/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:68

2) Tests_Feed_RSS2::test_channel
Unexpected deprecated notice for wp_title

/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:262
/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:68

3) Tests_Feed_RSS2::test_items
Unexpected deprecated notice for wp_title

/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:262
/srv/www/wp-develop/svn/tests/phpunit/includes/testcase.php:68

The patches don't consider wp_title_rss() and get_wp_title_rss().

@obenland
4 years ago

wp_title_rss and documentation and usage updates.

@obenland
4 years ago

#24 @stevenkword
4 years ago

31078.9.diff looks good from a feeds perspective.

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

#25 @wonderboymusic
4 years ago

@obenland this is your journey here - preferably this would land this week

#26 @obenland
4 years ago

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

In 35294:

Themes: Improve document title output.

Introduces more flexibility in filtering all parts of the document title,the
separator, and a way to short-circuit title generation. Plugins can now also
check for theme support and reliably filter the entire output. See #18548.
Deprecates wp_title().

Fixes #31078.

#27 @DrewAPicture
4 years ago

In 35297:

Docs: Improve vague changelog entries for functions and hooks where arguments were deprecated in [35294].

See #31078. See #32246.

#28 @DrewAPicture
4 years ago

In 35298:

Docs: Update the hook doc for the pre_get_document_title filter to explain what value is filterable rather than why the value is filterable :-)

The hook was introduced in [35294].

See #31078. See #32246.

#29 @DrewAPicture
4 years ago

In 35300:

Docs: Fix indentation and improve usefulness of inline comments surrounding the if/elseif conditions for assigning document titles in wp_get_document_title().

See #31078. See #32246.

#30 @DrewAPicture
4 years ago

In 35301:

Docs: Adjust syntax in the hook doc summaries for the document_title_separator and document_title_parts filters, introduced in [35294].

See #31078. See #32246.

#31 @DrewAPicture
4 years ago

In 35303:

Docs: Add translator comments for two _deprecated_argument() calls added to get_wp_title_rss() and wp_title_rss() in [35294].

See #31078. See #32246.

#32 @boonebgorges
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The new title tests break if your test rig has a custom WP_TESTS_TITLE.

#33 @boonebgorges
4 years ago

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

In 35329:

Document title tests should expect dynamic blogname.

Fixes #31078.

@obenland
4 years ago

#34 @obenland
4 years ago

In 35334:

Tests: Use most specific function for document titles.

Adds tests specific to _wp_render_title_tag().

See #31078.

#35 @luciole135
4 years ago

Since this afternoon, on my local test site, in WP 4.4, it appears a message that wp_title () is deprecated on the page tab.
I have attached a screenshot. http://hpics.li/8cfb5b6
Is this normal?

#36 @ocean90
4 years ago

In 35388:

Use correct placeholders for translator comments added in [35303].

See #31078.

#37 follow-up: @celloexpressions
4 years ago

Twenty Ten through Fourteen still call wp_title(), and thus generate notices whenever debug is on now. That should probably be fixed; at least by conditionally calling it and adding the theme support when older versions of WP still need to be supported.

Also, when the notice is thrown, it's shown in the <title>, which seems pretty bad as less technically inclined users won't notice it but it could cause major SEO (and other) issues over time. While they shouldn't have debug on, I'd still be concerned about the implications of throwing deprecated notices here because people may have debug on due to various reasons (that they're likely unaware of).

#38 in reply to: ↑ 37 @Fab1en
4 years ago

Replying to celloexpressions:

Twenty Ten through Fourteen still call wp_title(), and thus generate notices whenever debug is on now. That should probably be fixed; at least by conditionally calling it and adding the theme support when older versions of WP still need to be supported.

Also, when the notice is thrown, it's shown in the <title>, which seems pretty bad as less technically inclined users won't notice it but it could cause major SEO (and other) issues over time. While they shouldn't have debug on, I'd still be concerned about the implications of throwing deprecated notices here because people may have debug on due to various reasons (that they're likely unaware of).

Good point @celloexpressions ! I agree that this could have bad effects, even for the more tech aware WordPress users. Checkout @wonderboymusic blog (referenced in his WP profile) to see it in action ;)

#39 @dd32
4 years ago

I'm questioning if we should really have a _deprecated_function() call for something which a significant number of existing themes call, and isn't necessarily broken or bad.
I'm not 100% against it, just feels strange.

#40 @Fab1en
4 years ago

What about calling the _deprecated_function() but outside the <title> tag so that it does not harm the page that much ?

@Fab1en
4 years ago

Display deprecation notice outsite <title> tag

#41 @Fab1en
4 years ago

31078.11.patch displays the deprecation notice in a <meta> tag instead of in the <title> tag. Another solution could be to but it inside html comments.
The issue with this patch is that the reported file and line number are incorrect.

#42 @netweb
4 years ago

Looks like r35624 didn't pick up the ticket mention here during commit:


In 35624:

Template: Un-deprecate wp_title().

Before it can be deprecated we should identify alternative usages and define
a path forward for them.

See [35294], #31078.

Note: See TracTickets for help on using tickets.