Make WordPress Core

Opened 11 years ago

Closed 9 years ago

#22271 closed enhancement (fixed)

get_post_class() does not always apply filter to output

Reported by: f-j-kaiser's profile F J Kaiser Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.4
Component: Posts, Post Types Keywords: has-patch
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 (6)

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

Download all attachments as: .zip

Change History (43)

@F J Kaiser
11 years ago

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

#1 @toscho
11 years ago

  • Cc info@… added

#2 @F J Kaiser
11 years 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)

#3 @SergeyBiryukov
11 years ago

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

#4 @F J Kaiser
11 years 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')).

#5 @TJNowell
11 years ago

  • Cc tom@… added

#6 @Bueltge
11 years ago

  • Cc frank@… added

#7 @mbijon
11 years ago

  • Cc mike@… added

#8 @MikeSchinkel
11 years ago

  • Cc mike@… added

Seems like a no-brainer...

Last edited 11 years ago by MikeSchinkel (previous) (diff)

#9 @scribu
11 years 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.

#10 follow-up: @scribu
11 years 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.

#11 in reply to: ↑ 10 ; follow-ups: @MikeSchinkel
11 years 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.

#12 in reply to: ↑ 11 @scribu
11 years 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.

#13 in reply to: ↑ 11 @MikeSchinkel
11 years ago

Replying to MikeSchinkel:

Replying to scribu:

just because it's convenient for you

Trolling? Back at ya.

#14 @JustinSainton
11 years ago

  • Keywords dev-feedback added

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

#15 follow-ups: @scribu
11 years 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 11 years ago by scribu (previous) (diff)

#16 @scribu
11 years 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 11 years ago by scribu (previous) (diff)

#17 in reply to: ↑ 15 @MikeSchinkel
11 years 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 11 years ago by scribu (previous) (diff)

#18 in reply to: ↑ 15 ; follow-up: @scribu
11 years 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.

#19 in reply to: ↑ 18 @MikeSchinkel
11 years 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.

#20 @Rarst
11 years 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.

#21 follow-up: @nacin
11 years 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.

#22 @nacin
11 years ago

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

#23 in reply to: ↑ 21 ; follow-up: @scribu
11 years 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.

#24 in reply to: ↑ 23 @nacin
11 years 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.

#25 @F J Kaiser
11 years 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 11 years ago by F J Kaiser (previous) (diff)

#26 @scribu
11 years ago

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

That seems reasonable.

#27 follow-up: @Rarst
11 years 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 11 years ago by Rarst (next)

#28 @scribu
11 years 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.

@scribu
11 years ago

Always returns the original $class

#29 @scribu
11 years ago

  • Keywords has-patch added; needs-patch removed

#30 in reply to: ↑ 27 @F J Kaiser
11 years 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 Kaiser
11 years ago

Returning just the input classes

@F J Kaiser
11 years ago

Returning input classes filtered & escaped

@F J Kaiser
11 years ago

New filter as Nacin suggested - only two args

#31 @F J Kaiser
10 years 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.

#32 @nacin
10 years ago

  • Component changed from Template to Posts, Post Types
  • Focuses template added

#33 @obenland
10 years ago

  • Keywords needs-refresh added; dev-feedback removed

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
10 years ago

22271_2a.patch with codex

#34 follow-up: @Bueltge
10 years ago

  • Keywords needs-refresh removed

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.

#35 in reply to: ↑ 34 @F J Kaiser
9 years ago

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

#36 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.2

#37 @SergeyBiryukov
9 years ago

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

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.

Note: See TracTickets for help on using tickets.