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

  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 #1532692: Make Comment module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1332598: Clean up API docs for comment module
#500866: [META] remove t() from assert message #1742602: Removing t() from asserts in simpletests in comment module
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Novice Instructions Done
Manually check type hints validity Novice Instructions
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

FileSize
1.91 KB

Here is a patch that adds type hinting to the hooks in comment.api.php file.

Lars Toomre’s picture

Status: Active » Needs review
larowlan’s picture

Lars, 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

Lars Toomre’s picture

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

larowlan’s picture

Thanks, will do.

Lars Toomre’s picture

Status: Needs review » Postponed

Postponed until after feature freeze (Dec. 1 2012) for #3.

roderik’s picture

Issue summary: View changes
Status: Postponed » Needs work
Issue tags: +Amsterdam2014

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

roderik’s picture

Issue tags: +Novice
dankh’s picture

Assigned: Unassigned » dankh

I working on it

dankh’s picture

Patch needs to be re-rolled.

git apply --check 1808478-1-comment-api.patch
error: patch failed: core/modules/comment/comment.api.php:54
error: core/modules/comment/comment.api.php: patch does not apply

I'll re-roll the patch.

dankh’s picture

Assigned: dankh » Unassigned
Status: Needs work » Needs review

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

roderik’s picture

Status: Needs review » Closed (duplicate)

Thanks 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 :)

Mile23’s picture

Status: Closed (duplicate) » Needs review
FileSize
5.2 KB

Missed a few.

Re-opening here since it's already under that nice [meta].

Mile23’s picture

llbbl’s picture

Working on reviewing this.

llbbl’s picture

FileSize
5.16 KB
  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -333,9 +333,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +   * @param Drupal\Core\Form\FormStateInterface $form_state
    

    This needs to use Overrides instead of @param because there is a namespace at the top of the file.

  2. +++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
    @@ -85,13 +85,13 @@ protected function setUp() {
    +   *   checking, or an array of values to set contact info.
    

    Missing a return

  3. +++ b/core/modules/comment/src/Tests/CommentPagerTest.php
    @@ -167,9 +167,9 @@ function testCommentOrderingThreading() {
    +   * @param array $expected_order
    

    Missing a return

Attached is a new patch files with a few additional changes listed above.

Status: Needs review » Needs work

The last submitted patch, 16: 1808478_16.patch, failed testing.

llbbl’s picture

FileSize
5.29 KB

Screwed up the patch file. Oops! Lets try this again.

  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -333,9 +333,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +   * @param Drupal\Core\Form\FormStateInterface $form_state
    

    This needs to use Overrides instead of @param because there is a namespace at the top of the file.

  2. +++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
    @@ -85,13 +85,13 @@ protected function setUp() {
    +   *   checking, or an array of values to set contact info.
    

    Missing a return

  3. +++ b/core/modules/comment/src/Tests/CommentPagerTest.php
    @@ -167,9 +167,9 @@ function testCommentOrderingThreading() {
    +   * @param array $expected_order
    

    Missing a return

Attached is a new patch files with a few additional changes listed above.

rgristroph’s picture

I 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 !

Mile23’s picture

Status: Needs work » Needs review

Setting to needs review so the testbot works on #18.

Mile23’s picture

FileSize
17.73 KB
12.63 KB

Found a bunch more with the magic of NetBeansDrupalComposed. :-)

Status: Needs review » Needs work

The last submitted patch, 21: 1808478_21.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
18.76 KB
1.04 KB

Addressed the errors in the test, plus a few more. Let's see if the testbot finds any I didn't....

jhodgdon’s picture

Component: documentation » comment.module

Changing component. This is changing function signatures, not just documentation.

jhodgdon’s picture

Status: Needs review » Needs work

Oh and the patch needs work for CommentForm at least:

-   * @param $form_state
+   * Overrides Drupal\Core\Form\FormStateInterface $form_state
    *   The current state of the form.
    */
   public function preview(array &$form, FormStateInterface $form_state) {

Oops. @param was removed.

Mile23’s picture

Status: Needs work » Needs review
FileSize
18.76 KB
660 bytes

Fixed that one @param in CommentForm.

If nothing happens to this in the next week or so I'll take out the method signature changes.

Mile23’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs reroll.

mrjmd’s picture

Status: Needs work » Needs review
FileSize
18.79 KB

Reroll attached.

mrjmd’s picture

Issue tags: -Needs reroll
Mile23’s picture

Still applies.

Mile23’s picture

Issue summary: View changes
deepakaryan1988’s picture

Re-rolling the patch#28 with some modification.

yogen.prasad’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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

mlevasseur’s picture

FileSize
8.04 KB

Reroll...

Removed: LinkDelete.php, LinkEdit.php, Link.php
Conflicts: comment.module (fixed a typo for one @param: "sting" -> "string"), LinkApprove.php, LinkReply.php

jgeryk’s picture

Applies cleanly at commit 2e66338

deepakaryan1988’s picture

Status: Needs work » Needs review

The last submitted patch, 35: add-missing-type-hinting-to-comment-module-1808478-#33.patch, failed testing.

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -249,9 +249,9 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
- * @param $view_mode
- *   (optional) View mode; e.g., 'full', 'teaser', etc. Defaults to 'full'.
- * @param $langcode
+ * @param string $view_mode
+ *   (optional) View mode, e.g. 'full', 'teaser'... Defaults to 'full'.
+ * @param string|NULL $langcode

@@ -265,11 +265,11 @@ function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode
- * @param $view_mode
- *   View mode; e.g., 'full', 'teaser', etc.
- * @param $langcode
+ * @param string $view_mode
+ *   View mode, e.g. 'full', 'teaser'...
+ * @param string|NULL $langcode

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

prhavile’s picture

Status: Needs work » Needs review
Issue tags: +#DHCSprint
FileSize
7.27 KB

Patch Rerolled.

prhavile’s picture

apologies. wrong file got uploaded.

prhavile’s picture

Correct file uploaded which have the correct comment.

prhavile’s picture

There were two occurrences of "etc", removed both of them.

hctom’s picture

Assigned: Unassigned » hctom

I will start reviewing this and reroll the patch if needed

hctom’s picture

So.. 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)

hctom’s picture

Hid missed outdated interdiff file...

hctom’s picture

Assigned: hctom » Unassigned
stBorchert’s picture

Patch applies without any warnings. Manually check for type hints validity can be started ;)

hctom’s picture

Assigned: Unassigned » hctom

okay... i will take care of this now

Status: Needs review » Needs work

The last submitted patch, 45: add_missing_type-1808478-44.patch, failed testing.

hctom’s picture

Had 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).)

hctom’s picture

Assigned: hctom » Unassigned
Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/src/Plugin/views/field/LinkApprove.php
@@ -7,6 +7,9 @@
+use Drupal\comment\CommentInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Session\AccountInterface;

+++ b/core/modules/comment/src/Plugin/views/field/LinkReply.php
@@ -7,6 +7,8 @@
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Session\AccountInterface;

These changes look incorrect

Also I think this should be limited to fixing missing type hinting and not moving use statements.

no_angel’s picture

Assigned: Unassigned » no_angel

The last submitted patch, 45: add_missing_type-1808478-44.patch, failed testing.

no_angel’s picture

Assigned: no_angel » Unassigned
Mile23’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs reroll

Bumping to 8.1.x. Patch no longer applies.

naveenvalecha’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.71 KB
Mile23’s picture

Status: Needs review » Needs work

Nice, thanks. :-)

  1. +++ b/core/modules/comment/comment.module
    @@ -264,12 +264,12 @@ function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode
    - * @param $comments
    + * @param array $comments
      *   An array of comments as returned by entity_load_multiple().
    

    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

  2. +++ b/core/modules/comment/src/CommentForm.php
    @@ -335,9 +335,9 @@ protected function flagViolations(EntityConstraintViolationListInterface $violat
    -   * @param $form_state
    +   * @param array $form_state
        *   The current state of the form.
        */
       public function preview(array &$form, FormStateInterface $form_state) {
    

    Should be \Drupal\Core\Form\FormStateInterface, instead of array.

naveenvalecha’s picture

deepakaryan1988’s picture

Status: Needs work » Needs review
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks.

Patch applies, addresses all the concerns in #60, phpcs says no errors.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: 1808478-61.patch, failed testing.

The last submitted patch, 61: 1808478-61.patch, failed testing.

snehi’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/src/Tests/CommentPagerTest.php
@@ -173,9 +173,9 @@ function testCommentOrderingThreading() {
+   * @param array $comments

Shouldn't this also have the CommentInterface type hint?

catch’s picture

+++ b/core/modules/comment/src/Tests/CommentPagerTest.php
@@ -173,9 +173,9 @@ function testCommentOrderingThreading() {
+   * @param array $comments

Shouldn't this also have the CommentInterface type hint?

nikunjkotecha’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
706 bytes

Updating patch as per the comments #60 and #67

nikunjkotecha’s picture

Issue tags: +drupalconasia2016
lokesh.soni’s picture

Assigned: Unassigned » lokesh.soni
lokesh.soni’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Amsterdam2014, -Novice, -#DHCSprint, -drupalconasia2016 ++drupalconasia2016
Wim Leers’s picture

Priority: Normal » Minor
Issue tags: -+drupalconasia2016 +drupalconasia2016, +Quickfix

  • catch committed 45689e4 on 8.1.x
    Issue #1808478 by Mile23, prhavile, naveenvalecha, llbbl, hctom,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

Status: Fixed » Closed (fixed)

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