To get todays date, the MySQL only function CURDATE() or CURRENT_DATE() is being used in many places. Neither CURDATE() nor CURRENT_DATE() must be used, but instead the standard form CAST(now() AS date).
Created attachment 8526 [details] [review] Remove CUR_DATE() and CURRENT_DATE() MySQLism
Created attachment 8616 [details] [review] Remove date MySQLism Re-created the patch because the first version accidentally added trailing whitespace.
Created attachment 8901 [details] [review] Update patch tto remove a MySQLism Updated patch to reflect recent changes.
Created attachment 8952 [details] [review] Bug 7802: [SIGNED-OFF] Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date)
Created attachment 8953 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date) The precedent patch forgot an occurrence in C4/Reserves.pm
In the first patch, changes in C4/Overdues.pm from NOW() to CAST(now() as DATE) are no longer equivalent since the introduction of hourly loans. date_due is now a datetime, so the check on NOW() should not be cast. Marking first patch Failed QA. Will address second patch after this has been fixed.
Created attachment 19643 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 19644 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date).
(In reply to Jonathan Druart from comment #7) > Created attachment 19643 [details] [review] [review] > Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by > CAST(now() AS date). > > Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Jonathan: Is this still waiting for Sign Off?
(In reply to David Cook from comment #9) > Jonathan: Is this still waiting for Sign Off? David, Yes, the followup needs a signoff.
Created attachment 20057 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20058 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date).
(In reply to Kyle M Hall from comment #12) > Created attachment 20058 [details] [review] [review] > Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace > by CAST(now() AS date). Kyle, you forgot to add the Signed-off-by line to the second patch.
Created attachment 20733 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Do we have a reference about CURRENT_DATE being a mysqlism? I found it here: http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt Also bit worried about changing all those statements without tests.
(In reply to Katrin Fischer from comment #15) > Do we have a reference about CURRENT_DATE being a mysqlism? > I found it here: http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt At least, it does not work with PostgreSQL: postgres=# select CURRENT_DATE(); ERREUR: erreur de syntaxe sur ou près de « ( » LIGNE 1 : select CURRENT_DATE(); ^ postgres=# select current_date; date ------------ 2013-09-09 > Also bit worried about changing all those statements without tests. I don't have the time to provide unit tests for all modified routines. But MariaDB [koha_devstetienne]> select CURRENT_DATE(); +----------------+ | CURRENT_DATE() | +----------------+ | 2013-09-09 | +----------------+ 1 row in set (0.00 sec) MariaDB [koha_devstetienne]> SELECT CAST(now() AS date); +---------------------+ | CAST(now() AS date) | +---------------------+ | 2013-09-09 | +---------------------+ 1 row in set (0.00 sec)
Weird, it's in the Postgres documentation too? http://www.postgresql.org/docs/8.1/static/functions-datetime.html
(In reply to Katrin Fischer from comment #17) > Weird, it's in the Postgres documentation too? > http://www.postgresql.org/docs/8.1/static/functions-datetime.html Yes select current_date; -- works select current_date(); -- does not work
back to need QA.
Created attachment 22755 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22756 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I see the following change: - AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY) + AND reserves.reservedate <= CAST(now() AS date) Fixing a so detested mysqlism should at least preserve functionality. These changes seem to be made in a hurry, since we are changing functionality here. Here you change a date_add with just a now. The parameter for adding a number of days is just ignored. This change was a red traffic light for me :) I did not look any further.. Please correct and add a (more detailed) test plan for this patch.
(In reply to M. de Rooy from comment #22) > I see the following change: > > - AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY) > + AND reserves.reservedate <= CAST(now() AS date) > > Fixing a so detested mysqlism should at least preserve functionality. These > changes seem to be made in a hurry, since we are changing functionality here. No, not the changes, but the rebase yes. Only the last patch introduces this bad conflict merge. I consider after 3 rebase and 1 year, the rebase is not safe at all, there is here a good example... > Here you change a date_add with just a now. The parameter for adding a > number of days is just ignored. This change was a red traffic light for me > :) I did not look any further.. > > Please correct and add a (more detailed) test plan for this patch. There is no valid test plan. Check that the CUR_DATE(), CURRENT_DATE() mysql functions return the same as CAST(NOW() AS DATE) in your mysql cli. Your can verify, looking at the diff code, the patch is correct. If you want to test, you can play with reserves and holds queue.
Created attachment 22760 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22761 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22816 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22817 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment on attachment 22817 [details] [review] Bug 7802: Followup Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Review of attachment 22817 [details] [review]: ----------------------------------------------------------------- ::: C4/Koha.pm @@ +1431,4 @@ > # Fall through... we also return a random quote as a catch-all if all else fails > } > else { > + $query = 'SELECT * FROM quotes WHERE timestamp LIKE CONCAT(CAST(now() AS date),\'%\') ORDER BY timestamp DESC LIMIT 0,1'; Ironically, PostgreSQL will happily accept CURRENT_DATE (without the parentheses). It won't accept "LIMIT 0,1". It also won't like trying to compare a timestamp to a string datatype. Closer to the mark (as in, I've actually tested this on both MariaDB and Pg is "select * from quotes where cast(now() as date) = cast(timestamp as date) limit 1 offset 0;" A broader point: I don't think that the point should be to search-and-replace fragments of non-portable SQL. Actually ensuring that the output is portable would be more on target.
Comment on attachment 22816 [details] [review] Bug 7802: Remove CUR_DATE() and CURRENT_DATE() MySQLisms, replace by CAST(now() AS date). Review of attachment 22816 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/longoverdue.pl @@ +130,5 @@ > SELECT items.itemnumber, borrowernumber, date_due > FROM issues, items > WHERE items.itemnumber = issues.itemnumber > + AND DATE_SUB(CAST(now() AS date), INTERVAL ? DAY) > date_due > + AND DATE_SUB(CAST(now() AS date), INTERVAL ? DAY) <= date_due DATE_SUB() is not portable to PostgreSQL. As implied by my previous comment, I'm much more interested in seeing patches that provably improve portability of the SQL, not ones that just search-and-replace.
(In reply to Jonathan Druart from comment #16) > I don't have the time to provide unit tests for all modified routines. And I don't have the inclination to push a patch series that touches a bunch of routines without adequate test coverage, particularly (as my two immediately previous comments indicate) as it includes at least one case where it has the effective of making it harder to tell that a particular SQL statement is not portable. Setting to failed QA. Feel free to resubmit a partial patch that covers routines that have test cases, and feel free to write UTs.
I don't plan to provide a better way to fix this. I reset the assignee to default.
Wouldn't a user_defined_function be better suited to the task? One could create a koha_getdate() or even a koha_getdatetime() and they would only need be be changed once, depending on RDBMS used, until koha starts using a db abstraction layer that is.
Still an issue: C4/Acquisition.pm: DATEDIFF(CURDATE( ),closedate) AS latesince C4/Acquisition.pm: claimed_date = CURDATE() C4/NewsChannels.pm: AND DATE(timestamp) < DATE_ADD(CURDATE(), INTERVAL 1 DAY) circ/overdue.pl: $strsth .= " AND borrowers.debarred >= CURDATE()" ; misc/cronjobs/longoverdue.pl:our $bounds_sth = C4::Context->dbh->prepare("SELECT DATE_SUB(CURDATE(), INTERVAL ? DAY)"); misc/cronjobs/longoverdue.pl: AND DATE_SUB(CURDATE(), INTERVAL ? DAY) > date_due misc/cronjobs/longoverdue.pl: AND DATE_SUB(CURDATE(), INTERVAL ? DAY) <= date_due misc/cronjobs/remove_temporary_edifiles.pl:and datediff( CURDATE(), transfer_date ) > 5