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 menu_ui 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

  1. Attempt to apply the patch to see if it needs a reroll.
  2. Use the phpcs one-liner to evaluate whether all the relevant standards errors have been resolved: https://gist.github.com/paul-m/227822ac7723b0e90647
  3. Look at each change and determine whether the type hint is correct.

Related sprint issues:

Sprint Topic Sub Issue
#1518116: [meta] Make Core pass Coder Review #1533252: Make menu module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1326672: Clean up API docs for menu module
#500866: [META] remove t() from assert message #1797366: Remove t() from assertion messages in tests for the menu module

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improves documentation.
CommentFileSizeAuthor
#5 1800774_5.patch1.41 KBMile23
#1 1800774-1-th-menu-api.patch1.51 KBLars Toomre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Active » Needs review
FileSize
1800774-1-th-menu-api.patch1.51 KB

Attached is a small patch that adds type hinting to the hook examples in menu.api.php. This is untested locally, but should pass since all of the changes are in comments. Let's see what the bot thinks.

alansaviolobo queued 1: 1800774-1-th-menu-api.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 1800774-1-th-menu-api.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Parent issue: » #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Title: Add missing type hinting to Menu module docblocks » Add missing type hinting to menu_ui module docblocks
Component: menu.module » documentation
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1800774_5.patch1.41 KB

Since there's no such thing as the menu module any more, I'll hijack this for menu_ui module.

It only changes docblock type hinting, so it's a documentation issue.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Patch still applies, phpcs says no missing hints.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Still applies.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
ashutoshsngh’s picture

ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commented
Status: Needs review » Reviewed & tested by the community

Patch applies well moving to rtbc.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

@ashutoshsngh, the testbot checks that the patch applies for us. What we should do is confirm that the changes in each patch are correct.

I carefully read the code in MenuForm to confirm that the parameter typehints there were correct. Thanks @Mile23! This patch contains only documentation fixes, so can be accepted any time during the beta. Committed and pushed to 8.0.x

  • xjm committed 0fa10c6 on 8.0.x 
    Issue #1800774 by Mile23, Lars Toomre: Add missing type hinting to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.