get_post_class() does not always apply filter to output

[F J Kaiser]

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".

[F J Kaiser]

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

[F J Kaiser]

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')).

[MikeSchinkel]

Seems like a no-brainer...

[scribu]

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.

[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.

[MikeSchinkel]

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.

[scribu]

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.

[MikeSchinkel]

Replying to MikeSchinkel:

Replying to scribu:

just because it's convenient for you

Trolling? Back at ya.

[JustinSainton]

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

[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.

[scribu]

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.

[MikeSchinkel]

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.

[scribu]

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.

[MikeSchinkel]

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.

[Rarst]

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".

2. 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.

[nacin]

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.

[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.

[nacin]

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.

[F J Kaiser]

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)

[scribu]

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

That seems reasonable.

[Rarst]

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_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.

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.

[scribu]

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.

[scribu]

Always returns the original $class

[F J Kaiser]

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 Kaiser]

Returning just the input classes

[F J Kaiser]

Returning input classes filtered & escaped

[F J Kaiser]

New filter as Nacin suggested - only two args

[F J Kaiser]

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

[obenland]

I think back compat trumps the expectation that a filter will be applied to all return values. As nacin pointed out, WordPress is pretty consistent about that inconsistency, and in the context of get_post_class() I'd consider the lack of post context at least "unexpected", if not an "error".

I like 22271_2a.patch​. Passing back the custom classes makes sense to me. F J Kaiser, could you give the patch another do over with ​coding standards in mind?

[Bueltge]

22271_2a.patch with codex

[Bueltge]

Attached another version, rewrite with coding standards, handmade. I haven't use the IDE possibility to leave done the other functions, lines in this file.

[F J Kaiser]

Replying to Bueltge:

Attached another version, rewrite with coding standards, handmade. I haven't use the IDE possibility to leave done the other functions, lines in this file.

Thanks

[SergeyBiryukov]

In 31408:

Always pass back the custom classes get_post_class() was called with, even if the post was not found.

props F J Kaiser, Bueltge.
fixes #22271.