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:
Comment | File | Size | Author |
---|---|---|---|
#13 | 1811880-13.patch | 17.73 KB | tduong |
| |||
#6 | 1811880.patch | 7.96 KB | bleen |
#1 | 1811880.patch | 4.12 KB | bleen |
Comments
Comment #1
bleen CreditAttribution: bleen commentedComment #2
bleen CreditAttribution: bleen commentedIs this correct? I had never seen this before until I looked in the poll module
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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:
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.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedThanks 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.
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 '\'.
These two type hint additions to _poll_menu_access() are correct.
This type hint needs to start with a leading '\'.
Thanks for adding this missing @return directive. It needs a leading slash '\' and a explanation line.
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.
As above, I think that this type hint should be 'object' instead of a full Node object.
Thanks for adding these missing @param and @return directives and explanations. They are correct except that the first one needs a leading slash '\'.
This type hint addition is correct in theme_poll_choices().
This type hint addition is correct in template_preprocess_poll_results().
This type hint addition for poll_cancel_form() is correct.
This type hint addition for poll_page() is correct.
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.'
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.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedJust 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.
Comment #6
bleen CreditAttribution: bleen commentedI 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.
With this I agree...
Other than that I have made the suggested corrections (and a bunch more in the base test)
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.)
Comment #8
alexpottPoll has been removed from core
Comment #9
adammaloneThis will almost certainly need a reroll.
Comment #10
Mile23Comment #11
adammalone@Mile23 I'm not sure if #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* is an appropriate issue for this project to be linked to now that poll is no longer in core.
Comment #12
Mile23It'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.
Comment #13
tduong CreditAttribution: tduong at MD Systems GmbH commentedStart again from reroll. Many things have been changed.
Comment #15
BerdirThanks.