Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a sub-issue of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* focused on correctly adding @param and @return type hinting to the Block module.
Documentation patches that include type hinting are time consuming to both review and commit because one must dig into the actual code to confirm that the type hints are both correct and complete. Hence, please be patient and try to limit type hint patches to covering only a limited number of docblocks (20-25 as a guess).
How To Review This Issue
- Attempt to apply the patch to see if it needs a reroll.
- Use the phpcs one-liner to evaluate whether all the relevant standards errors have been resolved: https://gist.github.com/paul-m/227822ac7723b0e90647
- Look at each change and determine whether the type hint is correct.
Related sprint issues:
Comment | File | Size | Author |
---|---|---|---|
#43 | 1807674-43.patch | 1.32 KB | shetpooja04 |
#32 | 1807674-28-add-missing-type-hinting.patch | 42.2 KB | karolus |
#28 | 1807674-27.patch | 1.35 KB | naveenvalecha |
| |||
#27 | interdiff-1807674-21-27.txt | 586 bytes | naveenvalecha |
#21 | 1807674-21.patch | 1.34 KB | naveenvalecha |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a patch that addresses missing type hinting in the block.api.php file.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedPostponing until #1807652: Further clean up of API docs for Block module is committed.
Comment #3
mgiffordunless there is another way around this.
Comment #4
Mile23Fixes all parameter hinting errors in docblocks that aren't in
block.api.php
.In fact,
block.api.php
doesn't have any such errors at this point.Comment #5
Mile23Comment #6
mgiffordApplies nicely. Makes sense to me to add this inline documentation.
Comment #7
jhodgdonUmm...
Is this optional? The docs look like it is, but it doesn't say (optional), and what is the default, still a string or NULL?
should be string[]?
Comment #8
Mile23Thanks for the review, @jhodgdon.
Fixed both things.
Comment #9
mgiffordNice catch. Anyone want to mark this RTBC? Seems like a fairly straight forward set of improvements to the inline docs.
Comment #10
Mile23@mgifford: You could mark it RTBC, if it still applies and is a good patch.
Comment #11
mgiffordIt still applies. It also seems to conform with other examples, but this isn't an area I'm confident about.
The closest documentation I could find on this was:
https://www.drupal.org/node/1354#functions
https://www.drupal.org/node/608152#hinting
Can you point me better documentation? Just realizing that to me this looks like an array string[] made me realize I've got to know more before marking it RTBC.
Comment #13
Mile23Still applies.
Comment #14
Mile23Still applies.
Comment #15
Mile23Comment #16
deepakaryan1988Re-rolling the patch of @Mile23 as there was few minor thing were there.
:)
Comment #17
yogen.prasad CreditAttribution: yogen.prasad commentedLooking fine
Comment #18
xjmIf it's an array, it's not a string. Perhaps this should be
string[]
? Can we check what the function expects by looking over its code?Comment #19
deepakaryan1988@xjm Agreed!!
Ahh I missed it. Thanks for pointing out.
Re-rolling the patch
Comment #20
Mile23Bumping to 8.1.x, no longer applies.
Comment #21
naveenvalechaAdmins-MacBook-Pro% phpcs --standard="Drupal" --extensions="module/php,php" --report-csv core/modules/block | grep -F $'Missing param\nReturn type missing'
Comment #22
madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commentedChanges looks good to me.
Comment #23
Mile23Nice, thanks.
phpcs reports no missing
@param
or@returns
hints. All the hints look correct.Also: Easy review. :-)
Comment #24
jhodgdonThere is no reason this patch, if corrected, could not be committed to 8.0.x.
However, I do not think it is quite correct:
I seriously doubt that this is an integer. Most config entity IDs are strings. Please check this again.
Is this really a string or an object? Please verify.
Comment #25
naveenvalechaRe:#24.1
I have checked the type of entity_id in entity_load and its mixed.
Re : #24.2
I have confirmed view_mode is a string.check EntityViewBuilderInterface viewMultiple function.
So probably we need a reroll for #24.1
Comment #26
jhodgdonRE #25 ...
Yes, in entity_load(), entity IDs are allowed to be mixed. However, in this class it is specifically a block entity, so its ID is a string. If it were a node or comment, its ID would be an integer, but this is a block config entity so it is a string.
Agreed on the view mode though that it does seem to be expecting a string like 'full' for this.
Comment #27
naveenvalecha#26.1
Agreed, the id is string Drupal\block\Entity\Block
changed the id data type to string.
Changing the status to RTBC as per #23 b/c the above change has been addressed.
phpcs reports no missing @param or @returns hints.
Comment #28
naveenvalechaComment #30
jhodgdonThanks! The changes that are in the patch, I believe, are correct now.
I have not marked this patch RTBC however... Please see instructions in the issue summary. Someone (not @naveenvalecha) needs to do step 2 under "How to review this issue" to verify that the Block module has been completely patched with @param/@return types before we can call this issue done. Thanks!
Comment #32
karolus CreditAttribution: karolus as a volunteer commentedI tested by following the steps for review, and ran automatic fixes via PHPCS into patch #28.
Comment #33
Mile23Setting 'needs review.'
Comment #34
jhodgdonSorry, but the patch in #32 has some problems and should not be committed, and includes some out-of-scope fixes. Please discard.
So. Once again: Please someone go back to the patch in #28 and verify that step #2 under "How to review" has passed. And upload the patch in #28 again.
Comment #43
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedI have uploaded patch with changes made in #28 in 8.9.x version
as suggested in #34
used the below command to check https://gist.github.com/paul-m/227822ac7723b0e90647
Please review !
Comment #44
quietone CreditAttribution: quietone as a volunteer commentedThis work is now being done by sniff. The work here is now in #3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType so closing this as a duplicate. I've identified credit to add over there, let me know if I got it wrong.