Adds generic types (mostly return types - we don't need generic param typing for now since we're not really using static analysis).
There are also a few general corrections to PHPDocs that I noticed along the way (e.g. adding |null when the method is returning a null value.
There are some cases where either the return type or the whole PHPDoc was duplicated from the parent class - in those cases I've simply removed the duplication.
Caveats
DataObject::get() and DataObject::get_one() give weird results in some specific scenarios.
If using DataObject:get(MyClass::class), it won't know about the MyClass class and just give you a DataList<DataObject> - which is correct, but imprecise. Same with get_one() with the same syntax.
If using MyClass::get(AnotherClass::class), it won't know about the AnotherClass and will give you a DataList<MyClass> - which is NOT correct. I think this is okay since doing that is a bad code smell anyway - people should just not do that. Probably in a future release we should explicitly disallow that syntax. Same with get_one() with the same syntax.
We could use a conditional return type here which would be useful for static analysis, but that's not supported in VSCode using intellephense so we shouldn't do that.
Because ArrayList will wrap an associative array with ArrayData in its iterator (but not in First(), Last(), etc), the typing for the iterator is wrong if the arraylist was created with associative arrays. Ideally people should be wrapping their associative arrays in ArrayData anyway because of the existing inconsistency with what ArrayList returns.
Because ArrayList can accept values of mixed types (you could have a combination of ArrayData, DataObject, and some custom class for example), the return values might not always be reported correctly, especially if adding objects to the list after instantiation. This is acceptable since that sort of usage is less common - and they'd already have to use @var or similar in their IDE before this PR.
When passing a service name that isn't a direct FQCN to the injector (e.g. Injector::inst()->get(LoggerInterface::class . '.error');), there is no way for an IDE to determine the correct class name. In the future we may want to use conditional return types to try to resolve this sort of thing, but as of writing those aren't as widely supported in IDEs as non-conditional generic return types are.
For the extension generics to really be useful, subclasses will need to use the appropriate @extends annotation. I've raised a separate card to add a CI rule for this so we can at least be consistent with it in supported modules.
SapphireTest::objFromFixture() can technically take a table name as the first argument.... but that's probably never done in practice. I'm 99% sure in that case most IDEs would correctly still recognise that it's returning DataObject anyway so should be fine.
[x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
Description
Adds generic types (mostly return types - we don't need generic param typing for now since we're not really using static analysis).
There are also a few general corrections to PHPDocs that I noticed along the way (e.g. adding
|nullwhen the method is returning a null value.There are some cases where either the return type or the whole PHPDoc was duplicated from the parent class - in those cases I've simply removed the duplication.
Caveats
DataObject::get()andDataObject::get_one()give weird results in some specific scenarios.DataObject:get(MyClass::class), it won't know about theMyClassclass and just give you aDataList<DataObject>- which is correct, but imprecise. Same withget_one()with the same syntax.MyClass::get(AnotherClass::class), it won't know about theAnotherClassand will give you aDataList<MyClass>- which is NOT correct. I think this is okay since doing that is a bad code smell anyway - people should just not do that. Probably in a future release we should explicitly disallow that syntax. Same withget_one()with the same syntax.ArrayListwill wrap an associative array withArrayDatain its iterator (but not inFirst(),Last(), etc), the typing for the iterator is wrong if the arraylist was created with associative arrays. Ideally people should be wrapping their associative arrays inArrayDataanyway because of the existing inconsistency with whatArrayListreturns.ArrayListcan accept values of mixed types (you could have a combination ofArrayData,DataObject, and some custom class for example), the return values might not always be reported correctly, especially if adding objects to the list after instantiation. This is acceptable since that sort of usage is less common - and they'd already have to use@varor similar in their IDE before this PR.Injector::inst()->get(LoggerInterface::class . '.error');), there is no way for an IDE to determine the correct class name. In the future we may want to use conditional return types to try to resolve this sort of thing, but as of writing those aren't as widely supported in IDEs as non-conditional generic return types are.@extendsannotation. I've raised a separate card to add a CI rule for this so we can at least be consistent with it in supported modules.SapphireTest::objFromFixture()can technically take a table name as the first argument.... but that's probably never done in practice. I'm 99% sure in that case most IDEs would correctly still recognise that it's returningDataObjectanyway so should be fine.Manual testing steps
Check that things work as expected in your IDE.
Issues
Pull request checklist