WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 5 weeks ago

#24142 reviewing defect (bug)

Zero value for posts_per_page value in wp_query custom instance and for 'Blog pages show at most' option

Reported by: alexvorn2 Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Query Keywords: needs-unit-tests needs-testing
Focuses: Cc:

Description

To show no posts if the posts_per_page value is 0.

Currently for custom instances of wp_query, if the value is 0 then this is changed with the value from posts_per_page option from the database. "get_options( 'posts_per_page' )"

For home page if we set value 0 on the settings page, in wp-admin/options-reading.php, after the saves are changed, this value is changed to 1.

I think for both cases if the posts per page value is 0 then no posts should not display.

Attachments (1)

24142.diff (3.7 KB) - added by Howdy_McGee 16 months ago.
Removal of empty() in favor of isset() is_numeric() and is_bool()

Download all attachments as: .zip

Change History (15)

#1 follow-up: @ocean90
8 years ago

  • Keywords reporter-feedback close added

What's the use case of having a query which should return 0 results?

#2 in reply to: ↑ 1 @alexvorn2
8 years ago

Replying to ocean90:

What's the use case of having a query which should return 0 results?

To hide all posts, for example for a short period of time if the site is under construction...

#3 @alexvorn2
8 years ago

  • Keywords needs-docs added; reporter-feedback removed

#4 @wonderboymusic
8 years ago

  • Keywords close needs-docs removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Why would you write a WP_Query by hand that wants zero posts? Why not just comment out the code that displays posts or something

#5 @DrewAPicture
8 years ago

A more valid use case for this might be if you're passing the post_per_page value through absint() or similar and zero gets returned.

I can follow the logic of -1 = unlimited, or 1 = 1, 2 = 2 and so on. So it's odd that we say, "Oh, by the way, 0 doesn't equal 0 here, it actually equals this other thing over there."

It kind of stands to reason that if I pass 0 I'd expect to get 0 results, intentional or not. Just sayin'.

#6 @randell
6 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

I agree with @DrewAPicture's logic on how zero is handled relative to how -1, 1, 2, 3, etc. is handled. Wasted so much time trying to figure out what's wrong with my code because of this unintuitive behavior.

#7 @boonebgorges
6 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone set to Future Release

I agree that it's a bit odd to create a WP_Query with posts_per_page=0, but I also think there's no reason we shouldn't support it. The culprit appears to be https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/query.php#L2460. Something like 0 === $q['posts_per_page'] || '0' === $q['posts_per_page'] is probably going to work, but this needs unit tests.

#8 @teraviva
4 years ago

I want to give you an other case. For example I have CPT Destination with Tours and Posts attached to it. And I have no need to show posts at Destination page, because I provide further links to inner categories for Tours and Info posts at this page.

#9 @birgire
4 years ago

I agree that this is a strange behavior.

To better understand it, we can map it out for version 4.8.2:

value		posts_per_page	showposts	posts_per_archive_page	posts_per_rss
--------------------------------------------------------------------------------------
null	|	default		default		default			default
''	|	default		default		default			default
0	|	default		default		default			default
'0'	|	default		default		default			default
true	|	none		1		none			1
false	|	default		default		default			default
10	|	10		10		10			10
'10'	|	10		10		10			10
'string'|	1		1		default			1
array() |	default		default		1			default
-1	|	none		none		none			none
-2 	|	2		2		2			2

where default means the stored option's value. We could e.g. use this table when writing tests and data providers.

So there are some strange inconsistencies, e.g. the true row ;-)

The scope? of the ticket seems to be adjust the the 0 and '0' rows to:

value		posts_per_page	showposts	posts_per_archive_page	posts_per_rss
-------------------------------------------------------------------------------
0	|	0		0		0			0
'0'	|	0		0		0			0

I think these two row cases, should be the same as returning an empty array from
the posts_pre_query filter,

https://core.trac.wordpress.org/browser/tags/4.8.2/src/wp-includes/class-wp-query.php#L2781

so we don't have to run those db queries. Like using $this->posts = array().

We would need to check for e.g.

if( isset( $q['posts_per_page'] ) && ( 0 === $q['posts_per_page'] || '0' === $q['posts_per_page'] ) ) {

as mentioned by @boonebgorges and similar for the other query variables.

We would also have to adjust this part accordingly:

https://core.trac.wordpress.org/browser/tags/4.8.2/src/wp-includes/class-wp-query.php#L1785-L1798

i.e.

$q['posts_per_page'] = (int) $q['posts_per_page'];

if ( $q['posts_per_page'] < -1 )
	$q['posts_per_page'] = abs($q['posts_per_page']);
elseif ( $q['posts_per_page'] == 0 )
	$q['posts_per_page'] = 1;

The case of e.g. 'showposts' => 'string' is a little bit tricky here to detect, because of the previous $q['showposts'] = (int) $q['showposts'];

Before writing any patches I think we should consider what the scope of this ticket should be,
i.e. should it be to only adjust the 0 and '0' rows or if it should also touch the true, false, 'string', array(), ... rows?

Hope this helps anyone that's going to tackle this problem.

Update: Then there's ticket #39945 for yet another edge case input value.

Last edited 4 years ago by birgire (previous) (diff)

#10 @Howdy_McGee
16 months ago

  • Keywords needs-testing added; needs-patch removed

I think we can get away with == if we convert the given value to integer after checking if we need to provide a default. Similar to the showposts conditional directly below pre_get_posts.

Giving posts_per_page a TRUE boolean already translates to 1 (post per page) so it would make sense for FALSE to translate to 0 and return 0 posts.

This certainly needs tested but from what I've run it works as expected. To compare to the previously listed chart:

value		posts_per_page	showposts	posts_per_archive_page	posts_per_rss
--------------------------------------------------------------------------------------
null	|	default		default		default			default
''	|	default		default		default			default
0	|	0		0		0			0
'0'	|	0		0		0			0
true	|	1		1		1			1
false	|	0		0		0			0
10	|	10		10		10			10
'10'	|	10		10		10			10
'string'|	default		default		default			default
array() |	default		default		default			default
-1	|	all		all		all			all
-2 	|	2		2		2			2

I think in the end it makes posts_per_page a bit more predictable for edges cases such as this. The biggest change users should be aware of is how booleans passed to posts_per_page works.

@Howdy_McGee
16 months ago

Removal of empty() in favor of isset() is_numeric() and is_bool()

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


15 months ago

#12 @SergeyBiryukov
15 months ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing

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


14 months ago

#14 @molinerozadkiel
5 weeks ago

use case for "posts_per_page" => 0 to display no posts:

I am building a ajax search bar in a header, I have all the logic already done and working from older projects, but in this case the designer had the brilliant idea to show no posts if the search bar is empty (because the results are shown within the header, so is good to now show anything if the user is not looking for something so that is won't produce visual clutter)
So, I would like to jus be able to pass a 0 there when the search bar is empty...

Besides, "0 to show 0 posts" makes sense, and not doing it like this makes the system more prone to errors and harder to debug, as stated by other users before.
0 counts as "False" in many cases so I imagine the problem is problbly comming from there somehow

Note: See TracTickets for help on using tickets.