WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#26653 closed defect (bug) (fixed)

Date Query "inclusive" logic

Reported by: mboynes Owned by: SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.7
Component: Query Keywords: has-patch 4.0-early commit
Focuses: Cc:

Description

When querying dates ranges using before and after, the date logic when 'inclusive' => true seems non-intuitive. Consider the following date query:

$date_query = array(
	'after' => array( 'year' => 2013, 'month' => 1 ),
	'before' => array( 'year' => 2013, 'month' => 3 ),
	'inclusive' => true
);)

I would expect this to search between 2013-01-01 00:00:00 and 2013-03-31-23:59:59, but it actually generates the following SQL:

( post_date >= '2013-01-31 23:59:59' AND post_date <= '2013-03-01 00:00:00' )

"Inclusive" is essentially only including one second of the most refined argument you pass (here, that's month). The logic is perfect when "inclusive" is set to false. It seems to me that when 'inclusive' => true, the $default_to_max parameter passed to WP_Date_Query::build_mysql_datetime() should be inverted. Patch attached which does this.

Attachments (3)

date.php.diff (1022 bytes) - added by mboynes 6 years ago.
Resolves date query before/after logic when "inclusive" is set to true
unit-test.26653.diff (2.1 KB) - added by oso96_2000 6 years ago.
Unit test and fixed diff route
26653.diff (2.4 KB) - added by DrewAPicture 6 years ago.
+ docs

Download all attachments as: .zip

Change History (23)

@mboynes
6 years ago

Resolves date query before/after logic when "inclusive" is set to true

#1 @mboynes
6 years ago

  • Cc mboynes@… added
  • Keywords has-patch added

#2 @wonderboymusic
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9
  • Version set to 3.7

@oso96_2000
6 years ago

Unit test and fixed diff route

#3 @oso96_2000
6 years ago

  • Keywords needs-unit-tests removed

#4 @oso96_2000
6 years ago

  • Keywords dev-feedback added

This ticket was mentioned in IRC in #wordpress-dev by oso96_2000. View the logs.


6 years ago

#6 @nacin
6 years ago

I'm not positive we should change this. I think we should look at some other libraries to see how they handle this. Really to get what you want, you should set an inclusive start and an exclusive end of 2013-03-02 00:00:00. I am not familiar with how other libraries/ORMs handle this, though I do know that this is how Gmail filters work.

#7 @Viper007Bond
6 years ago

The change makes sense to me and was an oversight on my part.

The whole point of the inclusive parameter is so that you don't have to add or subtract one to your date/times in order to get them outside of the range you want to include.

#8 follow-ups: @khromov
6 years ago

This is a huge issue. It doesn't make sense that the default is exclusive both before and after. The edge case of midnight will result in a posts not being shown if you have two queries select from "day 1 to day 2" and "day 2 to day 3", and the inclusive case will make it show twice.

Edit
Is it possible to introduce "inclusive_before" and "inclusive_after" flags to cope with this?

Any other workaround available? Right now I'm doing

strtotime($end_date. '+1 second');

to get start date inclusive, end date exclusive...but it's not very pretty.

Last edited 6 years ago by khromov (previous) (diff)

#9 in reply to: ↑ 8 @oso96_2000
6 years ago

Replying to khromov:

This is a huge issue. It doesn't make sense that the default is exclusive both before and after. The edge case of midnight will result in a posts not being shown if you have two queries select from "day 1 to day 2" and "day 2 to day 3", and the inclusive case will make it show twice.

Care to post an example of your query? I can search for a solution or an alternative

#10 in reply to: ↑ 8 @Viper007Bond
6 years ago

Replying to khromov:

Edit
Is it possible to introduce "inclusive_before" and "inclusive_after" flags to cope with this?

Any other workaround available? Right now I'm doing

strtotime($end_date. '+1 second');

to get start date inclusive, end date exclusive...but it's not very pretty.

This is a separate issue. Please open a new ticket for that suggestion.

#11 @wonderboymusic
6 years ago

  • Keywords 4.0-early added
  • Milestone changed from 3.9 to Future Release

Let's regroup on these early 4.0

#12 @wonderboymusic
6 years ago

This is too late in the cycle, we need to spend some more time in Unit Tests and make sure enough people properly understand how all of this stuff works before tossing it all in. I personally moved a lot of the Query tickets into 3.9 and want to see resolutions as much as anyone else...

#13 @nacin
6 years ago

Yeah, and for what's essentially a breaking change, we can't make this three weeks before a release. I'm okay with dropping it in early, preferably with a post on make/core as well to explain the change.

#14 @wonderboymusic
6 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 4.0

Patch still applies cleanly - let me know if anyone has any objections to this going in...

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


6 years ago

#16 follow-up: @helen
6 years ago

Should happen now or be punted. No opinion here.

#17 @DrewAPicture
6 years ago

  • Keywords needs-docs added

I think this has merit and seems like an oversight as mentioned in comment:7.

It is also, though, a breaking change as noted by Nacin in comment:13 so I'd prefer we add both an original @since for 3.7.0 as well as an @since 4.0.0 notating the change to the inclusive argument behavior.

#18 in reply to: ↑ 16 @mboynes
6 years ago

Replying to helen:

Should happen now or be punted. No opinion here.

Aside from the documentation additions as noted by DrewAPicture, is there anything we're waiting on for this? Are there any concerns not noted here?

@DrewAPicture
6 years ago

+ docs

#19 @DrewAPicture
6 years ago

  • Keywords needs-docs removed

26653.diff adds the docs described in comment:17.

#20 @SergeyBiryukov
6 years ago

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

In 28935:

WP_Date_Query: The inclusive logic should include all times within the date range.

props mboynes, oso96_2000, DrewAPicture.
fixes #26653.

Note: See TracTickets for help on using tickets.