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

  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 #1533208: Make contextual module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1332648: Clean up API docs for contextual module
#500866: [META] remove t() from assert message N.A.
CommentFileSizeAuthor
#4 1816764_4.patch4.27 KBmile23

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Closed (duplicate)

This is a subset of #1533208: Make contextual module pass Coder Review, which already has a patch. Hence merging into that issue.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
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
commented
Status: Closed (duplicate) » Active

This issue may be a subset of #1533208: Make contextual module pass Coder Review, but its limited scope makes it very achievable, reasonably easy to review, and it will help IDEs do autocomplete.

Marking as active.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 1816764_4.patch4.27 KB

Shazam.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Still applies.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community

I've double checked this. these look good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Committed e7fcbb2 and pushed to 8.0.x. Thanks!

+++ b/core/modules/contextual/src/Tests/ContextualDynamicContextTest.php
@@ -134,8 +137,6 @@ function testDifferentPermissions() {
-   *
-   * @return bool
    */
   protected function assertContextualLinkPlaceHolder($id) {

@@ -146,8 +147,6 @@ protected function assertContextualLinkPlaceHolder($id) {
-   *
-   * @return bool
    */
   protected function assertNoContextualLinkPlaceHolder($id) {

Assertions should return a bool - these methods just need to return the result of their assertions.

martin107’s picture

martin107 commented
Assigned: Unassigned » martin107

Opps and yes ... fixing this.

martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned
Status: Needs work » Fixed
Related issues: +#2453931: ContextualDynamicContextTest follow up

Sorry for the delay ... it has taken sometime to float to the top of my todo list.

Here is the follow up

Status: Fixed » Closed (fixed)

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