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 Taxonomy 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 #1533402: Make taxonomy module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1326644: Clean up API docs for taxonomy module
#500866: [META] remove t() from assert message #1195254: Taxonomy test cleanup
CommentFileSizeAuthor
#34 1807002-34.patch7.35 KBidebr
#34 interdiff-26-34.txt13.08 KBidebr
#26 interdiff-24-26.txt6.45 KBwillzyx
#26 add_missing_type-1807002-26.patch15.39 KBwillzyx
#24 add_missing_type-1807002-24.patch9.39 KBwillzyx
#24 interdiff-22-24.txt860 byteswillzyx
#22 add_missing_type-1807002-22.patch8.45 KBsiva_epari
#18 interdiff.txt658 bytesmrjmd
#18 taxonomy_type_hints-1807002-18.patch9.94 KBmrjmd
#16 interdiff.txt6.45 KBmrjmd
#16 taxonomy_type_hints-1807002-16.patch9.29 KBmrjmd
#14 taxonomy_type_hints-1807002-14.patch3.32 KBadci_contributor
#10 taxonomy_type_hints-1807002-10.patch3.57 KBJeroen
#7 drupal-taxonomy-hinting-1807002-7.patch1.12 KBseanr
#3 1807002-1-th-taxonomy-api.patch1.53 KBseanr
#1 1807002-1-th-taxonomy-api.patch1.53 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
1807002-1-th-taxonomy-api.patch1.53 KB

Here is a small patch that adds missing type hinting to the hook examples in taxonomy.api.php file.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Component: node.module » taxonomy.module

Setting to the right component.

seanr’s picture

seanr
he/him
CreditAttribution: seanr commented
FileSize
1807002-1-th-taxonomy-api.patch1.53 KB

One of them had the $ in the wrong spot. See attached.

Status: Needs review » Needs work

The last submitted patch, 1807002-1-th-taxonomy-api.patch, failed testing.

seanr’s picture

seanr
he/him
CreditAttribution: seanr commented

Ouch, didn't realize how old this was - that file has completely changed now. Not sure if this is even relevant anymore.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented

It would be great if you went through this module now and added the type hints as appropriate. This issue was effectively created and then postponed until after API freeze to bring the docblocks up to snuff from a type hint perspective. I will review any patch you put forward @seanr.

seanr’s picture

seanr
he/him
CreditAttribution: seanr commented
Status: Needs work » Needs review
FileSize
drupal-taxonomy-hinting-1807002-7.patch1.12 KB

Only found three that needed it. This patch should cover it now.

Jalandhar’s picture

Jalandhar CreditAttribution: Jalandhar commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies. It needs reroll.

Jeroen’s picture

Jeroen CreditAttribution: Jeroen commented
Assigned: Unassigned » Jeroen
Issue tags: +DUGBE1409
Jeroen’s picture

Jeroen CreditAttribution: Jeroen commented
FileSize
taxonomy_type_hints-1807002-10.patch3.57 KB

There's no more taxonomy.api.php file. I'm assuming Drupal 8 merged nodes and taxonomy terms into entities. The reroll isn't possible.

I did add some type hinting and moved some parameters from their class-type to the interface they implement.

Tests seem to still work.

Jeroen’s picture

Jeroen CreditAttribution: Jeroen commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
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
Status: Needs review » Needs work
Issue tags: +Needs reroll

Unsurprisingly, needs a reroll. :-)

error: patch failed: core/modules/taxonomy/taxonomy.module:18
error: core/modules/taxonomy/taxonomy.module: patch does not apply
adci_contributor’s picture

adci_contributor CreditAttribution: adci_contributor commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
taxonomy_type_hints-1807002-14.patch3.32 KB

Trying to reroll.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Needs work

phpcs tells me there are 'Missing parameter name' and 'Missing parameter type' errors in /src/Controller/TermAutocompleteController.php, /src/Plugin/views/filter/TaxonomyIndexTid.php and /taxonomy.module

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Assigned: Jeroen » Unassigned
Status: Needs work » Needs review
FileSize
taxonomy_type_hints-1807002-16.patch9.29 KB
interdiff.txt6.45 KB

I ran phpcs after applying the patch in #14 and found several more issues. Patch attached.

Status: Needs review » Needs work

The last submitted patch, 16: taxonomy_type_hints-1807002-16.patch, failed testing.

mrjmd’s picture

mrjmd CreditAttribution: mrjmd commented
Status: Needs work » Needs review
FileSize
taxonomy_type_hints-1807002-18.patch9.94 KB
interdiff.txt658 bytes

Oops, another try.

Also resisting the urge to fix the numerous other phpcs violations in this module :).

Status: Needs review » Needs work

The last submitted patch, 18: taxonomy_type_hints-1807002-18.patch, failed testing.

Mile23’s picture

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

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Needs reroll

This needs re-roll after #1847596: Remove Taxonomy term reference field in favor of Entity reference

siva_epari’s picture

siva_epari CreditAttribution: siva_epari commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
add_missing_type-1807002-22.patch8.45 KB

Patch rerolled.

Status: Needs review » Needs work

The last submitted patch, 22: add_missing_type-1807002-22.patch, failed testing.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
FileSize
interdiff-22-24.txt860 bytes
add_missing_type-1807002-24.patch9.39 KB

Added missing type hinting in TermStorage

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Needs work

Still missing a bunch in taxonomy.module. Which shows me that I have a bug in my phpcs review one-liner above... add a --extensions="module/php" option.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
FileSize
add_missing_type-1807002-26.patch15.39 KB
interdiff-24-26.txt6.45 KB
willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Needs work

Patch in #26 has out-of-scope function signature hinting. We only want docblock changes here, for @param and @returns.

Bumping to 8.1.x.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

idebr CreditAttribution: idebr at iO commented
Status: Needs work » Needs review
FileSize
interdiff-26-34.txt13.08 KB
1807002-34.patch7.35 KB

Rerolled against 8.6.x.

#28 Removed changes to function arguments

Fixed the following sniffs in the Taxonomy module:

  1. Drupal.Commenting.FunctionComment.InvalidNoReturn
  2. Drupal.Commenting.FunctionComment.InvalidTypeHint
  3. Drupal.Commenting.FunctionComment.MissingParamComment
  4. Drupal.Commenting.FunctionComment.MissingParamType
  5. Drupal.Commenting.FunctionComment.MissingReturnComment
  6. Drupal.Commenting.FunctionComment.MissingReturnType
  7. Drupal.Commenting.FunctionComment.ParamMissingDefinition

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Closed (duplicate)
Related issues: +#3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType

This 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.