search

FMCorz / moodle-block_xp

A gamification plugin for Moodle allowing students to gain experience points and level up.
https://levelup.plus/?ref=github
150 stars 41 forks source link

Capture exceptions when context no longer exists #81

Closed polothy closed 6 years ago

polothy commented 6 years ago

This was actually discovered in an unrelated unit test (the course was deleted in the test), but seems like a good change since this subscribes to all events and could likely happen elsewhere.

FMCorz commented 6 years ago

Interesting. In what circumstances is Moodle broadcasting events that are no longer attached to a context? I wasn't aware that this had to be catered for. Is it documented anywhere? Thanks!

polothy commented 6 years ago

The unit test runs in a transaction. While a transaction is open, it appears that events are buffered. Durning the open transaction, the course is created and deleted. After the test is done, the transaction is closed and the buffered events are sent.

So, yes, unlikely to run into this in Moodle standard, but could happen. I patched it on our end, take it if you like or not. Just wanted to share the patch.

Is it documented anywhere?

Hah, good one 😉

FMCorz commented 6 years ago

Thanks for the details. It's interesting that the try/catch works because get_context() would return false, not throw an exception. Ultimately, because of false possibility, all code should always handle that possibility, it just feels very redundant.

I guess if the observer was internal, then this wouldn't happen as it would be notified as soon as the event occurs, not after the transaction. Which is fine, but seems unnecessary if the transaction is rolledback.

Thanks for spotting it!

polothy commented 6 years ago

Thanks for the details. It's interesting that the try/catch works because get_context() would return false, not throw an exception. Ultimately, because of false possibility, all code should always handle that possibility, it just feels very redundant.

It's because get_context actually returns the cached context object and it is actually has_capability that throws it because it tries to query for parent contexts.

An alternative fix would be to put the try/catch into your main entry point for all the observed events. Being unfamiliar with the code, wasn't sure if that was a good fix.

FMCorz commented 6 years ago

So I tried to reproduce this in a testcase but didn't succeed. Transactions are handled differently depending on the database engine, etc... so I gave up writing a testcase for it. However I wrote a commit which a) checks that get_context returns something, and b) captures the exception as you suggested.

Thanks! Now back to GDPR...

polothy commented 6 years ago

Thanks