date_sql_handler STR_TO_DATE math incorrect

@coltrane from #856140: Reminder E-mails Sent as Soon as Cron Runs I was seeing the same issues, and this patch seems to do the trick. I haven't tested it out exhaustively, but so far so good. Not sure that qualifies for RTBC, but +1 from me.

This change causes all events to disappear from my calendar view.

subscribe

When the date patch suggested above encounters a view which is set to filter a date by a granularity of less than a day - this causes the view to return nothing. Specifically, it appears the ADDTIME function doesn't cope well with being passed the results of a DATE_FORMAT call. Here's the where clause of my view which doesn't work.

WHERE ((node.status <> 0) AND (node.type in ('event'))) AND (DATE_FORMAT(ADDTIME(DATE_FORMAT(STR_TO_DATE(node_data_field_event_date.field_event_date_value, '%Y-%m-%dT%T'), '%Y-%m-%dT%T'), SEC_TO_TIME(-18000)), '%Y-%m-%d\T%H:%i') > '2011-09-22T10:25') ORDER BY node_data_field_event_date_field_event_date_value ASC

Am I correct that STR_TO_DATE takes in a string and returns a DATETIME value, while DATE_FORMAT takes in a DATETIME value and returns a string? If so, in the code listed by the OP, are we comparing a string with a DATETIME value, whereas after the patch we are comparing string to string?

I finally got all my views to show using the recent dev version of date and the patch. I had to change the granularity of the date filter from minute to day. This doesn't yet appear to cause any adverse effects.

An update - while for those views that you can change to a granularity of Day - this solution above works. However, if you need hour, minute or second granularity for a view, this change to the Date module gives incorrect results.

I found this in the MySQL manual, for DATE_ADD():

The return value depends on the arguments:

    - DATETIME if the first argument is a DATETIME (or TIMESTAMP) value, or if the first argument is a DATE and the unit value uses HOURS, MINUTES, or SECONDS.

    - String otherwise.

To ensure that the result is DATETIME, you can use CAST() to convert the first argument to DATETIME.

I think it's possible for DATE_ADD() to be returning a type/value that isn't comparable to Signup's input. Based on MySQL's recommendations I added a CAST() to DATETIME in the DATE_ADD() and DATE_SUB() calls in Date module. This fixed it for us without having to change signup or our other modules that use different intervals.

The only thing I'm unsure about is whether or not there is a use case for working with DATE, sans time.

Tested and seems to fix the issue on the Signup module.

Earlier in the code, in sql_offset(), we switched to using ADDTIME instead of DATE_ADD for later versions of MYSQL. I wonder if we need the same change here. The sql_date_math() function is not used much so may need to be updated.

No, ADDTIME() takes a TIME or DATETIME, adds a TIME and returns a TIME or DATETIME.
It does not support INTERVAL and so cannot deal with the $granularity needed by sql_date_math().
In fact, I can see no reason why ADDTIME() is used in sql_offset() except that maybe it is slightly faster due to being simpler.

Patch from #9 looks simple and clean and solves our problems.

Still applies too.

Applied to 6.x-2.x-dev.

The patch in this ticket needs to be ported to the D7 version.

Here's an updated version of the patch for 7.x-2.9-beta2

Can you ensure that patch applies against HEAD please, 7.x-2.x?

Re-rolled.

I need some help understanding the status of this issue.

On 2013-Oct-10, in comment #14, cafuego said that the patch was applied to "Applied to 6.x-2.x-dev" and changed the status to "Fixed" which I believe refers to the patch by djdevin in comment #9.) I am using Date 6.x-2.10 which is dated 2014-Mar-31. I assumed that the bug fixes made to Date 6.x-2.x-dev prior to 2014-Mar-31 would be included in that release. (Date 6.x-2.x-dev is currently dated 2015-May-09.)

Is that wrong?

I ask because I am using Date 6.x-2.10 and Signup 6.x-1.x-dev, and I am still having the problem of reminder e-mails being sent as soon as Cron runs. I don't know if I need to upgrade to or if I need to pursue another avenue for fixing this problem.

The 3 year old patch in #20 to date_api_sql.inc still applies cleanly to the latest 7.x-2.x-dev. Would anyone be able/willing to review/test this patch?

I reviewed and tested the original D6 patch and have used the D7 patch for many years without issue.

Committed. Thanks.