Make WordPress Core

Opened 13 years ago

Last modified 6 months ago

#18035 reopened enhancement

ignore_sticky_posts fails to remove sticky class

Reported by: mikkelbreum's profile mikkelbreum Owned by: johneckman's profile johneckman
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2
Component: Query Keywords: has-patch dev-feedback
Focuses: template Cc:

Description

When setting the query_posts parameter:

ignore_sticky_posts = 1

all sticky posts are returned as normal posts and placed accordingly in the flow. However the sticky posts keep their sticky class, which means that an additional filtering of post_class is necessary to avoid any css rules defined for the .sticky selector taking effect.

is this intended, or could it be considered an enhancement if it was patched?

Attachments (3)

18035.patch (607 bytes) - added by jakub.tyrcha 13 years ago.
18035.diff (548 bytes) - added by johneckman 9 years ago.
Updated
18035_3.diff (596 bytes) - added by ankit.gade@… 9 years ago.
Check if query is for secondary loop. then add sticky class.

Download all attachments as: .zip

Change History (34)

#1 @scribu
13 years ago

  • Keywords 2nd-opinion removed

As far as I can tell, the sticky class is the only thing that allows you to style a sticky post.

Therefore, posts that are displayed normally shouldn't have the sticky class.

#2 @dd32
13 years ago

Judging by the syntax of the get_post_class() function, it does look like it shouldn't apply in this case:

345	        // sticky for Sticky Posts
346	        if ( is_sticky($post->ID) && is_home() && !is_paged() )
347	                $classes[] = 'sticky';

#3 @jakub.tyrcha
13 years ago

  • Keywords has-patch added; needs-patch removed

is_sticky doesn't check for ignore_sticky_posts ;) patch attached

#4 @scribu
13 years ago

Good catch, but you can just write

&& !get_query_var('ignore_sticky_posts')

@jakub.tyrcha
13 years ago

#5 @jakub.tyrcha
13 years ago

Updated :)

#6 @scribu
13 years ago

When uploading a new version of a patch, don't overwrite the old one, so that context is preserved.

#8 @mikkelbreum
12 years ago

  • Cc mikkel@… added

Theres a patch provided here (18035), but it hasn't been implemented as of WP 3.3. Any reason?

#9 @SergeyBiryukov
12 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to Future Release

#10 @chriscct7
9 years ago

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

Needs a refresh

#11 @chriscct7
9 years ago

  • Keywords good-first-bug added

@johneckman
9 years ago

Updated

#12 @johneckman
9 years ago

  • Keywords has-patch added

#13 @DrewAPicture
9 years ago

  • Owner set to johneckman
  • Status changed from new to assigned

#14 @DrewAPicture
9 years ago

  • Component changed from General to Query
  • Keywords needs-refresh removed

#15 @SergeyBiryukov
9 years ago

  • Focuses template added
  • Milestone changed from Future Release to 4.1

#16 @SergeyBiryukov
9 years ago

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

In 30036:

Don't add 'sticky' class in get_post_class() if 'ignore_sticky_posts' query var is set.

props jakub.tyrcha, johneckman.
fixes #18035.

#17 follow-up: @nacin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't remember where (Twitter?) but I've seen a bug report related to the sticky class no longer appearing in 4.1.

I am trying to think how this could break things, but I bet it does, despite it looking harmless. Secondary loops, queries that deliberately splice sticky_posts and put it into post__in because the client stuck a few thousand posts (classic markjaquith rant), etc.

@ankit.gade@…
9 years ago

Check if query is for secondary loop. then add sticky class.

#18 in reply to: ↑ 17 @ankit.gade@…
9 years ago

Replying to nacin:

I don't remember where (Twitter?) but I've seen a bug report related to the sticky class no longer appearing in 4.1.

I am trying to think how this could break things, but I bet it does, despite it looking harmless. Secondary loops, queries that deliberately splice sticky_posts and put it into post__in because the client stuck a few thousand posts (classic markjaquith rant), etc.

We have to determine that if current query is main query or query for secondary loop. We can add class sticky in following couple of situations.

  1. If query is main query and ignore_sticky_posts parameter is not set in the query.
  1. If query is for secondary loop only.

Still, I am wondering when there is a check for is_home(), how could it break the things !

This ticket was mentioned in Slack in #core by nacin. View the logs.


9 years ago

#20 @ankit.gade@…
9 years ago

I think if we are dealing with secondary loop, then we can remove is_home() condition from that line. Otherwise on homepage secondary loop does not seem effective.

#21 follow-up: @helen
9 years ago

  • Keywords revert added

Even though we already rely on the state of the $wp_query global in the other parts of the conditional, looking at a query var that might not be from that particular loop seems extra wrong. I also don't think a post stops being marked as sticky based on the query that's being run. The global is_main_query() function is deprecated (see #23329) so the following patch is a no-go.

This definitely breaks things visually on the front for anybody who was previously styling sticky posts whether or not they were floated to the top in the query, which to me is a separate concern from an HTML/CSS class. I'm also sure there are people out there who use stickies to indicate some kind of importance without necessarily always chucking them to the front and rely on the class being there, no matter what that query var says.

Suggest revert and wontfix - not only do I think that we are mixing concerns here, but even if we weren't, there's no way this doesn't at least cause visual breakage in themes.

#22 in reply to: ↑ 21 ; follow-up: @SergeyBiryukov
9 years ago

Replying to helen:

The global is_main_query() function is deprecated (see #23329) so the following patch is a no-go.

is_main_query() only issues a notice when being called from pre_get_posts. It still has a valid use case otherwise ("is $wp_query the main query?"), see comment:3:ticket:23329, which is exactly the use case here.

I don't disagree with a wontfix, but checking is_main_query() seems like a correct thing to do here, so perhaps revert and fix in a future release would be a better way.

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#23 in reply to: ↑ 22 @helen
9 years ago

Replying to SergeyBiryukov:

Replying to helen:

The global is_main_query() function is deprecated (see #23329) so the following patch is a no-go.

is_main_query() only issues a notice when being called from pre_get_posts. It still have a valid use case otherwise ("is $wp_query the main query?"), see comment:3:ticket:23329, which is exactly the use case here.

Came back here to say oops, because you are correct and I am asleep at the wheel. :) Also, upon further thought, by default this class seems rather unreliable (sometimes applied to secondary queries because of global state) outside of the good old first-page-of-home-main-query context. If we add is_main_query(), I guess the default behavior overall would become more correct, though I'd still be concerned about how (custom) themes are potentially affected. But in any case, let's revert and try again later.

Last edited 9 years ago by helen (previous) (diff)

#24 @markjaquith
9 years ago

Revert and try again later.

This ticket was mentioned in Slack in #core by mark. View the logs.


9 years ago

#27 @markjaquith
9 years ago

In 30913:

Revert [30036].

Merges [30912] to the 4.1 branch.

see #18035

#28 @SergeyBiryukov
9 years ago

  • Keywords revert removed
  • Milestone changed from 4.1 to Future Release

#29 @swissspidy
8 years ago

  • Keywords dev-feedback added

#30 @flixos90
6 years ago

  • Keywords good-first-bug removed

#31 @peterwilsoncc
6 months ago

  • Keywords changed from has-patch, dev-feedback to has-patch dev-feedback

Another contributor asked me to take a look at this.

As the sticky class on posts has worked in roughly the same way for approximately fifteen years, I think it would be too risky to change it now.

Possibly we could add a hoisted class for sticky posts that have been raised to the top with the block of code looking something like this (possibly with an is_main_query() thrown in for good measure):

<?php
global $wp_query;
// ...
// Sticky for Sticky Posts.
if ( is_sticky( $post->ID ) ) {
        if ( is_home() && ! is_paged() ) {
                $classes[] = 'sticky';
                if ( true !== (bool) $wp_query->get( 'ignore_sticky_posts' ) ) {
                        $classes[] = 'hoisted';
                }
        } elseif ( is_admin() ) {
                $classes[] = 'status-sticky';
        }
}

Note: See TracTickets for help on using tickets.