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

Related sprint issues:

Sprint Topic Sub Issue
#1518116: [meta] Make Core pass Coder Review #1533376: Make poll module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1355712: Clean up API docs for Poll Module
#500866: [META] remove t() from assert message #1797376: Remove t() from assertion messages in tests for the poll module
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

Status: Active » Needs review
FileSize
4.12 KB
bleen’s picture

+++ b/core/modules/poll/poll.moduleundefined
@@ -107,11 +107,11 @@ function poll_menu() {
- * @param $node
+ * @param Drupal\node\Node $node

Is this correct? I had never seen this before until I looked in the poll module

Lars Toomre’s picture

Thanks @bleen18 for rolling this and the other type hinting patches. If you have a chance, perhaps you could try updating the patch in #1800330: Add missing type hinting to System module docblocks. That one has a bunch of review comments from @jhodgdon.

Re: #2 Look at #1487760: [policy, no patch] Decide on documentation standards for namespaced items for discussion about our new standard for name-spacing. I have seen several patches that use this new standard. As a result, from afar, I believe the following is the proper documentation:

- * @param $node
+ * @param \Drupal\node\Node $node

I will try to get to reviewing this and the other type hinting patches you have created later this weekend. In the interim, I have some personal life commitments to meet.

Lars Toomre’s picture

Status: Needs review » Needs work

Thanks for the patch @bleen18. Here is a purposefully nit-picking review along with comments in dreditor so that whoever commits this eventually can see what was checked.

One thing I noticed that was common throughout this patch was no leading slash '\' ahead of 'Drupal\node\Node' type hints in the docblocks. The policy for this was recently adopted in #1487760: [policy, no patch] Decide on documentation standards for namespaced items. As a result, you might want to do a global replace for those string occurrences within docblocks.

+++ b/core/modules/poll/poll.moduleundefined
@@ -107,11 +107,11 @@ function poll_menu() {
  *
- * @param $node
+ * @param Drupal\node\Node $node
  *   The poll node object.

I am not sure about this type hint. My suspicion is that a stdClass object is sufficient here since no Node methods are used in this function. Hence, I would change it object.

If it were to remain 'Drupal\node\Node', it needs to start with a '\'.

+++ b/core/modules/poll/poll.moduleundefined
@@ -107,11 +107,11 @@ function poll_menu() {
- * @param $perm
+ * @param string $perm
  *   A permission the user must have to view the page.
- * @param $inspect_allowvotes
+ * @param bool $inspect_allowvotes
  *   TRUE if the user can view votes, and FALSE if the user cannot view votes.
  */
 function _poll_menu_access($node, $perm, $inspect_allowvotes) {
@@ -620,11 +620,13 @@ function poll_delete($node) {

These two type hint additions to _poll_menu_access() are correct.

+++ b/core/modules/poll/poll.moduleundefined
@@ -620,11 +620,13 @@ function poll_delete($node) {
  *
  * @param Drupal\node\Node $node
  *   The node entity to load.

This type hint needs to start with a leading '\'.

+++ b/core/modules/poll/poll.moduleundefined
@@ -620,11 +620,13 @@ function poll_delete($node) {
+ * @return Drupal\node\Node $node

Thanks for adding this missing @return directive. It needs a leading slash '\' and a explanation line.

+++ b/core/modules/poll/poll.moduleundefined
@@ -690,10 +692,10 @@ function poll_view($node, $view_mode) {
  *
- * @param $node
+ * @param Drupal\node\Node $node
  *   The poll node object.
  *
- * @return
+ * @return string
  *   Poll choices in a simple teaser format.

This docblock needs some work. Again I think this can be a simple object as long as it contains the 'choice property. Hence, I would change the first type hint to object.

The second type hint should be 'string|null' since NULL can be returned.

+++ b/core/modules/poll/poll.moduleundefined
@@ -711,11 +713,12 @@ function poll_teaser($node) {
  * Form constructor for the poll voting form.
  *
- * @param $node

As above, I think that this type hint should be 'object' instead of a full Node object.

+++ b/core/modules/poll/poll.moduleundefined
@@ -834,6 +837,17 @@ function template_preprocess_poll_vote(&$variables) {
  *
+ * @param Drupal\node\Node $node
+ *   The poll node whose results are to be displayed.
+ * @param string $view_mode
+ *   The view mode to use when displaying the poll results.
+ * @param bool $block
+ *   (optional) TRUE if a poll should be displayed in a block. Defaults to
+ *   FALSE.
+ *
+ * @return string
+ *   The themed poll results.

Thanks for adding these missing @param and @return directives and explanations. They are correct except that the first one needs a leading slash '\'.

+++ b/core/modules/poll/poll.moduleundefined
@@ -877,7 +891,7 @@ function poll_view_results($node, $view_mode, $block = FALSE) {
  *
- * @param $variables
+ * @param array $variables
  *   An associative array containing:

This type hint addition is correct in theme_poll_choices().

+++ b/core/modules/poll/poll.moduleundefined
@@ -931,7 +945,7 @@ function theme_poll_choices($variables) {
  *
- * @param $variables
+ * @param array $variables
  *   An associative array containing:

This type hint addition is correct in template_preprocess_poll_results().

+++ b/core/modules/poll/poll.moduleundefined
@@ -960,7 +974,7 @@ function template_preprocess_poll_results(&$variables) {
  *
- * @param $nid
+ * @param int $nid
  *   The poll node ID.

This type hint addition for poll_cancel_form() is correct.

+++ b/core/modules/poll/poll.pages.incundefined
@@ -8,7 +8,7 @@
  *
- * @return
+ * @return string
  *   The HTML for the page that shows the available polls.

This type hint addition for poll_page() is correct.

+++ b/core/modules/poll/poll.pages.incundefined
@@ -56,10 +56,10 @@ function poll_page() {
  *
- * @param $node
+ * @param Drupal\node\Node $node
  *   The poll node object.
  *
- * @return
+ * @return array
  *   Render array containing table with votes.

These added type hints for poll_votes() are basically correct. The first one needs a leading slash '\'.

I would change the second explanation to 'A render array containing a table with votes.'

+++ b/core/modules/poll/poll.pages.incundefined
@@ -110,10 +110,10 @@ function poll_votes($node) {
  *
- * @param $node
+ * @param Drupal\node\Node $node
  *   The poll node object.
  *
- * @return
+ * @return array
  *   An array suitable for use by drupal_render().

The type hints added for poll_results() are again basically correct. However, the first again needs a leading slash '\'.

========== Additional review notes:

I noticed too that we are missing a @return directive for _poll_menu_access().

It appears that all of the @param and @return directive is missing for _poll_choice_form(). Remember to use '(optional)' and 'Defaults to x' in the explanation of each of the variables with a default value.

I am not sure whether or not we need @param and @return directives for poll_node_form_submit(). What do you think? Perhaps it might be better to change the one line summary to 'Form submission handler for node_form().' In that case, no @param directives are required according to doc standards.

Both poll_preprocess_block() and template_preprocess_poll_vote() need a @param directive like that used in template_preprocess_poll_results().

Checking the rest of the files for the Poll module, I see that we have missid two @return type hints in PollTestBase.php.

Thanks again for rolling this patch @bleen18. I could easily roll a patch that includes these changes. However, that would preclude me from reviewing the results. Could you address what I have noted above and then I will review it so that it is in pristine condition for @jhodgdon's final review and commit? Thanks in advance.

Lars Toomre’s picture

Just to note for cross-reference... Issue #1355712-45: Clean up API docs for Poll Module has notes about missing @param and/or @return directives for the Poll module. We need to double-check that we have caught here all of the @param/@return items noted there.

bleen’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.96 KB

I am not sure about this type hint. My suspicion is that a stdClass object is sufficient here since no Node methods are used in this function. Hence, I would change it object.

I disagree with this ... while nothing will technically break if you pass in a standard object with the correct properties defined, the intent is that you pass in a proper node object. Anything else would be hackish in most cases.

Perhaps it might be better to change the one line summary to 'Form submission handler for node_form().' In that case, no @param directives are required according to doc standards.

With this I agree...

Other than that I have made the suggested corrections (and a bunch more in the base test)

Lars Toomre’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @bleen18. Could you possibly add an interdiff between #6 and #1? It really would help when reviewing these detailed documentation patches. Thanks a lot!

(I am working on a detailed review of Overlay sub-issue right now and am almost done with that one. I will try to come back to this one after that.)

alexpott’s picture

Project: Drupal core » Poll
Version: 8.x-dev » 8.x-1.x-dev
Component: documentation » Documentation

Poll has been removed from core

adammalone’s picture

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

This will almost certainly need a reroll.

Mile23’s picture

adammalone’s picture

Mile23’s picture

It's listed there as a sub-issue, and I was just going through quickly and setting the parent relationships. I'll update the issue there.

tduong’s picture

Status: Needs work » Needs review
FileSize
17.73 KB

Start again from reroll. Many things have been changed.

  • Berdir committed 0e81b06 on 8.x-1.x authored by tduong
    Issue #1811880 by bleen, tduong: Add missing type hinting to Poll module...
Berdir’s picture

Status: Needs review » Fixed

Thanks.

Status: Fixed » Closed (fixed)

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