WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 9 months ago

Last modified 9 months ago

#24011 closed task (blessed) (invalid)

Consider hiding post title for "Status" and "Aside" formats and autogenerating

Reported by: markjaquith Owned by: nacin
Milestone: Priority: high
Severity: blocker Version: 3.6
Component: Post Formats Keywords: needs-patch
Focuses: Cc:

Description (last modified by markjaquith)

The status and asides post formats shouldn't have a title. The only reason for having a title is to generate a slug. We should consider hiding the post title for these formats, and auto-generating a title based on the first X chars of the post, P2 style.

Attachments (18)

24011.diff (2.4 KB) - added by kovshenin 13 months ago.
24011.2.diff (3.5 KB) - added by kovshenin 13 months ago.
24011.3.diff (3.5 KB) - added by kovshenin 12 months ago.
24011.4.diff (3.5 KB) - added by kovshenin 12 months ago.
24011.5.diff (4.0 KB) - added by kovshenin 12 months ago.
24011.6.diff (538 bytes) - added by MZAWeb 12 months ago.
24011.7.diff (402 bytes) - added by kovshenin 12 months ago.
24011.8.diff (1.2 KB) - added by kovshenin 12 months ago.
24011.9.diff (683 bytes) - added by kovshenin 12 months ago.
24011.10.diff (2.0 KB) - added by aaroncampbell 11 months ago.
24011.11.diff (2.1 KB) - added by aaroncampbell 11 months ago.
24011.12.diff (2.4 KB) - added by aaroncampbell 11 months ago.
24011.13.diff (4.2 KB) - added by markjaquith 11 months ago.
24011.14.diff (4.0 KB) - added by aaroncampbell 11 months ago.
24011.15.diff (4.1 KB) - added by aaroncampbell 11 months ago.
24011.16.diff (4.9 KB) - added by aaroncampbell 11 months ago.
24011.17.diff (4.9 KB) - added by aaroncampbell 11 months ago.
24011.18.diff (3.4 KB) - added by DrewAPicture 9 months ago.
To revert [24043]

Download all attachments as: .zip

Change History (79)

comment:1 markjaquith13 months ago

  • Description modified (diff)
  • Summary changed from Consider hiding post title for "Status" format and autogenerating to Consider hiding post title for "Status" and "Aside" formats and autogenerating

comment:2 aniketpant13 months ago

  • Cc me@… added

Are saying that the title field should not be available when creating a new post?
Because as of now, both Twenty Thirteen and Sempress, do not show the title of a Status or a Aside post format.

kovshenin13 months ago

comment:3 follow-up: kovshenin13 months ago

  • Keywords has-patch added

In 24011.diff:

  • For empty titles in asides and status posts, generate a title from post content or format
  • Return an empty string for get_the_title for statuses and asides on the front-end

Not sure how many existing themes show post titles for asides and statuses, but if they do, this approach might seem "broken." What if the post title is hidden only if it's auto-generated?

comment:4 in reply to: ↑ 3 helen13 months ago

Replying to kovshenin:

What if the post title is hidden only if it's auto-generated?

Does that actually happen/is that actually done?

I think I've noted this in at least one of my Make/Core posts, but I would really prefer that the title still be editable on the screen somehow, probably through a toggle or some such. I also wonder if we can do some smart auto-generating for some other formats if a user leaves the title blank, whether or not the title field is hidden for that format.

kovshenin13 months ago

comment:5 kovshenin13 months ago

Replying to helen:

Does that actually happen/is that actually done?

Nope. I meant to propose for the post title to be hidden only when auto-generated. Sorry if it sounded otherwise :)

In 24011.2.diff:

  • Hook in earlier at wp_insert_post_data to avoid multiple queries
  • Re-generate the post title during an update if it was auto-generated
  • Hide the post title in get_the_title only if it was auto-generated

comment:6 follow-up: jrbeilke13 months ago

24011.2.diff looks ok when working with a single post, but on my 3.6 beta site I'm running into problems with batch editing posts:

Notice: Undefined index: post_format in /wp-includes/post-formats.php on line 843

Looks like $postarr doesn't have 'post_format' available for wp_insert_post_data

kovshenin12 months ago

comment:7 in reply to: ↑ 6 ; follow-up: kovshenin12 months ago

Replying to jrbeilke: Thanks! Fixed in .3.diff

comment:8 SergeyBiryukov12 months ago

In 24011.3.diff, there's no need for array_keys() in line 860. See #24089.

kovshenin12 months ago

comment:9 kovshenin12 months ago

Thanks Sergey, fixed in .4.diff

comment:10 in reply to: ↑ 7 jrbeilke12 months ago

Replying to kovshenin:

Replying to jrbeilke: Thanks! Fixed in .3.diff

No more errors on my beta site.

Should this also generate a title for aside/status posts when bulk editing?

Last edited 12 months ago by jrbeilke (previous) (diff)

comment:11 markjaquith12 months ago

The patch doesn't hide the title field for me.

kovshenin12 months ago

comment:12 kovshenin12 months ago

Refreshed in 24011.5.diff, additionally:

  • Hides the title field on the edit screen for asides and status posts
  • Does not hide auto-generated titles in RSS feeds
  • Works when bulk editing

comment:13 follow-up: markjaquith12 months ago

In 24043:

Hide the post title and auto-generate based on the post body, for the status and aside post formats.

props kovshenin. see #24011.

comment:14 follow-up: markmcwilliams12 months ago

I've only just spotted this, but WHY should status and aside posts not have a title? I, for example, quite like being able to show the title on the single post views. Is there going to be a way to disable this particular "addition" or not?

comment:15 in reply to: ↑ 14 ; follow-up: knutsp12 months ago

Replying to markmcwilliams:

I've only just spotted this, but WHY should status and aside posts not have a title? I, for example, quite like being able to show the title on the single post views.

They do have a title. Hiding an edit field for something does not mean it's not there. Hiding and utogenerating means you don't have to write it.

Whether titles are used on single post views is theme dependent.

comment:16 in reply to: ↑ 15 ; follow-up: markmcwilliams12 months ago

Replying to knutsp:

Replying to markmcwilliams:

I've only just spotted this, but WHY should status and aside posts not have a title? I, for example, quite like being able to show the title on the single post views.

They do have a title. Hiding an edit field for something does not mean it's not there. Hiding and utogenerating means you don't have to write it.

Whether titles are used on single post views is theme dependent.

What I'm getting at is I don't wish for my titles to be auto-generated!

comment:17 in reply to: ↑ 16 ; follow-up: knutsp12 months ago

Replying to markmcwilliams:

What I'm getting at is I don't wish for my titles to be auto-generated!

I think this handling of titles reflects the definition of what the formats aside and status is about. You don't write a title to a status post on services like Twitter and Facebook, for instance. If you want your own titles you select another post format, like standard.

comment:18 greenshady12 months ago

  • Cc justin@… added

I thought we had already previously decided that we'd allow users to at least have access to editing the title for all post formats as Helen mentioned above. Whether this is done via a toggle, screen option, or whatever, it should be easy to do.

Many of us who've been using asides for years are accustomed to being able to write custom titles. Even Matt does this at http://ma.tt/type/aside (check single posts). I won't even get started on the SEO folks who'll want to have total control over this.

I actually like the idea of an auto-generated title (for all posts) if the title field is left empty.

comment:19 in reply to: ↑ 17 ; follow-up: mrwweb12 months ago

  • Cc info@… added

Replying to knutsp:

I think this handling of titles reflects the definition of what the formats aside and status is about. You don't write a title to a status post on services like Twitter and Facebook, for instance. If you want your own titles you select another post format, like standard.

I find that compelling for statuses but not asides. People get these two stati confused all the times and leaving the title for asides might actually be a good way to help make that clarification.

Generally, being able to [easily] edit the title strikes me as a fairly reasonable request for those who want full editorial control, even if only displayed backend. And how is Quick Edit affected by this change? If it's not at all, that only makes hiding the title that much more confusing.

+1 @greenshady to auto-generated titles on anything without a title.

comment:20 in reply to: ↑ 19 SergeyBiryukov12 months ago

Replying to mrwweb:

I find that compelling for statuses but not asides. People get these two stati confused all the times and leaving the title for asides might actually be a good way to help make that clarification.

Makes sense to me.

comment:21 markjaquith12 months ago

I'm good with keeping restoring the title for aside, but keeping the autogen code for when it's blank, and the filters on the front end. Aside has a lot of history, and a lot of people might be used to custom titles.

Separately, we have another issue, in that the code that autofocuses the title should autofocus the content for post formats that hide the title.

comment:22 markjaquith12 months ago

In 24085:

Restore the title visibility for Asides (but keep autogeneration fallback).

see #24011.

comment:23 in reply to: ↑ 13 MZAWeb12 months ago

There's a regression introduced in 24043

In _post_formats_fix_empty_title():

$post_id = ( isset( $postarr['ID'] ) ) ? absint( $postarr['ID'] ) : 0;

if ( $post_id )
	$post_format = get_post_format( $post_id );

if ( isset( $postarr['post_format'] ) )
	$post_format = ( in_array( $postarr['post_format'], get_post_format_slugs() ) ) ? $postarr['post_format'] : '';

if ( ! in_array( $post_format, array( 'aside', 'status' ) ) )
	return $data;

When you wp_insert_post() without passing a post_format, neither $post_id nor $postarrpost_format? are set, so that $post_format in the last if throws a warning because it's undefined.

comment:24 MZAWeb12 months ago

  • Cc wordpress@… added

MZAWeb12 months ago

comment:25 MZAWeb12 months ago

24011.6.diff fixes the issue in comment:23

kovshenin12 months ago

comment:26 kovshenin12 months ago

24011.7.diff is a slightly different approach at the issue above.

comment:27 markjaquith12 months ago

In 24094:

Prevent a PHP Warning for $post_format.

props MZAWeb, kovshenin. see #24011.

comment:28 ryan12 months ago

[24043] caused a significant increase in traffic to the posts cache on wordpress.com. Commenting out the add_action()s in default-filters.php brought the cache traffic back to normal.

Last edited 12 months ago by ryan (previous) (diff)

comment:29 ryan12 months ago

Restored the _post_formats_fix_empty_title action. Traffic remained normal. So, _post_formats_title appears to be the culprit.

kovshenin12 months ago

comment:30 follow-up: kovshenin12 months ago

It looks like the function is too heavy and the_title filter is called a bazillion times in a request. Seems like get_post() and get_post_format() are causing cache hits. 24011.8.diff implements a little internal cache with a static array. Here are some measurements I took:

Cache hitsIncrease
No filter21740%
r24043239810.30%
24011.8.diff21940.91%

Feedback appreciated, and @ryan please let me know if this does the trick. Thanks!

comment:31 ryan12 months ago

In general, I like to avoid adding caches other than those managed by WP_Object_Cache. I haven't investigated yet, but this feels like something that needs to be fixed lower down.

comment:32 in reply to: ↑ 30 SergeyBiryukov12 months ago

I guess some kind of caching would make sense here, as we don't need to regenerate and verify the title each time the_title filter runs for the same post.

comment:33 ryan12 months ago

kovshenin, are you seeing local cache hits or remote? With both wordpress.com and trunk, I'm not seeing any extra remote cache hits with the action enabled. This is with several aside and status posts so _post_formats_generate_title() is being run. I see extra local cache hits with the filter enabled, but no extra remote cache hits. I'm not sure why we're seeing increased remote hits in aggregate on wordpress.com.

comment:34 nacin12 months ago

Also worth noting that those kinds of static cache keys don't work right when the blog is switched. You could add the blog ID into the cache key, but at that point, it becomes obvious you should use the object cache.

comment:35 follow-up: kovshenin12 months ago

@nacin, you're right, didn't think of that :(

@ryan, local. I haven't tested it with external cache, but I can try if it helps. I'm no cache guru but I assumed every cache get was sent to external cache, and now I see there's a $force argument there, at least in the memcached plugin on .org, not entirely sure how that's used in the wild.

comment:36 in reply to: ↑ 35 nacin12 months ago

Replying to kovshenin:

@ryan, local. I haven't tested it with external cache, but I can try if it helps. I'm no cache guru but I assumed every cache get was sent to external cache, and now I see there's a $force argument there, at least in the memcached plugin on .org, not entirely sure how that's used in the wild.

External cache hits only occur once. Then it gets stored locally for future retrieval on that page. Local cache hits are just fine. $force relatively new and is only there for the rare instance when you need to hit external cache again (like when you are using an external object cache for locking a process — we use it for cron locking). Most object cache backends likely don't yet even support $force; they go straight to local no matter what.

Local cache hits are fine. It's no different than a local array, just with the overhead of an extra few function calls.

kovshenin12 months ago

comment:37 kovshenin12 months ago

Some plugins and themes (including P2) seem to use the_title filter with apply_filters without a post context. This can cause missing argument warnings in _post_formats_title. 24011.9.diff addresses that.

comment:38 follow-up: johnjamesjacoby12 months ago

Me, from #24013:

I'd rather we not ever hide the Title field, and instead disable it and change the "Enter title here" text to say "Not used for this format" -- it will cut down on the jumpiness and unpredictability of hiding core fields that have been around for a decade, and isn't as jarring to existing users that need to learn the new UI.

comment:39 in reply to: ↑ 38 markmcwilliams12 months ago

Replying to johnjamesjacoby:

Me, from #24013:

I'd rather we not ever hide the Title field, and instead disable it and change the "Enter title here" text to say "Not used for this format" -- it will cut down on the jumpiness and unpredictability of hiding core fields that have been around for a decade, and isn't as jarring to existing users that need to learn the new UI.

It would certainly cut down on the "Where did the title field go?" questions that are bound to come in?

I'd still rather have *some way* of editing it, for freaks such as myself! o_O

comment:40 SergeyBiryukov12 months ago

In 24146:

Make $post_id argument optional for _post_formats_title(). props kovshenin. fixes #24233. see #24011.

comment:41 follow-up: mindctrl12 months ago

I'm in the club of people who assigns titles to aside and status posts.

Instead of hiding it or saying "Not used for this format", how about "Enter title here (optional)" on those formats that would get an auto-generated title if left blank?

comment:42 SergeyBiryukov11 months ago

#24339 was marked as a duplicate.

comment:43 in reply to: ↑ 41 alex-ye11 months ago

Replying to mindctrl:

I'm in the club of people who assigns titles to aside and status posts.

Instead of hiding it or saying "Not used for this format", how about "Enter title here (optional)" on those formats that would get an auto-generated title if left blank?

1+

The title input is very helpful, Sometimes I need to create a unique post title ever for 'Status' format.. the post title not only shown in the front-end it's also helpful in the back-end and custom queries.

In also showing Asides and Status title in the front-end is depending on the theme, I can't say that if theme show the Status title is wrong...

My opinion is:
1- Show the post title input with a opacity and "Enter title here (optional)" place-holder.

2- When the user publish the Aside/Status post with an empty title, leave it blank even in the database ( The database should only contain the user input not something that auto-generated ).

3- Hook to the get_the_title filter to show the feedback title ( title based on the first X chars of the post content ) only in the Admin/Feeds and cache the result.

I really don't like when WordPress make a strict rules about how I must design the theme or mange the website.. So keep it free as possible.

Version 0, edited 11 months ago by alex-ye (next)

comment:44 toscho11 months ago

  • Cc info@… added

aaroncampbell11 months ago

aaroncampbell11 months ago

aaroncampbell11 months ago

comment:45 aaroncampbell11 months ago

24011.12.diff sets the opacity for the title element to 50% for status and aside, and sets it back to 100% when focused. It also auto-focuses on the content for those types instead of the title.

markjaquith11 months ago

comment:46 markjaquith11 months ago

24011.13.diff changes the title prompt text to indicate that it is optional for status/aside (well, MORE optional than usual). Changes some variable names.

aaroncampbell11 months ago

comment:47 aaroncampbell11 months ago

  • Keywords commit added

I noticed one small issue with .13. When I edited an existing status or aside post the title wasn't faded until after I focused in and back out, or after changing the post format and changing it back. 24011.14.diff fixes that.

As far as I can tell, everything looks good now.

comment:48 follow-up: ocean9011 months ago

Having one filter for two different strings (Enter title here/Enter title here (optional)) seems weird. Perhaps a third boolean arg $optional.

comment:49 alex-ye11 months ago

I am wondering if there is an easy way to know if the Aside/Status title is auto-generated or custom ?!

comment:50 in reply to: ↑ 48 ryan11 months ago

Replying to ocean90:

Having one filter for two different strings (Enter title here/Enter title here (optional)) seems weird. Perhaps a third boolean arg $optional.

A third arg sounds good.

aaroncampbell11 months ago

aaroncampbell11 months ago

comment:51 follow-up: aaroncampbell11 months ago

In 24011.16.diff I just added a true/false as the third arg with a note that says "Third argument passed to filter denotes optional title field"

Let me know if you have any preferences beyond that.

comment:52 wonderboymusic11 months ago

People have been getting dark about boolean args - I personally have no problem with them, just beware

comment:53 in reply to: ↑ 51 ; follow-up: SergeyBiryukov11 months ago

Replying to aaroncampbell:

In 24011.16.diff I just added a true/false as the third arg with a note that says "Third argument passed to filter denotes optional title field"

Could we make that 'optional'/'required' for clarity (per our coding standards)?

aaroncampbell11 months ago

comment:54 in reply to: ↑ 53 aaroncampbell11 months ago

Replying to SergeyBiryukov:

Could we make that 'optional'/'required' for clarity (per our coding standards)?

Should have done that to begin with. Changed in 24011.17.diff

comment:55 wonderboymusic10 months ago

  • Milestone 3.6 deleted
  • Resolution set to invalid
  • Status changed from new to closed

comment:56 SergeyBiryukov10 months ago

  • Milestone set to 3.6
  • Resolution invalid deleted
  • Status changed from closed to reopened

This ticket falls under "Post Format Things to Keep in Core", per the post on make/core.

#24452 didn't touch [24043]. Looks like we still have a performance issue to deal with: comment:28.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

comment:57 markjaquith10 months ago

Let's add caching, but using WP core caching with a non-persistent cache group.

comment:58 markjaquith10 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Severity changed from normal to blocker

Bumping up the severity. Needs a patch for the above.

comment:59 kovshenin10 months ago

I spent a fair amount of time with this today and could not really find where it all goes wrong, at least in core. I added the following piece of code to the filter callback.

$group = 'posts';
$key = $wp_object_cache->key( $post_id, $group );
if ( ! isset( $wp_object_cache->cache[ $key ] ) ) {
	error_log( sprintf( 'key not set: %s on %s', $key, $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'] ) );
}

This checks whether the passed in post id is already in the local cache, and if it's not, it means that the following calls to get_post_format and get_post will fetch the remote cache. I haven't got a single entry while browsing trunk with a few plugins. Not a single entry.

I tried the same experiment on WordPress.com and I did get a few entries. I'm looking further into it now, but it seems like get_the_title in conjunction with get_post is slightly misbehaving.

DrewAPicture9 months ago

To revert [24043]

comment:60 nacin9 months ago

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

In 24693:

Revert title auto-generation for asides and statuses.

Reverts [24043] and related.

fixes #24011.

comment:61 nacin9 months ago

  • Milestone 3.6 deleted
  • Resolution changed from fixed to invalid
Note: See TracTickets for help on using tickets.