-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH Add generic types #11108
ENH Add generic types #11108
Conversation
762822d
to
a8aa0b5
Compare
a8aa0b5
to
1957242
Compare
1957242
to
6797d8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot to peer review here so all of the peer review will take multiple peer review sessions to get through. I got about halfway through and got up to (but haven't reviewed) EagerLoadedList
6797d8b
to
186a4a6
Compare
* @return ArrayIterator | ||
*/ | ||
public function getIterator(): Traversable | ||
{ | ||
Deprecation::notice('5.2.0', 'Will be removed without equivalent functionality'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be deprecating things in the PR, it's very unrelated. Why are we deprecating this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not unrelated. I thought I had added a comment to the PR about this but it looks like I forgot to.
This is being deprecated because:
- It's not actually needed anymore - the
<% control %>
template block was removed in CMS 3, and that was the only reason this method is here at all. - having the
getIterator()
here causes problems with identifying the correct type forT
being iterated through fromgetIterator()
of subclasses in some IDEs. This was brought up in the original POC in this issue. One solution we considered was adding generic typing toViewableData
but that really is nonsensical outside of the context of lists, and mostViewableData
isn't a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Add it to the 5.2.0 changelog that it's being deprecated I suppose then.
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.
186a4a6
to
d4ab55b
Compare
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
|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()
andDataObject::get_one()
give weird results in some specific scenarios.DataObject:get(MyClass::class)
, it won't know about theMyClass
class 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 theAnotherClass
and 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.ArrayList
will wrap an associative array withArrayData
in 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 inArrayData
anyway because of the existing inconsistency with whatArrayList
returns.ArrayList
can 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@var
or 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.@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 returningDataObject
anyway so should be fine.Manual testing steps
Check that things work as expected in your IDE.
Issues
Pull request checklist