getTimestamp() issue in src/Util.php on PHP Version 5.5.9-1ubuntu4.4

[lewyckoff]

For some reason this function:

    public static function getTimestamp() {
        $time = microtime(true);
        $microseconds = sprintf('%06d', ($time - floor($time)) * 1000000);
        $millseconds = round($microseconds, -3)/1000;
        $millsecondsStr = str_pad($millseconds, 3, '0', STR_PAD_LEFT);
        $date = (new \DateTime())->format('c');

        $position = strrpos($date, '+');
        $date = substr($date,0,$position).'.'.$millsecondsStr.substr($date,$position);

        return $date;
    }

is resulting in timestamps that look like this:
.7162016-08-02T17:39:30-04:00

As a result my submissions to Watershed LRS are going into the error logs like this:
Object with name 'statement' could not be created from the provided JSON. Error message: Error encountered while unpacking object (".7722016-08-01T13:01:57-04:00" of type java.lang.String) from JSON at line 1, column 274, near: ... imestamp":".7722016-08-01T13:01:57-04:00 ... Archive

I'm using this library inside of Opigno LMS (the latest stable version)
running PHP Version 5.5.9-1ubuntu4.4 (I also just upgraded to PHP Version 5.6.23 but the timestamp still looks the same)
and submitting to Watershed LRS to test.

I'm not seeing any reports anywhere else (even on the Opigno LMS forum or after asking support) about this issue.

[garemoko]

@lewyckoff I can see what's happening here. This function only works with timezones that are UTC0 or UTC+, not minus. Anywhere with a server timezone west of Greenwich is going to run into problems.

$position = strrpos($date, '+');
$date = substr($date,0,$position).'.'.$millsecondsStr.substr($date,$position);

Keeping in the spirit of the current approach, something like the following should work:

$position = strrpos($date, '+');
if ($position === false) {
    $position = strrpos($date, '-');
}
$date = substr($date,0,$position).'.'.$millsecondsStr.substr($date,$position);

Really though, the best approach is to convert the timezone to UTC 0, as using UTC 0 is recommended by the xAPI spec. See http://php.net/manual/en/datetime.settimezone.php

Something like this should work:

 $date = (new \DateTime(null, new \DateTimeZone("UTC")))->format('c');

I've not tested either of these snippets; please let me know how you get on!

EDIT: added missing closing parentheses

[lewyckoff]

Thank you!

Both solutions worked:

$position = strrpos($date, '+');
if ($position === false) {
    $position = strrpos($date, '-');
}
$date = substr($date,0,$position).'.'.$millsecondsStr.substr($date,$position);

I got the UTC 0 approach working with 1 extra closing parentheses:

$date = (new \DateTime(null, new \DateTimeZone("UTC")))->format('c');

[lewyckoff]

Do you know if there are other spots that I may need to change that line as well?

In Document.php I see this:

            if ($value instanceof \DateTime) {
                // Use format('c') instead of format(\DateTime::ISO8601) due to bug in format(\DateTime::ISO8601) that generates an invalid timestamp.
                $value = $value->format('c');
            }

In Opigno LMS I'm noticing that in their Opigno Statistics > LRS pages that I'm still not able to pull data from Watershed LRS (although now I am able to send data to Watershed LRS so there's definitely data in there to pull). That's mostly why I'm wondering.

[garemoko]

@lewyckoff it might be useful to set the timezone in both Document.php and StatementBase.php as well, and translate the input to UTC. This prevents people from using a different timezone both accidentally and deliberately, which is either a good thing or a bad thing depending on your views on how much of a Nanny you think the library should be. @brianjmiller any thoughts there?

Certainly the getTimestamp function should always use UTC 0 though. Do you fancy making a Pull Request @lewyckoff?

[WillSkates]

@lewyckoff @garemoko Should the "instanceof \DateTime" check be checking against \DateTimeInterface instead?

[garemoko]

@WillSkates maybe, I'd have to check, but that's a separate issue.

[WillSkates]

@garemoko : Agreed, sorry for bringing that up here.

[brianjmiller]

Will be in the next minor version, this can't be a patch as people might be counting on (even if they shouldn't) the timestamps being non-UTC. I don't consider it a major version breaking change though as they are requesting the default and it shouldn't be breaking any proper handling of returned statements from the LRS.