Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#26653 closed defect (bug) (fixed)

Date Query "inclusive" logic

Reported by: mboynes's profile mboynes Owned by: sergeybiryukov's profile 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 10 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 10 years ago.
Unit test and fixed diff route
26653.diff (2.4 KB) - added by DrewAPicture 10 years ago.
+ docs

Download all attachments as: .zip

Change History (23)

@mboynes
10 years ago

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

#1 @mboynes
10 years ago

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

#2 @wonderboymusic
10 years ago

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

@oso96_2000
10 years ago

Unit test and fixed diff route

#3 @oso96_2000
10 years ago

  • Keywords needs-unit-tests removed

#4 @oso96_2000
10 years ago

  • Keywords dev-feedback added

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


10 years ago

#6 @nacin
10 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
10 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
10 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 10 years ago by khromov (previous) (diff)

#9 in reply to: ↑ 8 @oso96_2000
10 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
10 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
10 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
10 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
10 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
10 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.


10 years ago

#16 follow-up: @helen
10 years ago

Should happen now or be punted. No opinion here.

#17 @DrewAPicture
10 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
10 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
10 years ago

+ docs

#19 @DrewAPicture
10 years ago

  • Keywords needs-docs removed

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

#20 @SergeyBiryukov
10 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.