WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 3 months ago

#22271 new enhancement

get_post_class() does not always apply filter to output

Reported by: F J Kaiser Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.4
Component: Posts, Post Types Keywords: has-patch dev-feedback
Focuses: template Cc:

Description

Currently the post class function/template tag simply returns an empty array for 404 requests. The reason is simple: The output gets generated based on the return values of get_post().

This avoids using it to style parts of the 404 template using the post_class filter.

Use case: In one theme I'm currently developing, I use the post_class filter to override the parent themes classes with some grid classes from a CSS framework. This avoids duplicating the templates and handle the positions, etc. of the template parts with a simple callback for the post_class filter. The attached patch makes sure that classes get added, instead of just returning an empty array if no $post->ID is present. It doesn't harm any other callback (or interfere with it).

The attached patch is tested in current 3.4.2 and 3.5 Beta. It is built against the v3.5 nightly build - revision 606806.

Sidenote: The somehow wired phpDocBlock comment was also "fixed".

Attachments (5)

22271_1.patch (1.3 KB) - added by F J Kaiser 18 months ago.
Adds the post_class filter for requests where no post ID is present
22271.diff (821 bytes) - added by scribu 18 months ago.
Always returns the original $class
22271_2a.patch (807 bytes) - added by F J Kaiser 18 months ago.
Returning just the input classes
22271_2b.patch (930 bytes) - added by F J Kaiser 18 months ago.
Returning input classes filtered & escaped
22271_2c.patch (934 bytes) - added by F J Kaiser 18 months ago.
New filter as Nacin suggested - only two args

Download all attachments as: .zip

Change History (37)

F J Kaiser18 months ago

Adds the post_class filter for requests where no post ID is present

comment:1 toscho18 months ago

  • Cc info@… added

comment:2 F J Kaiser18 months ago

  • Summary changed from Allow post class for 404 requests to get_post_class() does not always apply filter to output
  • Type changed from enhancement to defect (bug)

comment:3 SergeyBiryukov18 months ago

  • Keywords has-patch added
  • Version changed from trunk to 3.4

comment:4 F J Kaiser18 months ago

From a brief test, post_class also fails on the index page, if there's no loop (so get_post() can't trigger wp_cache_get($post_id, 'posts')).

comment:5 TJNowell18 months ago

  • Cc tom@… added

comment:6 Bueltge18 months ago

  • Cc frank@… added

comment:7 mbijon18 months ago

  • Cc mike@… added

comment:8 MikeSchinkel18 months ago

  • Cc mike@… added

Seems like a no-brainer...

Last edited 18 months ago by MikeSchinkel (previous) (diff)

comment:9 scribu18 months ago

  • Keywords dev-feedback removed

Why would you expect a function called get_post_class() to work when no post is present?

It doesn't harm any other callback (or interfere with it).

Yes, it does: filters that don't expect $post to be null will break.

comment:10 follow-up: scribu18 months ago

It seems to me that you're trying to transform get_post_class() into something that dilutes it's purpose, just because it's convenient for you, instead of fixing your theme to account for the is_404() case.

comment:11 in reply to: ↑ 10 ; follow-ups: MikeSchinkel18 months ago

Replying to scribu:

It seems to me that you're trying to transform get_post_class() into something that dilutes it's purpose, just because it's convenient for you, instead of fixing your theme to account for the is_404() case.

Making sure it always runs the filter is "diluting it's purpose?" Sounds like avoidance of change for avoidance of change sake.

comment:12 in reply to: ↑ 11 scribu18 months ago

Replying to MikeSchinkel:

Making sure it always runs the filter is "diluting it's purpose?" Sounds like avoidance of change for avoidance of change sake.

Sounds like trolling.

comment:13 in reply to: ↑ 11 MikeSchinkel18 months ago

Replying to MikeSchinkel:

Replying to scribu:

just because it's convenient for you

Trolling? Back at ya.

comment:14 JustinSainton18 months ago

  • Keywords dev-feedback added

Let's all play nice and get some core dev feedback.

comment:15 follow-ups: scribu18 months ago

I call it trolling because:

  • "You're avoiding change for the sake of avoiding change" is something you can throw around whenever you disagree with a wontfix, regardless of what argument was made.
  • It detracts from the actual discussion, steering it into meta discussion about governance.
Last edited 18 months ago by scribu (previous) (diff)

comment:16 scribu18 months ago

  • Keywords close added; dev-feedback removed

Anyway, back to the topic at hand:

Example breakage:

function my_old_callback( $classes, $class, $post ) {
  if ( 'foo' == $post->post_type )
    $classes[] = 'bar';

  return $classes;
}
add_filter( 'post_class', 'my_old_callback', 10, 3 );

With the patch applied, my_old_callback will break when $post is null and would have to be amended, like so:

function my_old_callback( $classes, $class, $post ) {
  if ( $post && 'foo' == $post->post_type )
    $classes[] = 'foo2';

  return $classes;
}

This is clearly a change that would break backwards compatibility.

Last edited 18 months ago by scribu (previous) (diff)

comment:17 in reply to: ↑ 15 MikeSchinkel18 months ago

Replying to scribu:

I call it trolling because:

  • "You're avoiding change for the sake of avoiding change" is something you can throw around whenever you disagree with a wontfix, regardless of what argument was made.
  • It detracts from the actual discussion, steering it into meta discussion about governance.

And I replied as I did because I was reacting to how frequently you reply in a manner that is disrespectful to others in the community who are trying to contribute. Had you simply held your comment to be "Yes, it does: filters that don't expect $post to be null will break" and not added the disrespectful comment I wouldn't have replied as I did. But yes, this is a meta/governance issue so that's all I'll say on this ticket.

Last edited 18 months ago by scribu (previous) (diff)

comment:18 in reply to: ↑ 15 ; follow-up: scribu18 months ago

I apologize if my tone came off as disrespectful, but I did have a point. I'm guilty of it myself: trying to push a change into Core that would be helpful for the narrow problem I'm trying to solve at the time, not realizing that it's actually a bad idea in the general case.

comment:19 in reply to: ↑ 18 MikeSchinkel18 months ago

Replying to scribu:

I apologize if my tone came off as disrespectful, but I did have a point.

Thank you for apology. After posting those comments I regretted it.

comment:20 Rarst18 months ago

There is a conflict of two expectations here:

  1. Function that applies filter to its return will do so in all cases.

This is generic expectation. I expect (possibly mistakenly, but it makes sense to me) WP to behave like this and would be (am) surprised when it's not the case. Note that "why would it apply filter in some specific case" defeats this expectation in generic form. It is "filter return", not "filter return sometimes".

  1. post_class filter will always pass valid post ID.

This is specific expectation. It is formed by how code works historically and in my perception was not intentionally engineered. It is not a goal of this filter to only pass valid ID. ID is just meta information there without explicit guarantee of validity. There are a lot of functions that verify if they have been passed valid input (including get_post_class() itself) instead of arguing that they should always receive only valid input.

So what this ticket boils down - which of the two expectations is more important to uphold.

comment:21 follow-up: nacin18 months ago

WordPress often applies a filter to all return values, but sometimes will not do so in an error, or will use an alternate filter name for unexpected or sudden returns. This seems like a good situation for there to be a unique filter.

Those applying a post class will be doing so — in nearly all cases — with the expectation that $post is indeed an object. This seems like a very reasonable expectation, and practically, things could break if we try to apply the same post_class filter in this case.

comment:22 nacin18 months ago

  • Keywords close removed
  • Type changed from defect (bug) to enhancement

comment:23 in reply to: ↑ 21 ; follow-up: scribu18 months ago

  • Keywords dev-feedback added

It seemed like you're agreeing that applying the filter to a null value is a bad idea, but then you removed the 'close' tag, so it's still up in the air.

comment:24 in reply to: ↑ 23 nacin18 months ago

  • Keywords needs-patch added; has-patch dev-feedback removed

Replying to scribu:

It seemed like you're agreeing that applying the filter to a null value is a bad idea, but then you removed the 'close' tag, so it's still up in the air.

I suggested a new filter.

comment:25 F J Kaiser18 months ago

To summon up the ideas we've so far:

  • Apply filter in all cases (breaks with Notice when $post object is expected and debug turned on)
  • Apply special-ops filter (needs new patch)
  • Leave current state (close ticket)

Other possibilities how to resolve this:

  • Return just the classes the user added as argument to post_class( 'example-class' );. This is a minimum "solution". It helps working around those cases where the index.php triggers as fallback for non existing 404.php templates and no check for have_posts() exists (but just a HTML wrapper tag that has post_class(); attached). (needs new patch)
  • Leave everything as it is and introduce a new template_part_class(); (or however this could be named) template tag that should be used on wrappers. Then just add a _doing_it_wrong(); if post_class(); is ! in_the_loop(); to deny further "wrong" usage. (needs new patch + new ticket)
Last edited 18 months ago by F J Kaiser (previous) (diff)

comment:26 scribu18 months ago

Return just the classes the user added as argument to post_class( 'example-class' );

That seems reasonable.

comment:27 follow-up: Rarst18 months ago

Some things to note:

  • get_post_class() does not pass post object, only ID. Any filter will have to retrieve post object anyway... and as for me it's filter's problem if it doesn't check that if that came up with something.
  • there are other filters that will happily pass non-valid ID, for example get_the_title. So case can be made that specific get_post_title filter historically only passes valid ID, but there is no general case that filter passing ID as additional argument must ensure that it is valid one.

PS introducing new filter doesn't make sense to me because I don't see strong case to not just apply existing to all cases and be done.

Version 0, edited 18 months ago by Rarst (next)

comment:28 scribu18 months ago

There is no strong argument for always applying the filter in this case, but there is an argument for keeping it the way it is, namely back-compat:

get_post_class() does not pass post object, only ID. Any filter will have to retrieve post object anyway...

That's fair, but how do they retrieve the object? By calling get_post(), which will return false.

and as for me it's filter's problem if it doesn't check that if that came up with something.

Yes, it would be the callback's problem, if we always applied the filter from the beginning.

scribu18 months ago

Always returns the original $class

comment:29 scribu18 months ago

  • Keywords has-patch added; needs-patch removed

comment:30 in reply to: ↑ 27 F J Kaiser18 months ago

Replying to Rarst:

  • get_post_class() does not pass post object, only ID. Any filter will have to retrieve post object anyway... and as for me it's filter's problem if it doesn't check that if that came up with something.
  • there are other filters that will happily pass non-valid ID, for example get_the_title. So case can be made that specific get_post_class filter historically only passes valid ID, but there is no general case that filter passing ID as additional argument must ensure that it is valid one.

I took a look at other functions like the_content();, the_guid(), etc. There're dozens that behave like this. This kept me thinking about it for a while.

  • get_post_class() is a public function. It neither has leading _/underscore to tell that it's not meant for public usage, nor a line stating this in the phpDocBlock.
  • When we then take a look at the input arguments, then both $classes as well as $post_id are optional.

So the default behavior of this function is to simply fail, returning the empty array(), when publicly used and no global $post object is present that can be used as fallback inside get_post(). And that is strange.

Replying to scribu:

Yes, it would be the callback's problem, if we always applied the filter from the beginning.

I don't see anything wrong with throwing a Notice or a Warning.

"NOTICE messages will warn you about bad style."

quote from php.net

The error that happens if one doesn't check if an object is set, will only be visible with WP_DEBUG (or PHP error env) turned on. And that shouldn't happen in production.

Based on all heard ideas and arguments so far, my personal preference would be to do the merge as early it is in scribus patch (no need to merge with the final array - speeds things up slightly), but also move the filter in for early returns.

F J Kaiser18 months ago

Returning just the input classes

F J Kaiser18 months ago

Returning input classes filtered & escaped

F J Kaiser18 months ago

New filter as Nacin suggested - only two args

comment:31 F J Kaiser3 months ago

  • Keywords dev-feedback added

Patches for all scenarios are in place - since since more than 2 years. We need some feedback here. Or a close tag.

comment:32 nacin3 months ago

  • Component changed from Template to Posts, Post Types
  • Focuses template added
Note: See TracTickets for help on using tickets.