xAPI-vle / moodle-logstore_xapi

A Moodle plugin to send xAPI statements to an LRS using events in the Moodle logstore.
GNU General Public License v3.0
75 stars 85 forks source link

The mere installation presence of this plugin messes up the moodle timezone #802

Open brendanheywood opened 2 years ago

brendanheywood commented 2 years ago

Description

https://github.com/xAPI-vle/moodle-logstore_xapi/blob/master/src/transformer/utils/create_timestamp.php#L20

Version

Steps to reproduce the bug

  1. During the moodle boostrap which ends up loading this page it sets the timezone which is then incorrect for part or all of the remainder of the request. Under some conditions it will be corrected later on. Having the timezone different in different parts of the request creates some very odd edge cases bugs in other places.

Expected behaviour

Actual behaviour

Server information

Client information

davidpesce commented 1 year ago

I can't seem to find a relevant reason as to why the time is hard coded. If it had been set to UTC that'd make a bit more sense. Looking at the logs over the last couple of years, the activity timestamp is in fact UTC+1 (UK time).

This raises the question that without a requirement from the xAPI spec, what is the preferred method?

The next question is, if changing this causes issues with already stored statements?

@ryansmith94 @garemoko @brendanheywood thoughts?

brendanheywood commented 1 year ago

Honestly don’t know and my interest is fairly low. The client who hit this wasn’t even using this plug-in it was just installed but idle. I would say it is kinda deprecated now that moodle core has its own xapi api. No matter what this plug-in shouldn’t touch Moodles settings.

davidpesce commented 1 year ago

@brendanheywood not to worry. This plugin is getting cleaned up to go into core. The current xapi implementation (in core) is the reverse of this in that it takes xAPI statements and turns them into events (while giving a round-about way to retrieve progress via Moodle - rather than an LRS).

davidpesce commented 1 year ago

There are rumblings that the xAPI 2.0 spec is going to require statements be in UTC, so I'm going to push forward with going that route.