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
76 stars 84 forks source link

$event->id does not exist in multiple places. #234

Open caperneoignis opened 6 years ago

caperneoignis commented 6 years ago

Description When going through a quiz, with the logstore app set to real time transmission to the LRS. I get the following in several places.

Notice: Undefined property: stdClass::$id in {web_directory}\moodle\admin\tool\log\store\xapi\src\transformer\events\mod_quiz\attempt_viewed.php on line 37

And again at transformer\handler.php line 37

$eventobj->id is undefined. Version

Steps to reproduce the bug

  1. Have debugging turned on for moodle. Go and take a quiz, and look at the bottom, you will see several undefined warnings.

Expected behaviour

Actual behaviour

caperneoignis commented 6 years ago

We can not assume event will have an id, we need to handle this/assign this appropriately.

ryasmi commented 6 years ago

Yeah I'd recommend checking if it's there and setting it to null if not.

caperneoignis commented 6 years ago

Any opposition to using a ternary expression in those spots? And to use zero instead of null since null can put us back into the undefined boat depending on where that element ends up?

ryasmi commented 6 years ago

I can see four uses of the event's id.

  1. /src/transformer/events/mod_quiz/attempt_viewed.php#L37
  2. /src/loader/lrs.php#L70
  3. /src/transformer/handler.php#L46
  4. /classes/task/emit_task.php#L46

Let me know if there are any other uses. I think the first usage is actually meant to be $event->objectid rather than the $event->id. If that's the case, we should probably just avoid using the event id, except where it's absolutely needed like the 4th usage where the id will have to exist.

caperneoignis commented 6 years ago

That's agreeable.