View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0017980||mantisbt||administration||public||2014-12-22 13:41||2015-03-15 19:58|
|Priority||normal||Severity||minor||Reproducibility||have not tried|
|Target Version||1.3.0-beta.2||Fixed in Version||1.3.0-beta.2|
|Summary||0017980: manage_user_page php error due to time user creation time in the future|
For some reason the administrator created had a creation date in the future which caused the query for new user count fail due to a negative unsigned int. We should understand why this happened and possibly harden db_helper_compare_days() to handle such case. For example, before subtracting make sure date1 <= date2 || date1 - date2 <= 7 days.
APPLICATION ERROR 0000401
|Tags||No tags attached.|
This is likely caused by the recent change related to timezone handling installer_db_now() vs. db_now(). They handle timestamps differently.
There are several calls in manage_user_page that looks like this:
The '' translates to nothing. I wonder if the intention was to use "'" instead of ''.
This issue is like introduced by Paul's checkin below:
OK, I'll have a look. Can you pls provide steps to reproduce ?
Indeed it used to be "'". This was introduced by an old commit converting DateTimes in DB to integer .
I suppose we could remove these as part of a code cleanup commit after making sure that the code handles the type change correctly, as the use of '.' operator changes return value of db_now() from int to string). If string is needed, we could replace '' . db_now() . '' by (string)db_now() which would be clearer.
I had a quick look over lunch, and while I was not able to reproduce the problem, I believe the error occurs because MySQL is using unsigned data type for the substraction (date_created column is unsigned).
I don't think that's feasible, because the function needs to handle date columns, not just date values (integers) so the resolution happens at RDBMS level, not in PHP.
We could use a type cast, but implementation would be difficult while maintaining cross-DB compatibility - SQL-92 defines a CAST(value AS type) function, but the available target types are not standardized across all platforms.
I'm thinking that a (probably) better alternative could be to refactor the function to return
date1 [operator] date2 + [number of days] ==> 1419273289 <= date_created + 604800
date1 - date2 [operator] [number of days] ==> 1419273289 - date_created <= 604800
Please test and review: https://github.com/mantisbt/mantisbt/pull/564
EDIT: I'm not sure that the PR actually fixes the root cause of this issue (date created in the future), just the symptoms (MySQL error), but as I was not able to reproduce the problem...
In all cases, I noticed that we compare against db_now(). Is there a reason why the code can't look like the following pseudo-code:
$t_cutoff_timestamp = time() - $t_days * SECONDS_PER_DAY;
That is easier to understand. I think the method is adding unneeded complexity. In terms of code reviews, we should make sure we avoid $t_timestamp1 - $t_timestamp2 in our queries.
We need in any case to use a DB parameter:
'SELECT table WHERE timestamp >= ' . db_param()
This allows the RDBMS to optimize/cache the query and avoids the SQL being re-parsed every time it is executed.
Yep, that is why I called it pseudo code. We also need to use a real table name :)
The point is that we don't really need this method. What do you think about removing it completely?
timestamp.php (448 bytes)
<?php require_once( 'core.php' ); echo "Time = " . time() . "<br />"; global $g_db; echo "Bind timestamp = " . strtotime( $g_db->BindTimeStamp( time() ) ); echo "<br />"; $t_timezone = @date_default_timezone_get(); echo "Timezone = $t_timezone<br />"; date_default_timezone_set( 'UTC' ); $t_time = $g_db->BindTimeStamp( time() ); @date_default_timezone_set( $t_timezone ); echo "Bind timestamp with zone = " . strtotime( $t_time ) . "<br />";
timestamp.php (448 bytes)
I've attached a script that reproduces the problem:
Here is the output of the script:
Time = 1419358917 (time())
The last once is what is used for date_created for the administrator user. It is 8 hours in the future compared to time() which we use later in the code. This issue may reproduce based on the timezone of the machine compared to UTC. My timezone (Los Angeles timezone) is -8, hence UTC is behind local. Your timezone may be different, i.e. local behind UTC.
Basically, it is likely that even though time() returns UTC, the ADODB library assumes it is local time and translates it again to UTC by adding 8 hours.
I had a look at ADOdb source code, and in fact this is exactly what is happening:
ADOConnection::BindTimestamp ==> ADOConnection::DBTimeStamp ==> ADOConnection::UnixTimeStamp ==> adodb_mktime ==> mktime (NOT gmmktime)
I need to go through the code again and make some tests, but I think we should probably revert grangeway's change to schema.php introduced by https://github.com/mantisbt/mantisbt/commit/b0d0c9dc90.
MantisBT: master 0a33bdfd
2014-12-23 06:25:44Details Diff
1. Function renamed to db_helper_compare_time()
2. It now accepts 4 parameters, which have been reordered
- date 1
- an SQL operator to use for the comparison
- date 2
- the number of seconds to compare against
Note: the date parameters should only be strings (column names);
date constants should be passed as DB parameters
3. The comparison is rewritten based on sign of $p_num_secs to avoid
issues with unsigned integers on MySQL
Returns: date1 [operator] date2 + days
All occurences of the function in MantisBT code base have been updated
|mod - core/database_api.php||Diff File|
|mod - core/news_api.php||Diff File|
|mod - core/summary_api.php||Diff File|
|mod - manage_user_page.php||Diff File|
|mod - manage_user_prune.php||Diff File|
MantisBT: master 53f14198
2015-01-20 06:48:43Details Diff
|Partial revert 'Additional timezone init fixes'
This reverts the changes to schema.php introduced by commit
It is not necessary (and in fact, incorrect) to change timezone to UTC
prior to calling ADOConnection::BindTimestamp(), because ADOdb considers
the given timestamp to be local.
As a consequence, the timezone translation is applied twice, causing the
timestamp to be in the future.
A comment to explain this behavior has been added to the code, to avoid
this issue popping up again in the future.
|mod - admin/schema.php||Diff File|
|2014-12-22 13:41||vboctor||New Issue|
|2014-12-22 13:41||vboctor||Status||new => assigned|
|2014-12-22 13:41||vboctor||Assigned To||=> vboctor|
|2014-12-22 13:47||vboctor||Note Added: 0042048|
|2014-12-22 13:47||vboctor||Assigned To||vboctor => dregad|
|2014-12-22 13:52||vboctor||Note Added: 0042049|
|2014-12-22 23:25||vboctor||Note Added: 0042050|
|2014-12-23 04:51||dregad||Note Added: 0042052|
|2014-12-23 08:19||dregad||Note Added: 0042053|
|2014-12-23 08:23||dregad||Note Edited: 0042053||View Revisions|
|2014-12-23 12:04||vboctor||Note Added: 0042058|
|2014-12-23 12:43||dregad||Note Added: 0042059|
|2014-12-23 12:53||vboctor||Note Added: 0042060|
|2014-12-23 13:21||vboctor||File Added: timestamp.php|
|2014-12-23 13:25||vboctor||Note Added: 0042061|
|2014-12-23 13:26||vboctor||Note Added: 0042062|
|2015-01-20 06:44||dregad||Note Added: 0042216|
|2015-01-20 06:53||dregad||Note Added: 0042217|
|2015-02-02 06:03||dregad||Relationship added||related to 0019317|
|2015-02-03 17:00||dregad||Changeset attached||=> MantisBT master 0a33bdfd|
|2015-02-03 17:00||dregad||Changeset attached||=> MantisBT master 53f14198|
|2015-02-03 17:00||dregad||Status||assigned => resolved|
|2015-02-03 17:00||dregad||Resolution||open => fixed|
|2015-02-03 17:00||dregad||Fixed in Version||=> 1.3.0-beta.2|
|2015-03-15 19:58||dregad||Status||resolved => closed|