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 Comment 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 (10-15 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:
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Novice | Instructions | Done |
Manually check type hints validity | Novice | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff-1808478-61-69.txt | 706 bytes | nikunjkotecha |
#69 | 1808478-69.patch | 2.84 KB | nikunjkotecha |
#61 | interdiff-1808478-59-61.txt | 1.23 KB | naveenvalecha |
#61 | 1808478-61.patch | 2.77 KB | naveenvalecha |
#59 | 1808478-59.patch | 2.71 KB | naveenvalecha |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a patch that adds type hinting to the hooks in comment.api.php file.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedComment #3
larowlanLars, can this be held off till after feature freeze?
I don't want to have to reroll #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Thanks
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commented@larowlan I do want to have you re-roll #731724: Convert comment settings into a field to make them work with CMI and non-node entities.
As you might notice, the initial patch in #1 in quite small. Perhaps you might just include those changes in your work before you next roll a patch from your sandbox? They are simply type hinting additions to comment.api.php file.
I will hold off on further patches about type hinting in the Comment module until after feature freeze.
Comment #5
larowlanThanks, will do.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedPostponed until after feature freeze (Dec. 1 2012) for #3.
Comment #7
roderikUnpostponing. Let's check whether this needs reroll now.
As the issue summary says: re-checking that type hints are correct can be tiring, but this one is small enough.
Comment #8
roderikComment #9
dankh CreditAttribution: dankh commentedI working on it
Comment #10
dankh CreditAttribution: dankh commentedPatch needs to be re-rolled.
I'll re-roll the patch.
Comment #11
dankh CreditAttribution: dankh commentedAll the documentation fixes included in this patch are no longer needed.
The hooks are no longer part of core/modules/comment/comment.api.php . I have checked that they were not moved elsewhere as is, in order to apply the patch there.
I have also checked that the docblock comments for hook_comment_links_alter (the only hook in this file) are correct.
Comment #12
roderikThanks for checking this, dankh.
Verified #11, as we did on the sprint.
It seems commit # 5ffb1d3 / issue #2216535: Replace Node overview topic and Node API topic with Entity Hooks topic moves all that stuff to core/system/entity.api.php - and I'm sure the type hinting was done correctly in entity.api.php when that documentation was added.
So, one less open issue :)
Comment #13
Mile23Missed a few.
Re-opening here since it's already under that nice [meta].
Comment #14
Mile23Comment #15
llbbl CreditAttribution: llbbl commentedWorking on reviewing this.
Comment #16
llbbl CreditAttribution: llbbl commentedThis needs to use Overrides instead of @param because there is a namespace at the top of the file.
Missing a return
Missing a return
Attached is a new patch files with a few additional changes listed above.
Comment #18
llbbl CreditAttribution: llbbl commentedScrewed up the patch file. Oops! Lets try this again.
This needs to use Overrides instead of @param because there is a namespace at the top of the file.
Missing a return
Missing a return
Attached is a new patch files with a few additional changes listed above.
Comment #19
rgristroph CreditAttribution: rgristroph commentedI looked over @llbbl's patch in #18 , applied and tested commenting and also ran the tests that were affected. I looked through the files that were affected for any missed stuff in the docblocks, I didn't see any. Looks good to me !
Comment #20
Mile23Setting to needs review so the testbot works on #18.
Comment #21
Mile23Found a bunch more with the magic of NetBeansDrupalComposed. :-)
Comment #23
Mile23Addressed the errors in the test, plus a few more. Let's see if the testbot finds any I didn't....
Comment #24
jhodgdonChanging component. This is changing function signatures, not just documentation.
Comment #25
jhodgdonOh and the patch needs work for CommentForm at least:
Oops. @param was removed.
Comment #26
Mile23Fixed that one @param in CommentForm.
If nothing happens to this in the next week or so I'll take out the method signature changes.
Comment #27
Mile23Needs reroll.
Comment #28
mrjmd CreditAttribution: mrjmd commentedReroll attached.
Comment #29
mrjmd CreditAttribution: mrjmd commentedComment #30
Mile23Still applies.
Comment #31
Mile23Comment #32
deepakaryan1988Re-rolling the patch#28 with some modification.
Comment #33
yogen.prasad CreditAttribution: yogen.prasad commentedLooks fine
Comment #34
xjmThis patch includes changes that are out of scope. Please only add typehints to function docblocks, and then reviewers should carefully review the affected functions to confirm that the typehints are correct for all usages. Thanks!
Comment #35
mlevasseur CreditAttribution: mlevasseur at Acro Commerce commentedReroll...
Removed: LinkDelete.php, LinkEdit.php, Link.php
Conflicts: comment.module (fixed a typo for one @param: "sting" -> "string"), LinkApprove.php, LinkReply.php
Comment #36
jgeryk CreditAttribution: jgeryk commentedApplies cleanly at commit 2e66338
Comment #37
deepakaryan1988Comment #39
Mile23'e.g.' means 'for example,' and tells us that a sample list follows. There's no need for either 'etc.' or ellipsis ('...').
So like this:
(optional) View mode; e.g. 'full', 'teaser'. Defaults to 'full'.
Comment #40
prhavile CreditAttribution: prhavile as a volunteer commentedPatch Rerolled.
Comment #41
prhavile CreditAttribution: prhavile as a volunteer commentedapologies. wrong file got uploaded.
Comment #42
prhavile CreditAttribution: prhavile as a volunteer commentedCorrect file uploaded which have the correct comment.
Comment #43
prhavile CreditAttribution: prhavile as a volunteer commentedThere were two occurrences of "etc", removed both of them.
Comment #44
hctomI will start reviewing this and reroll the patch if needed
Comment #45
hctomSo.. I finally got managed to reroll the patch. Attached are the new files (rerolled patch and its interdiff). This is the corresponding rebase information containing auto merges:
Please give the new patch a review if it applies now.
(Sorry for the wrong comment number in the patch filename... should have been 45 instead of 44)
Comment #46
hctomHid missed outdated interdiff file...
Comment #47
hctomComment #48
stBorchertPatch applies without any warnings. Manually check for type hints validity can be started ;)
Comment #49
hctomokay... i will take care of this now
Comment #52
hctomHad to queue a re-test of the patch due to a testbot error (
GET http://ec2-52-11-109-200.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes).
)Comment #53
hctomPhew... now the testbot was succesful :)
I applied the patch from comment #45 to the latest HEAD and ran the following command:
phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme --report-csv /path/to/drupal/core/modules/comment | grep -F $'Missing param\nReturn type missing'
No errors or warnings have been displayed, so this should be fixed for now!
Thanx to all the contributors
Comment #54
alexpottThese changes look incorrect
Also I think this should be limited to fixing missing type hinting and not moving use statements.
Comment #55
no_angel CreditAttribution: no_angel as a volunteer commentedComment #57
no_angel CreditAttribution: no_angel as a volunteer commentedComment #58
Mile23Bumping to 8.1.x. Patch no longer applies.
Comment #59
naveenvalechaComment #60
Mile23Nice, thanks. :-)
We always prefer to have an array of type rather than just array as the hint. This is an array of
CommentInterface
so it should be:@param \Drupal\comment\CommentInterface[] $comments
Should be
\Drupal\Core\Form\FormStateInterface
, instead of array.Comment #61
naveenvalechaComment #62
deepakaryan1988Comment #63
Mile23Great, thanks.
Patch applies, addresses all the concerns in #60, phpcs says no errors.
Comment #66
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #67
catchShouldn't this also have the CommentInterface type hint?
Comment #68
catchShouldn't this also have the CommentInterface type hint?
Comment #69
nikunjkotechaUpdating patch as per the comments #60 and #67
Comment #70
nikunjkotechaComment #71
lokesh.soni CreditAttribution: lokesh.soni commentedComment #72
lokesh.soni CreditAttribution: lokesh.soni as a volunteer and commentedComment #73
Wim LeersComment #75
catchCommitted/pushed to 8.1.x, thanks!