date_query inclusive logic still not quite right

[magicroundabout]

[Please be gentle - I've not contributed much!]

This was sort-of addressed in #26653, but I'm re-raising it because the inclusive logic still doesn't seem right to me.

I've done:

date_query => array(
  "after" => 2013-04-07,
  "before" => 2013-10-13,
  "inclusive"=> true,
}

And this generates:

AND ( ( post_date >= '2013-04-07 00:00:00'
AND post_date <= '2013-10-13 00:00:00' ) )

This is NOT inclusive of the before date. It should be:

AND ( ( post_date >= '2013-04-07 00:00:00'
AND post_date <= '2013-10-13 23:59:59' ) )

I've not had time to dive into the code and see how this is generated yet. Might have time to do that later. Will log some more detail then if I can.

Thanks

[boonebgorges]

Confirmed. See 29908.test.patch​ for an automated test that illustrates the issue.

The root of the problem is that when you pass a string value as 'before' or 'after', no "inclusive" logic is run: it's converted to 'Y-m-d H:i:s' and passed back to the SQL generator. See line 349.

There are a couple ways to fix this for WP. Probably the most thorough is to convert string values into year/month/day/hour/minute/second arrays, and then run it through the rest of the logic of build_mysql_datetime() to convert it back to MySQL format.

magicroundabout - For your specific case, you can workaround this issue by either (a) using an array 'before' => array( 'year' => 2013, 'month' => 10, 'day' => 13 ), or (b) creating a more precise date string 'before' => '2013-01-13 23:59:59'.

[magicroundabout]

Ah. Yes. Of course. Thanks @boonebgorges (P.S. Love that you're doing advanced query improvements for 4.1)

The strtotime() means that this is correct and, to be fair, documented (if slightly unexpected) behaviour. I should have got that.

I'm happy not to patch this if the documentation is made a bit clearer. I've added a note to the codex example to clarify to others!

Do you think that this needs fixing by doing the string-to-array conversion that you suggest? If we do, we'll have to remember to remove the note from the codex.

Thanks

[nacin]

It would probably be easier if it generated this instead:

AND ( ( post_date >= '2013-04-07 00:00:00'
AND post_date < '2013-10-14 00:00:00' ) )

[magicroundabout]

Presumably the inverse is also true. If I pass a string-formatted date (with no time) and set inclusive to false, the string is converted to, say, 2013-04-07 00:00:00 and is therefore, counter-intuitively, inclusive?

[boonebgorges]

Replying to nacin:

It would probably be easier if it generated this instead:

AND ( ( post_date >= '2013-04-07 00:00:00'
AND post_date < '2013-10-14 00:00:00' ) )

If I were building this from scratch, I might agree with you. But the rounding-up logic is already in WP_Date_Query, for when an incomplete array value like array( 'year' => 2013, 'month' => 04 ) is passed. So it makes sense to use it.

The real challenge here is figuring out what date formats to accept. [29908.patch] adds inclusive support for Y, Y-m, Y-m-d, Y-m-d H:i, and Y-m-d H:i:s, as well as the same formats with / instead of -. (If there is PHP tool for computing the date format from a string - _without_ rounding it down, like strtotime() - I couldn't find it. Obviously they have the internals....) I figure this is better than nothing, and the first three in particular will probably cover many use cases.

magicroundabout, you want to have a look and see if it's working for you?

[magicroundabout]

Why don't we date_parse()?

The only issue that I can see with using date_parse() is that it would be breaking if, say, someone had sent a date AND time string in before or after (e.g. 'before' => '2013-01-13 12:00:00'). Arguably, the documentation doesn't say that that will work. It says "Date to retrieve posts after. Accepts strtotime()-compatible string"

I'm happy to code up a new patch and/or test Boone's work.

Is there some info somewhere on the best way to create and test out patches? Would love to contribute (finally!). It's just not something I've done before. I'm an experienced dev with multiple local and remote test installs, so happy to talk as technical as you like, but just not worked with trac and patches much.

[boonebgorges]

Replying to magicroundabout:

Why don't we date_parse()?

Yeah, I tried that too :) date_parse() does some rounding down. date_parse( '2014-05' ) returns day=1. So there's no way to know, just by looking at the results of date_parse(), whether the intended value was '2014-05-01' or just '2014-05'. It would be possible to use date_parse() for some formats (it's reliable for Y-m-d, for example), but relying on this somewhat odd behavior for certain cases seemed odd.

Is there some info somewhere on the best way to create and test out patches? Would love to contribute (finally!).

Absolutely - we'd love to have your contributions. Check out the Contributor Handbook: ​https://make.wordpress.org/core/handbook/the-wordpress-codebase/ You'll see links along the left-hand menu for using Trac and SVN for generating and applying patches.

[dd32]

Replying to boonebgorges:

The real challenge here is figuring out what date formats to accept. 29908.patch​ adds inclusive support for Y, Y-m, Y-m-d, Y-m-d H:i, and Y-m-d H:i:s, as well as the same formats with / instead of -.

I'd question the inclusion of / personally, It's not a date format used within code very often (IMHO) and could increase confusion of it supporting Y/d/m. Is there any harm in only supporting the standard Y-m-d H:i:s (or part of) mysql format?

[boonebgorges]

Replying to dd32:

I'd question the inclusion of / personally, It's not a date format used within code very often (IMHO) and could increase confusion of it supporting Y/d/m. Is there any harm in only supporting the standard Y-m-d H:i:s (or part of) mysql format?

Nope, no harm. I agree the / format is not used much when writing code. I just threw it in at the last minute because it was a minor mod to the regular expression. It's pretty arbitrary where we draw the line between supported formats and unsupported formats, but maybe saying "we support subsets of MySQL format" is a bit less arbitrary than other choices. Happy to go with majority opinion on this point.

[boonebgorges]

In 29936:

Better "inclusive" support for string values in WP_Date_Query.

The 'inclusive' parameter for WP_Date_Query determines whether non-precise
dates for 'before' and 'after' will be rounded up or down. Previously, this was
supported only when 'before' and 'after' were arrays; string-formatted dates
were run through strtotime(), which rounded them all down (inclusive in the
case of after, non-inclusive in the case of before). Now, we attempt to parse
formats that look like MySQL-formatted date strings, and apply inclusive logic
to them if we recognize them successfully.

Fixes #29908.
string values. Array values support the 'inclusive

[boonebgorges]

(FYI - I dropped support for the /. Thanks for the feedback, dd32!)