Ostico / PhpOrient

PhpOrient - Official Php driver based on the binary protocol of OrientDB.
Other
68 stars 37 forks source link

Date older than 1970-01-01 returning as false #91

Open sameer-shelavale opened 7 years ago

sameer-shelavale commented 7 years ago

hi, I am facing this strange problem, I have a Date field in my OrientDB database which is storing a birthdates. I am able to see the dates using OrientDB Browser. When I execute a query using PHPOrient, those dates older than 1970-01-01 are returned as false. However for newer dates it returns the DateTime Object with correct value. Am I missing something here? I am using a latest dev-master and have tested with orientdb 2.1.9 and 2.2 Right now I am forced to use dob.format('yyyy-MM-dd')

Ostico commented 7 years ago

Hib @sameer-shelavale ,

can you send a code snippet to create/reproduce the issue?

Ostico commented 7 years ago

I tried on my side with OrientDb 2.2.0 and this script: https://gist.github.com/Ostico/4187e07a0f85cb509c4cf98cd8de06fd it works fine.

andreyvk commented 7 years ago

@Ostico im facing the same problem. Basically what happened is that user manually entered a date in our system and format was not an acceptable one. From the Studio date showed as 31/12/0305 (clearly some parsing issues in our software). So PHPOrient library threw an exception on that record :

PHP Fatal error: Call to a member function setTimeZone() on a non-object in /home/ubuntu/shared/www/nsure/lib/vendor/ostico/phporient/src/PhpOrient/Protocols/Binary/Serialization/CSV.php on line 241

I've looked at the CSV.php code and debugged it. The line $collected = \DateTime::createFromFormat( 'U', substr( $collected, 0, -3 ) ); indeed returned false. Plz, take a look

Ostico commented 7 years ago

Hi @andreyvk , this is a bug clearly not related with the driver, anyway please perform a select in the OrientDB with its command line console and show me the internal format of the date field.

andreyvk commented 7 years ago

Hi @Ostico ,

Here's the date field for that record as is:

orientdb {db=test}> select arrivalDate from MyClass where id='0ef5fd3c-7449-4ac0-9dfb-e4363df7a4d3'

+----+-------------------+
|#   |arrivalDate        |
+----+-------------------+
|0   |0305-12-31 00:00:00|
+----+-------------------+

You could be right though. It might be serialization problem on the DB side. In which case we'll just forward this issue to the OrientDB team.

andreyvk commented 7 years ago

@Ostico

Have you had a chance to look at the problem? We are having more issues now with clients because of this.

Btw, here's how to reproduce it (we are using Orient 2.2.19):

create class Test extends V
create property Test.dated date
insert into Test content {"dated": "1969-12-31"}

and then

try {
    $client = new \PhpOrient\PhpOrient( 'localhost', 2424 );
    $client->username = 'admin';
    $client->password = 'admin';
    $client->connect();
    $client->dbOpen( 'test', 'admin', 'admin' );

    $records = $client->query("select from Test");

    //output (DOES NOT GET TO THIS LINE)
    print_r($records);
}
catch(\Exception $e) {
    print_r($e->getMessage());
}

throws exception and prints

PHP Fatal error:  Call to a member function setTimeZone() on a non-object in /home/ubuntu/shared/www/nsure/lib/vendor/ostico/phporient/src/PhpOrient/Protocols/Binary/Serialization/CSV.php on line 241

Please fix this asap if possible. I hope this helps. Thanks in advance!

andreyvk commented 7 years ago

@Ostico I found smth interesting abt DateTime::createFromFormat(...). It indeed has problems converting negative time into date (see https://stackoverflow.com/questions/23103451/php-how-to-create-date-before-the-epoch-1970-using-date-instead-of-datetime). Moreover, implementation using substr( $collected, 0, -3 ) returns false when $collected is 0 (or less than 3 characters long for that matter). I'd suggest the following implementation instead:

//starting phporient/src/PhpOrient/Protocols/Binary/Serialization/CSV.php line 239
$dt = new \DateTimeZone( date_default_timezone_get() );

//instead of substr( $collected, 0, -3 );
$ts = (int) ($collected / 1000); 

//instead of \DateTime::createFromFormat( 'U', substr( $collected, 0, -3 ) );
$collected = new \DateTime();
$collected->setTimestamp($ts);
$collected->setTimeZone( $dt );

Plz let me know what u think

andreyvk commented 7 years ago

hi @Ostico,

Have you had any chance to look at this?

Ostico commented 7 years ago

Hi @andreyvk,

I can't do anything at moment because i've no internet connection nor pc. I'm really sorry about that, but i think there is to wait 2 or 3 weeks more.

andreyvk commented 7 years ago

@Ostico cool. I'll hang on until that. Thanks!

andreyvk commented 6 years ago

@Ostico, hi. Are you back now? Can you please take a look at the issue?

Ostico commented 6 years ago

Hi @andreyvk , @sameer-shelavale i tried to reproduce this bug again and i've not found any issue, it works fine for me. @andreyvk , i tried your code on OrientDB version 2.2.19 and 2.2.25 with PHP 7.0.22 and i got this result:

Array
(
    [0] => PhpOrient\Protocols\Binary\Data\Record Object
        (
            [rid:protected] => PhpOrient\Protocols\Binary\Data\ID Object
                (
                    [cluster] => 17
                    [position] => 0
                )

            [oClass:protected] => Test
            [version:protected] => 1
            [oData:protected] => Array
                (
                    [dated] => DateTime Object
                        (
                            [date] => 1969-12-30 23:00:00.000000
                            [timezone_type] => 3
                            [timezone] => UTC
                        )

                )

        )

)
Ostico commented 6 years ago

@andreyvk regarding this: https://github.com/Ostico/PhpOrient/issues/91#issuecomment-313374072

i have to check better how the dates before the unix epoch are returned by OrientDB because i can use your suggested implementation only for 64bit platforms.

Anyway, for me this implementation works as is... I never got this issue. Are you sure that the field is DateTime in OrientDB?

Even if your software permit the wrong insertion in the database, OrientDB should not accept this kind of wrong dates.

andreyvk commented 6 years ago

@Ostico, orientdb cannot and should not prohibit storing any dates technically. what if database is about some historical values, for instance? dates can be old, right? :)

I am not sure abt 32-bit systems. But all i know is that PHP 5.5 has the issue and it's valid (and that is the version we are currently running on production server). You can try that on http://phptester.net by setting PHP version to 5.5 and running this code there:

$datetime = \DateTime::createFromFormat('U', "-1");
var_dump($datetime);

Also me and the other guy too are sure that Orient has problems with dates below 1970 and the field is DATETIME or DATE. That's a valid issue.

Ostico commented 6 years ago

Ok, i will try with an older php version

sameer-shelavale commented 6 years ago

@Ostico I have windows 7 64bit and PHP 5.5.33 and this issue is still haunts me. The solution by @andreyvk of using setTimestamp() instead of createFromFormat() works.

I have date of birth of a user as 1953-04-11 00:00:00 which translates to $collected = "-527853600000"; on phporient/src/PhpOrient/Protocols/Binary/Serialization/CSV.php line 240 but createFromFormat() returns false for negative timestamps.

Ostico commented 6 years ago

@andreyvk i checked, this fails until php 5.6.

@sameer-shelavale the question for me is not the DateTime method used to build the correct date but the implementation needed to handle the timestamps:

//instead of substr( $collected, 0, -3 );
$ts = (int) ($collected / 1000); 

The explicit cast to integer doesn't works on 32bit if the timestamp is lower than -2147483648 so it's better to use substr, it works for all PHP versions.

Moreover this will not work on 32bit systems:

$collected->setTimestamp($ts);

because of the internal limitation of integers, setTimestamp will fail also.

I must to do a double implementation by checking the PHP version.

andreyvk commented 6 years ago

@Ostico, you can probably use the following approach (need to test on all PHP versions though, but i think it works):

$time = '-12345';
$datetime = new DateTime("@$time");

However, the use of substr( $collected, 0, -3 ) in your code might return false (or null ?), if $collected is less than 3 characters long. Also, if $collected, for example, is "-100", then substr will return "-". In which case this DateTime("@$time") fails. So, maybe smth like this will work:

$collected = substr( $collected, 0, -3 );
if(empty($collected) || $collected === "-")
    $collected = "0";
$datetime = new DateTime("@$collected");

How about that?

sameer-shelavale commented 6 years ago

@Ostico I think casting to int is not really necessary. I tried $collected->setTimestamp( '-527853600' ); and it works well.