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 Action 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:
Comment | File | Size | Author |
---|---|---|---|
#29 | 1802070_29.patch | 883 bytes | Mile23 |
#24 | add_missing_type-1802070-24.patch | 1.62 KB | alvar0hurtad0 |
#22 | add_missing_type-1802070-22.patch | 5.14 KB | alvar0hurtad0 |
| |||
#17 | 1802070_17.patch | 2.33 KB | Mile23 |
| |||
#14 | 1802070_14.patch | 1.62 KB | Mile23 |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedMarking this postponed until #1787900: Clean up cruft in the Action documentation is committed.
Comment #1.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #1.1
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedSetting this issue to active now, since there now is a report in #1816682-3: Make Action module pass Coder Review that aims to correct the coding style in the rest of the Action module.
Comment #2.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #3
baisongI was pointed to this issue in core mentoring hours.
I added as many type hints as I could following these guidelines, and corrected 1 punctuation error (missing period).
Patch attached.
Comment #4
Mile23Reroll.
The only problematic docblock is in
EmailAction::_construct()
. It's missing the parameter name for$mail_manager
.I say this rather than change it so I can do the review later.
Comment #5
Mile23Comment #6
Mile23Caught the problems in #4, with a few others.
Comment #7
Mile23Comment #8
deepakaryan1988Re-rolling the patch of @mile23 .
Comment #9
yogen.prasad CreditAttribution: yogen.prasad commentedLooking fine
Comment #10
deepakaryan1988Comment #11
deepakaryan1988Comment #12
xjmThanks @yogen.prasad and @deepakaryan1988. Did you carefully review each function to ensure that the typehints were correct in all cases?
These patches should only add missing type hinting, not do any other cleanups. The scope creep of adding other changes do these patches makes them more difficult to review. Long reference as to why: http://webchick.net/please-stop-eating-baby-kittens
This is not the correct docblock format, but it doesn't belong in this patch anyway. :) See above.
Comment #13
deepakaryan1988Ahh @xjm Thanks for pointing out..
will keep in mind in future!
Comment #14
Mile23I re-did the work rather than try to modify #8.
A couple of small out-of-scope items are still in there, because they modify the same file as in-scope issues.
There are instructions for reviewing this issue in the issue summary.
Comment #15
dawehnerDon't we know the action ID is a string (or maybe an integer?)
Comment #16
Mile23I couldn't find it. The example feeds it to a DB function requiring mixed.
Comment #17
Mile23Reroll of #14.
Still not sure about the type of $aid, because it's not documented anywhere.
Comment #18
naveenvalechaI suspect its integer
Comment #20
naveenvalechaComment #22
alvar0hurtad0here's the reroll of #17
Comment #23
alvar0hurtad0I think, my patch removes to much things, I'm reviewing it.
Comment #24
alvar0hurtad0Sorry for my previous patch this is the reroll of #17
Comment #26
Mile23In IRC w/ @chx we discussed that
action.api.php
is a) wrong, and b) isn't needed because we already documenthook_ENTITY_TYPE_delete()
.If
action.api.php
had any action module specific information it would be worth salvaging but it doesn't.Let's revert the changes to
action.api.php
in this patch, and then have a follow-up about deleting the file in another issue.Comment #27
Mile23The follow-up #2663830: Remove action.api.php
Comment #28
xjmAgain, please remove the out-of-scope changes. See: https://www.drupal.org/core/scope#merge
Comment #29
Mile23Needed a reroll anyway. Removes out of scope changes.
Changing to 8.0.x because that's what we've done on the other similar child issues. The patch applies cleanly to 8.0.x and 8.1.x.
Comment #32
naveenvalechaThis has been fixed as part of #2721901: Fix Drupal.Commenting.FunctionComment.MissingParamName