WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#21233 closed defect (bug) (fixed)

Twenty Twelve: Make title truly pluggable

Reported by: obenland Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description

I had a discussion with @mamaduka over at the Github repo about this topic (which is gone :( ) and it seems to be a controversial one.

WPTRT guidelines state:

wp_title(): Themes are required to modify output via filter ('wp_title')

I'd like to see Twenty Twelve set a good example for Theme developers here. I have to flag (almost) every new Theme I review for that.

Attachments (4)

21233.diff (1.3 KB) - added by obenland 2 years ago.
21223.2.diff (1.6 KB) - added by obenland 2 years ago.
Beef up the wp_title callback
21233.2.diff (1.8 KB) - added by obenland 2 years ago.
Beef up the wp_title callback - and don't mess up ticket numbers
21233.3.diff (452 bytes) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (17)

obenland2 years ago

comment:1 ocean902 years ago

  • Milestone changed from Awaiting Review to 3.5

comment:2 sirzooro2 years ago

  • Cc sirzooro added

Consider resolving this as part of these related tickets: #12370, #18548.

comment:3 obenland2 years ago

Let's not.

While #12370 is somewhat related, IMO #18548 is not really. In the iterative spirit from last night's dev chat, this ticket here would be more a "lets make the best from what we have" approach.

If any outcome from #18548 has repercussions on Twenty Twelve, let's deal with it at that time.

comment:4 lancewillett2 years ago

  • Cc lancewillett added

comment:5 follow-up: lancewillett2 years ago

Agree 100% with the filter, though I'd like to add more to it along the lines of what Twenty Eleven does right now (which should also be moved to a filter now).

http://core.trac.wordpress.org/browser/trunk/wp-content/themes/twentyeleven/header.php#L27

@obenland Want to update your patch with that extra output?

obenland2 years ago

Beef up the wp_title callback

obenland2 years ago

Beef up the wp_title callback - and don't mess up ticket numbers

comment:6 in reply to: ↑ 5 ; follow-up: obenland2 years ago

Replying to lancewillett:

Agree 100% with the filter, though I'd like to add more to it along the lines of what Twenty Eleven does right now (which should also be moved to a filter now).

Should I create a new ticket for that? Or add a patch in #12370?


@obenland Want to update your patch with that extra output?

Sure thing, I hope you like it!

comment:7 in reply to: ↑ 6 lancewillett2 years ago

  • Keywords dev-feedback removed

Replying to obenland:

@obenland Want to update your patch with that extra output?

Sure thing, I hope you like it!

Patch is 99% there, thanks for that. I'm going to update the comments a tiny bit and remove an extra trailing whitespace near line 153.

comment:8 lancewillett2 years ago

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

In [21276]:

Twenty Twelve: make title truly pluggable by moving output from header.php to a function. Props obenland, fixes #21233.

SergeyBiryukov2 years ago

comment:9 SergeyBiryukov2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Since [21276], site title is displayed twice in RSS feed:

<title>WordPress TrunkWordPress Trunk</title>

In the feed files, wp_title_rss() is already prepended with site title:
http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/feed-rss2.php#L24

21233.3.diff fixes the duplication.

comment:10 follow-up: obenland2 years ago

Great catch, Sergey! The patch would be a quick fix, but wouldn't it make more sense to remove bloginfo_rss('name'); from the feed title?

We could add a filter to 'wp_title_rss' or 'get_wp_title_rss' instead and check if the title is empty, before adding the blog name.
Clearly not part of this ticket, but way more sustainable than coding around it, IMO.

What do you think?

comment:11 in reply to: ↑ 10 ; follow-up: SergeyBiryukov2 years ago

Replying to obenland:

Great catch, Sergey! The patch would be a quick fix, but wouldn't it make more sense to remove bloginfo_rss('name'); from the feed title?

Sounds good, but I'm not sure about possible back compat repercussions for themes and plugins which already take into account that site title is hardcoded.

We could add a filter to 'wp_title_rss' or 'get_wp_title_rss' instead and check if the title is empty, before adding the blog name.

Adding the blog name to empty titles only would mean that it will no longer be displayed for category feeds. I guess we should add it if it's not already present (while keeping the whole title filterable).

BTW, category and tag feed titles are also broken in current trunk:

<title>WordPress Trunk &#187; helloWordPress Trunk</title>

Removing the prepended site title wouldn't fix the order, so 21233.3.diff might still be needed, unless we want to reimplement some of wp_title() or try to filter out the separator.

Clearly not part of this ticket, but way more sustainable than coding around it, IMO.

I've created #21366 for removing the hardcoded site title.

comment:12 in reply to: ↑ 11 obenland2 years ago

remove bloginfo_rss('name'); from the feed title

Sounds good, but I'm not sure about possible back compat repercussions for themes and plugins which already take into account that site title is hardcoded.

Yes, but this is a catch 22: It's either back-compat problems or prepending the site title with a really late filter, which makes removing it pretty much obsolete.


Adding the blog name to empty titles only would mean that it will no longer be displayed for category feeds. I guess we should add it if it's not already present (while keeping the whole title filterable).

I agree. I like your patch in #21366.


Removing the prepended site title wouldn't fix the order, so 21233.3.diff might still be needed, unless we want to reimplement some of wp_title() or try to filter out the separator.

Right. I'd be fine with the patch, leaving the order as is.

comment:13 lancewillett2 years ago

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

In [21327]:

Twenty Twelve: remove duplicate RSS feed title from wp_title output, props SergeyBiryukov. Closes #21233.

Note: See TracTickets for help on using tickets.