A bit about constructors
While exploring the admin/addons and PhpGit issues, I came across another thing I thought worth mentioning.
We have a number of classes that return from their constructors. Some try to return values (typically
return null or similar,) but most of the time it's just a plain
return statement.
However, the values returned by a constructor will not be passed on to the code that instantiates the object. Consider the following code:
class Foo {
function __construct() {
return null;
}
}
$foo = new Foo();
var_dump($foo);
It's tempting to think that this would print
NULL, but instead it prints:
object(Foo)#1 (0) {
}
Iow,
$foo contains an object of type
Foo, regardless of the value returned from the constructor.
Why is this a problem?Imo constructors should either succeed or fail. There's no way for the code instantiating the object to know whether initialization was successful, partially successful or just plain failed. And continuing with an object of unknown state could cause problems we don't want to deal with.
So what to do instead?The (again imo) correct way to handle a failed object initialization is by throwing an exception. This ensures that the execution flow is broken and we don't risk continuing with an uninitialized or partially initialized object.
We may then choose whether to handle the exception, or let it terminate execution of the program/request. This is typically where we would return a status 500 to the client.
Which files are affected?I wrote
a simple PHPMD rule to find how many cases we have of this issue. The following files contain
return statements in the constructor:
- Zotlabs/Lib/ASCollection.php
- Zotlabs/Lib/ActivityStreams.php
- Zotlabs/Lib/DB_Upgrade.php
- Zotlabs/Lib/JcsEddsa2022.php
- Zotlabs/Lib/Multibase.php
- Zotlabs/Lib/Share.php
- Zotlabs/Storage/GitRepo.php
- Zotlabs/Web/SubModule.php
- Zotlabs/Zot6/Receiver.php
- include/dba/dba_driver.php
I haven't looked in detail at these files (apart from
GitRepo.php) yet, but will try to move them towards using exceptions instead.
I'm also thinking it could be a good idea to include the PHPMD rule in the core repo somewhere, so we can use it to ensure this issue don't come up again.
