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. -- You received this bug notification because you are a member of Mahara Contributors, which is subscribed to Mahara. Matching subscriptions: Subscription for all Mahara Contributors -- please ask on #mahara-dev or mahara.org forum before editing or unsubscribing it! https://bugs.launchpad.net/bugs/1513710 Title: Destructor methods that access global variables can cause crashes Status in Mahara: Confirmed Status in Mahara 15.04 series: Confirmed Status in Mahara 15.10 series: Confirmed Status in Mahara 16.04 series: Confirmed Bug description: 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. To manage notifications about this bug go to: https://bugs.launchpad.net/mahara/+bug/1513710/+subscriptions _______________________________________________ Mailing list: https://launchpad.net/~mahara-contributors Post to : mahara-contributors@lists.launchpad.net Unsubscribe : https://launchpad.net/~mahara-contributors More help : https://help.launchpad.net/ListHelp
