Skrol29 / tinybutstrong

TBS is a PHP template engine for pro and beginners. Only 1 class with few methods properties, but it can do may things for any text templates, including HTML and XML. The only engine that enables W3C compliant templates. It has many plugins including OpenTBS.
http://www.tinybutstrong.com
60 stars 18 forks source link

Formatting some timestamps results in wrong date #10

Closed mindhaq closed 5 years ago

mindhaq commented 6 years ago

When a field with a date format is merged with a timestamp, the result is - sometimes - wrong, if the timestamp is passed in as a string and not a number.

Example:

$timestamp1 = 1523447097;   // 13:44:57
$timestamp2 = 1528648609;   // 18:36:49

$TBS = new clsTinyButStrong;
$TBS->Source = "[timestamp1;frm='hh:nn:ss'] [timestamp2;frm='hh:nn:ss']\n[timestamp1string;frm='hh:nn:ss'] [timestamp2string;frm='hh:nn:ss']";
$TBS->MergeField("timestamp1", $timestamp1);
$TBS->MergeField("timestamp2", $timestamp2);
$TBS->MergeField("timestamp1string", "$timestamp1");
$TBS->MergeField("timestamp2string", "$timestamp2");
$TBS->Show();

Output:

13:44:57 18:36:49
15:23:44 18:36:49

I found that out while trying to find a fix for https://github.com/kimai/kimai/issues/1032

Version 3.10.0 for PHP 5

roxblnfk commented 6 years ago

I do not think this is a bug. If you pass time by a timestamp, then pass it as integer

On the other hand it would be more friendly to check the string for the presence of only digits and in this case to convert to int :/

mindhaq commented 6 years ago

I agree that you should pass it as an integer. In my case, the data was read from a database, and all columns where read as strings.

But the results are inconsistent if you are not passing it as one. timestamp2 is always converted correctly, timestamp1 is not treated as a timestamp and split into digits.

This certainly smells like a problem, and might affect other use cases as well.

roxblnfk commented 6 years ago

Just such a mechanics in the function strtotime

let's fix it?

roxblnfk commented 6 years ago

In the test code of the TBS plugin there is a check for this behavior. I made a fix, but on this line the test fails

$this->assertEqualMergeFieldStrings("{[a;frm=yyyy-mm-dd hh:nn:ss]}", array('a'=>'20011205'), "{2001-12-05 00:00:00}", "test string values (compact without hour)");


But i can compare a string length and tests will passes. But i thik it's bad code

if (strlen($Value) > 6 && ctype_digit($Value)) {
    $x = (int)$Value;
} else {
    $x = strtotime($Value);
}
mindhaq commented 6 years ago

I'm sorry, but I'm of no big help here. I only learned about tinybutstrong because I wanted to fix that bug in Kimai. I'm not too much into PHP anymore as well.

Anyhow, I think if you try to fix this, all old test cases should still work, otherwise you'll just introduce new bugs.

darkain commented 6 years ago

These inconsistencies with date conversion were the initial reason why I forked this project in the first place. The original author intended for these awkward conversions due to oddities with certain database systems. Though, since forking the project, I've entirely changed/simplified the syntax since then, so my version with fixed date assuming wouldn't work as a direct drop in replacement. Checking my emails, it was a solid five years ago that this bug was reported to the main TBS author though, with no resolution at that time.

Skrol29 commented 6 years ago

Hi, This is not an inconsistency. When you have a string such as « 20180610 » no one can tell if it's a date or a timestamp. So TBS assumes that a string value will be always converted with strtotime() before a date formatting with date() ; while any numerical value will be directly formated with date().

I will notice that in the manual.

darkain commented 6 years ago

When running into this issue and reporting it back in 2013, I also discovered that others had ran into this issue prior to that as well. There is a specific edge case for the timestamp 943916400 in the code, because that one also bugged out. https://github.com/Skrol29/tinybutstrong/search?q=943916400

There was also inconsistencies between the 32-bit and 64-bit version of PHP.

Like I said, I corrected these issues in a fork, but other major syntax is changed as well. I might at some point do a conversion guide to show how to use it, since I've been trying to simplify and extend the syntax significantly. https://github.com/darkain/TinyButXtreme/

mindhaq commented 6 years ago

@Skrol29 With your example, it is true that it's not possible to see the difference. But I don't see any reason why the string 1523447097 should be interpreted as the time 15:23:44 and 7097 10^-4 seconds, and the string 1528648609 not being interpreted in a similar way (probably because 64 seconds is invalid).

If strtotime does that, then that's a problem.

Skrol29 commented 6 years ago

@mindhaq

Output: 13:44:57 18:36:49 15:23:44 18:36:49

I have 13:44:57 18:36:49 13:44:57 18:36:49 With TBS 3.10.1 and both PHP 5.6.30 and PHP 7.2.3

Skrol29 commented 6 years ago

@darkain timestamp 943916400 is the result for strtotime('00-00-00') or strtotime('2000-00-00'). This value is checked by TBS only for conditional formats. This is for compatibility with old version.

In my opinion, it is always recommended to retrieve date-time or timestamp values from a database and into PHP as a non ambiguous formatted string. Most of database drivers does that be default. The format is often "yyyy-mm-dd hh:ii:ss". This representation is relatively all-terrain between systems.

darkain commented 6 years ago

Unix timestamps are unambiguous, and are extremely common. They're integers, that's it. That's the exact thing everyone is mentioning is the issue, the fact that Unix timestamps that should be treated as such are instead being processed by arbitrary string conversions into other formats, but only with certain numbers and not others. This inconsistent handling of Unix timestamps is what is leading to bugs in all of our applications (and why I had to fork the project to remove such code from it)

Also, strtotime('00-00-00') or strtotime('2000-00-00') becomes Unix timestamp 943920000 UTC, unless you have an alternate timezone set. Which means this is just another "magic" conversion based on local configuration or improper configuration of other parts of the system. https://3v4l.org/IXgBQ

Skrol29 commented 6 years ago

@darkain

Unix timestamps do make ambiguity regarding to some other possible date-time strings. In PHP, if you want a string to be an explicit Unix timestamp, then prefix it with '@'.

str2time('@20142000') returns 20142000 (corresponding to date '1970-08-22 04:00:00') while str2time('20142000') returns false. This is the strtotime() matter, not a TBS behavior.

TBS works ok with explicit Unix timestamps such as '@20142000'.