Added parameter types for some methods and fixed a wrong return type of the __get() method
Comment | File | Size | Author |
---|---|---|---|
#4 | fix_parameter_type_and_return_type-2887080-4.patch | 2.13 KB | edaa |
#2 | fix_parameter_tyep_return_type-2887080.patch | 1.77 KB | edaa |
Added parameter types for some methods and fixed a wrong return type of the __get() method
Comment | File | Size | Author |
---|---|---|---|
#4 | fix_parameter_type_and_return_type-2887080-4.patch | 2.13 KB | edaa |
#2 | fix_parameter_tyep_return_type-2887080.patch | 1.77 KB | edaa |
Comments
Comment #2
edaa CreditAttribution: edaa commentedComment #3
FeyP CreditAttribution: FeyP as a volunteer and at werk21 commentedThanks for the patch, @edwardaa. It applies and is definitely a documentation improvement. I noticed, that the
@return
tag forstorageSettingsForm()
is still missing a type hint. Let's add that to the patch while we're at it.Comment #4
edaa CreditAttribution: edaa commentedThanks for you feedback, added return type of
array
forstorageSettingsForm()
Comment #5
FeyP CreditAttribution: FeyP as a volunteer and at werk21 commentedThanks, looks good. The patch still applies and it still is a documentation improvement. During the CI run I checked FieldItemBase and a few other random implementations to verify that the new type hints actually make sense. Marking as RTBC.
Comment #6
Wim LeersAre you sure this is correct?
Comment #7
edaa CreditAttribution: edaa commentedYes, the __get() method returns the value of typed data rather than the typed data itself.
I am not pretty sure there is any case where the $property_name is anything else except 'string'
Comment #9
xjmRegarding #6, I had the same initial reaction when I saw the
mixed
. However, this is the default implementation provided byFieldItemBase
:So, that code comment would seem to support the change in the patch. And
TypedData::getValue()
does indeed returnmixed
(depending on the type) rather than a classed object.Comment #12
xjmIt's worth noting that while most coding standards should be cleaned up to fix the rule once across all of core, for scalar data types especially, the code has to be read carefully to ensure the documented types are correct and complete. (As #6 - #9 show.) This is even mentioned specifically at the end of https://www.drupal.org/core/scope#examples for that reason.
However, we should also check the implementations' and callers' typehints and fix them in the same issue scope, since those need to be checked anyway to verify the correct typehints are being added. Since these are all magic methods, there's no callers to check, but there are implementations on at least
FieldItemBase
. I reviewedFieldItemBase
and confirmed that it does not add or override any documentation. So, the scope of this issue is sufficient and complete.Committed and pushed to 8.5.x and 8.4.x. Thanks!