Destructor methods that access global variables can cause crashes

[Aaron Wells]

See : https://mahara.org/interaction/forum/topic.php?id=7397

Here's the problem (which appears to be intermittent):

1. ArtefactType has a destructor method, which gets called when an artefact object is garbage-collected by PHP.
2. PHP runs destructor methods and garbage collects variables in no guaranteed order when exit() is called.
3. Sometimes the ArtefactType destructor method tries to call DML methods, which use the global variable $db
4. Apparently sometimes the $db global variable has already been garbage collected by the time the destructor runs
5. This causes a fatal "method on a non-object" crash.

It's unclear why this error has only started happening recently. It might be due to a change in the behavior of PHP's garbage collector, or it might be from new Mahara code leaving some artefacts with their $dirty flag set (which triggers that artefact commit).

In either case, both global variables and implicit destructor methods are considered harmful design practices, in part because of this particular thing. So it's high time we got rid of these __destruct() methods.

Alternatively, as a workaround for older Mahara versions, we could add some code to the top of each __destruct() method that re-creates the global $db if it's not set.

[Aaron Wells (u-aaronw)]

Okay, for blog posts in particular, I've found the specific new code that's causing it. I've filed a separate bug 1513716 to take care of that.

With that known issue out of the way, this greater issue becomes Medium priority because it's not known to cause any problems, it just has the major potential to cause problems.

[Aaron Wells (u-aaronw)]

The Behat test suite ran through successfully with no errors with the destructors commented out.

Although, I guess what I should do, is run it with the destructors set to throw an error if there is uncommitted changes.

[Aaron Wells (u-aaronw)]

I ran the Behat tests again, with the destructors changed to throw an exception if there were uncommitted changes.

No problems at all. So, I think we should be fairly safe getting rid of them.

However, rather than just deleting them, I'm first going to look into whether there's a safe way to make them log a warning or something if there are uncommitted changes. The tricky part there is that I'll need to see what kinds of things are safe to do during which states of garbage collection. For instance, I can't use the standard mahara log_warn() message or anything like that, because it depends on Mahara's global $SESSION variable, which may no longer be present by that point.

[Kristina Hoeppner (kris-hoeppner)]

At the moment, we have some work arounds in place, and removing the destructors could cause problems. We'd need to be careful when we tackle that.