This is a sub-issue of #1800046: [META] Add missing type hinting to core docblocks 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
Manually check type hints validity Novice Instructions
Files: 
CommentFileSizeAuthor
#36 add-missing-type-hinting-to-comment-module-1808478.patch7.27 KBjgeryk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,302 pass(es).
[ View ]
#35 add-missing-type-hinting-to-comment-module-1808478-#33.patch8.04 KBmlevasseur
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add-missing-type-hinting-to-comment-module-1808478-%2333.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 add-missing-type-hinting-to-comment-module-1808478-#32.patch9.7 KBdeepakaryan1988
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es).
[ View ]
#23 interdiff.txt1.04 KBMile23
#23 1808478_23.patch18.76 KBMile23
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,090 pass(es).
[ View ]
#21 interdiff.txt12.63 KBMile23
#21 1808478_21.patch17.73 KBMile23
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#18 1808478_18.patch5.29 KBllbbl
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,019 pass(es).
[ View ]
#16 1808478_16.patch5.16 KBllbbl
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1808478_16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 1808478_13.patch5.2 KBMile23
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,437 pass(es).
[ View ]
#1 1808478-1-comment-api.patch1.91 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 42,116 pass(es).
[ View ]

Comments

Lars Toomre’s picture

StatusFileSize
new1.91 KB
PASSED: [[SimpleTest]]: [MySQL] 42,116 pass(es).
[ View ]

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
StatusFileSize
new5.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,437 pass(es).
[ View ]

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

StatusFileSize
new5.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1808478_16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  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

StatusFileSize
new5.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,019 pass(es).
[ View ]

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

StatusFileSize
new17.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new12.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
StatusFileSize
new18.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,090 pass(es).
[ View ]
new1.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
StatusFileSize
new18.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,472 pass(es).
[ View ]
new660 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
StatusFileSize
new18.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,125 pass(es).
[ View ]

Reroll attached.

mrjmd’s picture

Issue tags:-Needs reroll
Mile23’s picture

Still applies.

Mile23’s picture

Issue summary:View changes
deepakaryan1988’s picture

StatusFileSize
new9.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,949 pass(es).
[ View ]

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

StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add-missing-type-hinting-to-comment-module-1808478-%2333.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new7.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,302 pass(es).
[ View ]

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