Support from Acquia helps fund testing for Drupal Acquia logo

Comments

surendramohan’s picture

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

@surendramohan: Do you still plan to work on this? If so, please respond. If not, we'll unassign it so someone else can. Thanks!

Also, tagging.

chertzog’s picture

Assigned: surendramohan » chertzog

I will take this over. Should have a patch tomorrow.

chertzog’s picture

Status: Active » Needs review
FileSize
9.4 KB

First run.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch! Looks like a good start. A few things need to be addressed:

a)

--- a/core/modules/poll/poll.module
+++ b/core/modules/poll/poll.module
@@ -105,6 +105,18 @@ function poll_menu() {
 
 /**
  * Callback function to see if a node is acceptable for poll menu items.

Needs to start with a verb, or perhaps use the standard for callback functions:
http://drupal.org/node/1354#callbacks

b)

@@ -123,6 +135,7 @@ function poll_block_info() {
  * Implements hook_block_view().
  *
  * Generates a block containing the latest poll.
+ * @see poll_block_latest_poll_view()
  */
 function poll_block_view($delta = '') {

Blank line is needed before the @see.

c)

@@ -360,6 +385,18 @@ function poll_more_choices_submit($form, &$form_state) {
   $form_state['rebuild'] = TRUE;
 }
 
+/**
+ * Form submit handler for node_form().
+ *
+ * Upon preview and final submission, we need to renumber poll choices and
+ * create a teaser output.
+ *
+ * @see poll_form()
+ * @see poll_choice_js()
+ * @see poll_node_form_submit()
+ * @see poll_more_choices_submit()
+ * @see poll_teaser()
+ */
 function _poll_choice_form($key, $chid = NULL, $value = '', $votes = 0, $weight = 0, $size = 10) {

This is not actually a form submission handler -- wrong args. Need to figure out what it is and document that instead.

d)

@@ -407,12 +444,17 @@ function _poll_choice_form($key, $chid = NULL, $value = '', $votes = 0, $weight
 }
 
 /**
- * Ajax callback in response to new choices being added to the form.
+ * Ajax callback for adding new choices to the poll node form.
  *
  * This returns the new page content to replace the page content made obsolete
  * by the form submission.
  *
+ *

Don't add this extra blank line.

e)

@@ -672,6 +726,12 @@ function poll_view($node, $view_mode) {
  * Creates a simple teaser that lists all the choices.
  *
  * This is primarily used for RSS.
+ *
+ * @see poll_more_choices_submit()
+ * @see poll_node_form_submit()
+ * @see poll_choice_js()
+ * @see _poll_choice_form()
+ * @see poll_form()
  */
 function poll_teaser($node) {

Why are all these @see lines needed here? Also this needs @param docs, and probably @return.

f)

@@ -686,10 +746,11 @@ function poll_teaser($node) {
 }
 
 /**
- * Generates the voting form for a poll.
+ * Form constructor for the poll voting form.
  *
  * @ingroup forms
  * @see poll_vote()
+ * @see poll_view_voting_validate()
  * @see phptemplate_preprocess_poll_vote()
  */
 function poll_view_voting($form, &$form_state, $node, $block = FALSE) {

Needs @param docs for $node, $block.

g)

@@ -787,7 +855,7 @@ function poll_preprocess_block(&$variables) {
 /**
  * Themes the voting form for a poll.
  *
- * Inputs: $form
+ * @see poll-vote.tpl.php
  */
 function template_preprocess_poll_vote(&$variables) {

Not a good one-line description here. A preprocess function doesn't "theme" anything.

h) There are quite a few other functions that need @param and/or @return docs in poll.module.

i)

@@ -898,12 +969,6 @@ function theme_poll_choices($variables) {
 /**
  * Preprocess the poll_results theme hook.
  *
- * Inputs: $raw_title, $results, $votes, $raw_links, $block, $nid, $vote. The
- * $raw_* inputs to this are naturally unsafe; often safe versions are
- * made to simply overwrite the raw version, but in this case it seems likely
- * that the title and the links may be overridden by the theme layer, so they
- * are left in with a different name for that purpose.
- *
  * @see poll-results.tpl.php
  * @see poll-results--block.tpl.php
  */

In the first line, wrong verb tense, and instead of "the poll_results theme hook" it should say "poll-results.tpl.php".

j)

@@ -939,7 +1007,10 @@ function poll_cancel_form($form, &$form_state, $nid) {
 }
 
 /**
- * Submit callback for poll_cancel_form().
+ * Form submission handler for poll_cancel_form().
+ *
+ * @ingroup forms
+ * @see poll_cancel_form()
  */
 function poll_cancel($form, &$form_state) {

The @see is not necessary, as it's referenced in the first line.

k) poll.pages.inc now...

 /**
- * Menu callback to provide a simple list of all polls available.
+ * Page callback: Displays a simple list of all available polls.
+ *
+ * @return
+ *   A page displaying the poll results.
+ *
+ * @see poll_menu()
  */
 function poll_page() {

The @return here needs to say whether it's returning a string or a renderable array. "a page" is ambiguous. See
http://drupal.org/node/1354#menu-callback
Same applies to the other page callbacks in this file.

l) There is a lot to clean up in poll.test, which is missing from this patch.

m) poll.module : poll_block_latest_poll_view() - needs to be fixed up and included in this patch.

gapa’s picture

I have corrected problem from a), b), d), e), f), g), i), k)
And not (yet) from c), h), l), m)

This is my first time to provide patch for documentation. Please review it and give me feedback so I could continue with this in proper direction. I don't know documentation standars so good but will learn them. I have a feeling that more can be done than just notes that were given to me to create good documentation.

br,
Gapa

xjm’s picture

Status: Needs work » Needs review

Marking for review.

Status: Needs review » Needs work

The last submitted patch, poll_api_cleanup-1355712-6.patch, failed testing.

xjm’s picture

Thanks for your work on this @gapa. It appears that the patch was created with NetBeans and is not compatible with git. See http://drupal.org/node/707484 for information on creating git patches.

gapa’s picture

Ahhh. I uploded wrong one. Hope this works better. Done with git diff.

Thanks

gapa’s picture

Status: Needs work » Needs review

Marking for review.

Status: Needs review » Needs work

The last submitted patch, poll_api_cleanup-1355712-6.patch, failed testing.

jhodgdon’s picture

The poll.module part of the patch apparently needs a reroll. This part doesn't apply:

@@ -656,6 +704,9 @@
  *
  * @param $node
  *   The node object to load.
+ *
+ * @see poll_view_results()
+ * @see poll_view()
  */
 function poll_block_latest_poll_view($node) {
gapa’s picture

Status: Needs work » Needs review
FileSize
10.48 KB

Let's try again. Thanks for your help with this.

Status: Needs review » Needs work

The last submitted patch, poll_api_cleanup-1355712-6.patch, failed testing.

jhodgdon’s picture

The failed test is in the Poll module, so there is likely some actual problem with this patch...

xjm’s picture

+++ b/core/modules/poll/poll-results.tpl.phpundefined
@@ -15,9 +15,11 @@
-<div class="poll">
+<article class="poll">

@@ -30,7 +32,7 @@
-</div>
+</article>

+++ b/core/modules/poll/poll-vote.tpl.phpundefined
@@ -12,9 +12,11 @@
-<div class="poll">
+<article class="poll">

@@ -26,4 +28,4 @@
-</div>
+</article>

+++ b/core/modules/poll/poll.moduleundefined
@@ -6,8 +6,6 @@
-use Drupal\node\Node;
-

@@ -608,10 +654,13 @@ function poll_delete($node) {
-function poll_block_latest_poll_view(Node $node) {
+function poll_block_latest_poll_view($node) {

These changes do not belong. The patch should only include doxygen changes. Please review patches carefully before you upload them. :)

gapa’s picture

Status: Needs work » Needs review
FileSize
9.75 KB

The last two changes I probably made. Don't know actually why.
But the first four changes I most definitly didn't do. But I probably had older version of drupal8. It actually was "<article ..." at some point but went back to "<div ...". Convert poll tpls and markup to HTML5
I wasn't expecting this to be so complicated. :) But thanks for your help and I hope that this time it will work.

xjm’s picture

It was probably a result of having a branch that was not up-to-date and doing a diff. (So your branch was missing some changes that were in the 8.x branch, and thus when you created the diff it reverted those changes.) One way to avoid those issues is to use git's rebase functionality in the local repository while you are working on your patch. :) I know I still mess it up sometimes anyway, which is why I always read over patch files when I submit them to make sure all the changed lines are changes I actually meant to make!

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch! I gave it a review, and it still needs quite a bit of work... First, some general observations:

1. There were a LOT of @see lines added. While this is not a bad idea, I am not sure all of them are necessary. I didn't check them over, but generally we want @see to related functions that might not be obvious, but not @see to all the functions that a given function calls or that call this function.

2. The standards for writing documentation are at http://drupal.org/node/1354

3. Usually we don't want to remove information that is there -- just reformat it correctly.

Specific comments on the patch (I didn't apply the patch to see if there were other things in the Poll module directory that needed to be changed -- these comments only apply to what I could see in the patch file):

a)

+++ b/core/modules/poll/poll.module
@@ -106,7 +106,19 @@ function poll_menu() {
 }
 
 /**
- * Callback function to see if a node is acceptable for poll menu items.
+ * Looks if a node is acceptable for poll menu items.
+ *
...
+ * @param $inspect_allowvotes
+ *   Boolean indicated whether the user can view the individual votes.
...
+ * @return
+ *   A render array for a page containing a list of content.
+ *
+ * @see poll_menu()
  */
 function _poll_menu_access($node, $perm, $inspect_allowvotes) {

- The first line is not good grammar, and it doesn't follow the standards at http://drupal.org/node/1354#menu-callback
- The param description shown here also isn't good grammar. How about "TRUE if the user can view votes, and FALSE if the user cannot view votes"?
- The return value is just wrong. This is an access callback. It doesn't return a render array at all.

b)

@@ -217,6 +231,12 @@ function poll_field_extra_fields() {
 
 /**
  * Implements hook_form().
+ *
+ * @see poll_more_choices_submit()
+ * @see poll_node_form_submit()
+ * @see poll_choice_js()
+ * @see _poll_choice_form()
+ * @see poll_teaser()
  */
 function poll_form($node, &$form_state) {

This needs to follow the standards for form generating functions at http://drupal.org/node/1354#forms

c)

@@ -340,13 +360,19 @@ function poll_form($node, &$form_state) {
 }
 
 /**
- * Submit handler to add more choices to a poll form.
+ * Form submission handler for the add more choices button on a poll node form.
  *
  * This handler is run regardless of whether JS is enabled or not. It makes
  * changes to the form state. If the button was clicked with JS disabled, then
  * the page is reloaded with the complete rebuilt form. If the button was
  * clicked with JS enabled, then ajax_form_callback() calls poll_choice_js() to
  * return just the changed part of the form.

- First line should say "on poll_form()" not "on a poll node form", I think? Or whichever form function it is for.
- The description should have JavaScript written out, not abbreviated as JS.

d)

@@ -362,6 +388,18 @@ function poll_more_choices_submit($form, &$form_state) {
   $form_state['rebuild'] = TRUE;
 }
 
+/**
+ * Form submit handler for node_form().
+ *
+ * Upon preview and final submission, we need to renumber poll choices and
+ * create a teaser output.
+ *
+ * @see poll_form()
+ * @see poll_choice_js()
+ * @see poll_node_form_submit()
+ * @see poll_more_choices_submit()
+ * @see poll_teaser()
+ */
 function _poll_choice_form($key, $chid = NULL, $value = '', $votes = 0, $weight = 0, $size = 10) {

The first line does not look right to me. It doesn't have the right arguments to be a form submission handler.

e)

@ -608,8 +656,11 @@ function poll_delete($node) {
 /**
  * Return content for 'latest poll' block.
  *
- * @param Drupal\node\Node $node
- *   The node entity to load.
+ * @param $node
+ *   The node object to load.

- First line: verb tense is wrong.
- Other changes here: The original was better. There is no reason to take away the parameter type documentation that was added in another patch.

f)

+ * @return
+ *   String of choices.
+ *
+ * @see poll_node_form_submit()
  */
 function poll_teaser($node) {

"String of choices" is not very good documentation.

g)

@@ -685,10 +747,17 @@ function poll_teaser($node) {
 }
 
 /**
- * Generates the voting form for a poll.
+ * Form constructor for the poll voting form.
  *
  * @ingroup forms
...
+ * @param $block
+ *   (optional) TRUE if poll should be done in block. Defaults to FALSE.

- The @ingroup should be moved to the end -- see http://drupal.org/node/1354#forms
- $block parameter -- description needs some a/the added.

h)

@@ -726,7 +795,11 @@ function poll_view_voting($form, &$form_state, $node, $block = FALSE) {
 }
 
 /**
- * Validation function for processing votes
+ * Form validation handler for poll_view_voting().
+ *
+ * @see poll_view_voting()
+ * @see phptemplate_preprocess_poll_vote()
+ * @see poll_vote()
  */
 function poll_view_voting_validate($form, &$form_state) {

Do not include an @see for poll_view_voting() -- that is taken care of by the first line description. The same applies to the submission handler just below.

i)

@@ -784,9 +860,9 @@ function poll_preprocess_block(&$variables) {
 }
 
 /**
- * Themes the voting form for a poll.
+ * Processes variables for poll-vote.tpl.php.
  *
- * Inputs: $form
+ * @see poll-vote.tpl.php
  */
 function template_preprocess_poll_vote(&$variables) {

It's pre-processing, not processing. And the @see is not necessary, as above.

j)

@@ -895,13 +974,7 @@ function theme_poll_choices($variables) {
 }
 
 /**
- * Preprocess the poll_results theme hook.
- *
- * Inputs: $raw_title, $results, $votes, $raw_links, $block, $nid, $vote. The
- * $raw_* inputs to this are naturally unsafe; often safe versions are
- * made to simply overwrite the raw version, but in this case it seems likely
- * that the title and the links may be overridden by the theme layer, so they
- * are left in with a different name for that purpose.
+ * Preprocess the poll-results.tpl.php.
  *
  * @see poll-results.tpl.php
  * @see poll-results--block.tpl.php

- Wrong verb tense in first line, and it should be consistent with other pre-process functions.
- Why is the information about inputs removed? It should actually list @param $variables, and then list the array elements present as a list.

k) poll.pages.inc now...

 
 /**
- * Menu callback to provide a simple list of all polls available.
+ * Page callback: Displays a simple list of all available polls.
+ *
+ * @return
+ *   A string with listed polls.
+ *
+ * @see poll_menu()
  */
 function poll_page() {

The return value should probably say "HTML for the page".

l)

+ * @return
+ *   A page displaying the poll results.
+ *
+ * @see poll_menu()
  */
 function poll_votes($node) {

Here the return value should specify whether it's returning a render array or HTML.

m)

 /**
- * Callback for the 'results' tab for polls you can vote on
+ * Page callback: Displays the 'results' tab for polls you can vote on.
+ *
+ * This page displays a summary of the votes that have been cast.
+ *
+ * @param $node
+ *   The poll node object.
+ *
+ * @return
+ *   A page displaying the poll results.
+ *
+ * @see poll_menu()
  */
 function poll_results($node) {

I thought this was very confusing. The first line made me think it was returning results for multiple polls, but I think it is for just one poll node? And the tab is called both "tab" and "page". The return value also needs to tell me whether it's HTML or a render array.

batigolix’s picture

This is my first attempt at contributing to cleaning up API docs for core, so be gentle with your feedback ;)

I deliberately chose this issue to start as it seemed rather manageable. It also seemed to me that my possible errors are less critical in the poll module then elsewhere as the future of poll in core is uncertain ;).

I spend the largest part of the day learning to do this (api docs, doxygen, reroll patch), and struggled especially with applying the patches in this issue to my drupal 8 installation. I hope the patch has been generated correctly. It seems to contain both the changes from patch 18 & mine.

I applied all of Jennifer's comments from #20 and made minor style changes.

Crossing fingers

batigolix’s picture

Status: Needs work » Needs review

Set status to needs review

jhodgdon’s picture

Thanks! I'll review this sometime soon. Meanwhile, batigoix - the other thing you should do when you take over is assign the issue to yourself. :)

batigolix’s picture

Assigned: chertzog » batigolix

yes, that was on my todo list (that i never look at) :)

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch! Rather than trying to see if the previous review issues were addressed, I started over reviewing this patch from scratch. It looks pretty good, but there are a few things that need to be fixed:

a) poll.module

/**
- * Implements hook_form().
+ * Form constructor to display the poll choices.
+ *
+ * @see poll_more_choices_submit()
+ * @see poll_node_form_submit()
+ * @see poll_choice_js()
+ * @see _poll_choice_form()
+ * @see poll_teaser()
+ * @ingroup forms
  */
 function poll_form(Node $node, &$form_state) {

Actually, the previous documentation block here was correct. This is an implementation of hook_form(), and not a form constructor. The @ingroup forms line should be removed also. I also can't see how this is related to poll_teaser(), so that should be removed... We probably don't need the rest of the @see lines either, but I guess they don't hurt anything (they are actually related).

b) poll.module

+ * This handler is run regardless of whether JavaScript is enabled. 
+ * It makes changes to the form state. If the button is clicked
+ * with JavaScript disabled, then the page is reloaded with 
+ * the complete rebuilt form. If the button was clicked with JavaScript 
+ * enabled, then ajax_form_callback() calls poll_choice_js() to 

This section is over-agressively wrapped -- it should go as close to 80 characters as possible. Also there are a number of extra spaces at the ends of lines (most text editors for programming allow you to automatically remove these spaces or at least to show them).

c) Just below that, again I don't see how poll_teaser() is related to this function, so that @see (at least -- maybe all of them?) should be removed. The previous patch author seems to have added that @see poll_teaser() in a lot of places -- probably all can be removed. And generally, most of the @see lines could be removed -- see note #1 in comment #20 above. It's really not helpful to have so many see also lines...

d) This see from a few places in poll.module points to a function that does not exist:

+ * @see phptemplate_preprocess_poll_vote()

e)

+++ b/core/modules/poll/poll.pages.inc
@@ -2,11 +2,16 @@
 
 /**
  * @file
- * User page callbacks for the poll module.
+ * Page callbacks for the poll module.
  */

We consider module names to be proper nouns, so this should be "the Poll module".

jhodgdon’s picture

I also took a look at the other files in core/modules/poll that are not included in this patch. There are a few more things to be fixed:

1. Item (e) from my comment above (proper noun for module) needs to be applied to the file header in poll.install.

2. I also just noticed that in the patch, poll.module :: _poll_menu_access() is documented as a page callback. But it's actually an access callback.

3. The tests in core/modules/poll/lib/Drupal/poll/Tests need documentation updates (classes and methods are incorrectly documented or completely undocumented).

batigolix’s picture

Status: Needs work » Needs review
FileSize
15.45 KB

new attempt. incorporated your feedback

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good! A couple more fixes and I think we'll be done here:

1. Item (e) from #25 still isn't fixed:

+++ b/core/modules/poll/poll.pages.inc
@@ -2,11 +2,16 @@
 
 /**
  * @file
- * User page callbacks for the poll module.
+ * Page callbacks for the poll module.
  */

Should be "the Poll module".

2.

+++ b/core/modules/poll/lib/Drupal/poll/Tests/PollTestBase.php
@@ -9,6 +9,9 @@ namespace Drupal\poll\Tests;
 
 use Drupal\simpletest\WebTestBase;
 
+/**
+ * Tests creating a poll
+ */ 
 class PollTestBase extends WebTestBase {

SInce the name of this class is PollTestBase, I think it's actualy a base class for the poll tests, not a test itself?

3.

+++ b/core/modules/poll/lib/Drupal/poll/Tests/PollJsAddChoiceTest.php
@@ -10,7 +10,7 @@ namespace Drupal\poll\Tests;
 use Drupal\simpletest\WebTestBase;
 
 /**
- * Test adding new choices.
+ * Test adding new choices to a poll.
  */
 class PollJsAddChoiceTest extends WebTestBase {

- Test -> Tests
- This also needs to be done for PolLTokenReplaceTest.php

4. There are a couple more methods in test classes that don't have documentation:
PollTestBase::_generateChoices()
PollVoteTest - class and ::testPollVote() method

5. This method from PollVoteCheckHostnameTest.php needs a bit of fix-up (one line summary & correct verb tense):

  /**
   * Check that anonymous users with same ip cannot vote on poll more than once
   * unless user is logged in.
   */
  function testHostnamePollVote() {
 
batigolix’s picture

Status: Needs work » Needs review
FileSize
18.04 KB

new attempt

this incorporates the comments from #28

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011

The last submitted patch, poll_api_cleanup-1355712-29.patch, failed testing.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
18.07 KB

Re-rolled, applies cleanly at 96614a6

jhodgdon’s picture

Status: Needs review » Needs work

This is getting closer, thanks! A couple more things to fix:

a)

+  /**
+   * Tests creating, viewing, voting on recent poll block
+   */
   function testRecentBlock() {

missing . at end

b)

+  /**
+   * Tests creating, editing and closing a poll.
+   */
   function testPollClose() {

Needs serial comma before "and".

c)

}
-
+  /**
+   * Tests the expiration of a poll.
+   */
   function testAutoExpire() {

I don't think we want to remove that blank line? There are a couple of places that is done in this patch. We normally do want a blank line between methods in a class.

d)

+   * Checks that anonymous users with same ip cannot vote on poll more than
+   * once unless user is logged in.
    */
   function testHostnamePollVote() {

Summary needs to be one line. IP should be all-caps.

e)

+++ b/core/modules/poll/poll.install
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Install, update and uninstall functions for the poll module.
+ * Install, update and uninstall functions for the Poll module.
  */

Needs serial comma before and

f)

/**
- * Preprocess the poll_results theme hook.
+ * Preprocesses the variables for poll-results.tpl.php.

Should be:
Implements template_preprocess_HOOK() for poll-results.tpl.php.

g)

/**
- * Submit callback for poll_cancel_form().
+ * Form submission handler for poll_cancel_form().
+ *
+ * @ingroup forms
  */
 function poll_cancel($form, &$form_state) {

We don't add submission handlers to @ingroup forms.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
17.28 KB

Re-rolled. Applies cleanly at e4c2a57.

Status: Needs review » Needs work

The last submitted patch, poll_api_cleanup-1355712-34.patch, failed testing.

kid_icarus’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
17.32 KB

Thanks for the review @jhodgdon! Addresses #33.

kid_icarus’s picture

Whitespace cleanup from the last patch.

jhodgdon’s picture

Status: Needs review » Needs work

Sorry again for the delay in reviewing these patches... This looks mostly great, thanks! A couple of things to fix before I can commit this:

a)

- * @ingroup forms
- * @see poll_vote()
- * @see phptemplate_preprocess_poll_vote()
+ * @param $node
+ *   The poll node object.
+ * @param $block
+ *   (optional) TRUE if a poll should be displayed in a block. Defaults to
+ *   FALSE.
  */
 function poll_view_voting($form, &$form_state, $node, $block = FALSE) {

The @ingroup needs to be put back here, and probably the @see lines too?

b)

/**
- * Validation function for processing votes
+ * Form validation handler for poll_view_voting().
+ *
+ * @see poll_view_voting()
  */
 function poll_view_voting_validate($form, &$form_state) {
   if ($form_state['values']['choice'] == -1) {
@@ -736,7 +763,9 @@ function poll_view_voting_validate($form, &$form_state) {
 }
 
 /**
- * Submit handler for processing a vote.
+ * Form submission handler for poll_view_voting().
+ *
+ * @see poll_view_voting()
  */
 function poll_vote($form, &$form_state) {

There should be an @see to the submission handler from the validation handler, and vice versa. It's also not necessary (or advisable) to have @see poll_view_voting() if poll_view_voting() is mentioned in the first line.

c)


 /**
- * Themes the voting form for a poll.
- *
- * Inputs: $form
+ * Preprocesses variables for poll-vote.tpl.php.
  */
 function template_preprocess_poll_vote(&$variables) {

Should say "Implements template_preprocess_HOOK() for poll-vote.tpl.php".

Other than that, looks great, thanks!

izus’s picture

Status: Needs work » Needs review
FileSize
17.33 KB

Hello,
here is a new version of the patch with recommanded modifications of #38.
hope it helps

jhodgdon’s picture

Can you make an interdiff? http://drupal.org/node/1488712

izus’s picture

FileSize
1.27 KB

Hi,
Here is the interdiff corresponding the the last patch in #39.

jhodgdon’s picture

Status: Needs review » Needs work

OK, this patch is almost there! The only thing I see now that needs to be fixed is that @ingroup and @see need to be at the end of documentation blocks (@ingroup last) -- applies to poll_view_voting().

izus’s picture

hi,
here is the corresponding patch and interdiff.

EDIT : please forget that patch, the interdiff is ok but the patch is not anly patching the poll module ! my bad !

izus’s picture

Status: Needs work » Needs review
FileSize
707 bytes
17.37 KB
jhodgdon’s picture

Assigned: batigolix » Unassigned
Status: Needs review » Active

Thanks all! Committed this patch in #44 to 8.x.

I took a look at the files in the Poll module and sub-directories, and I think we now need a follow-up to address the following remaining issues:

a) poll-vote.tpl.php - missing a header before the list saying "Available variables:" -- see http://drupal.org/node/1354#templates - and actually the poll-results.tpl.php file is a bit non-standard there too.

b) Both of these templates need a change to the first line of the @file too, see that link in (a).

*** In poll.module:

c) _poll_menu_access() should have @see to poll_menu().
http://drupal.org/node/1354#menu-callback

d) poll_more_choices_submit() - this is a submission handler for the add another choice button in poll_form(); needs to say that in the documentation, and see:
http://drupal.org/node/1354#forms

e) poll_choice_js() - first line needs attention... this is an Ajax form callback, so it could say something like "Ajax callback for poll_form(): Adds new choices..."

f) poll_node_form_submit() - first line needs attention - this is an entity builder callback for poll_form(), so it could say "Entity builder callback for poll_form: Saves the poll choices and builds a teaser."

g) poll_view_voting() - blank line needed between @param and @see sections, and needs @see to its validation function.

h) poll_view_results() - needs @param/@return documentation.

*** Test files:

i) PollVoteCheckHostnameTest.php - missing class docblock.

j) PollTestBase::pollCreate() -- @return says "id" instead of "ID".

k) A few of the methods in PollTestBase are missing param/return documentation.

Lars Toomre’s picture

Note that #1811880: Add missing type hinting to Poll module docblocks is currently being refined to add type hinting to the appropriate @param and @return directives.

It is also picking up those docblocks with missing @param and/or @return directives. Not sure yet whether it captures all of those noted in #45. I will check once that issue is near ready to RTBC.

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

jhodgdon’s picture

Project: Poll » Drupal core
Version: 8.x-1.x-dev » 7.x-dev
Component: Documentation » documentation
Status: Active » Patch (to be ported)

Well, we still need a Core issue for 7.x, and since this was a Core initiative, I'd prefer to keep this issue for Core. (The patch above in #44 needs to be backported, and then additional issues addressed as outlined in #45.)

If the maintainers of the used-to-be-Core Poll module want to clean up the API docs, maybe they can file another issue?

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Issue summary: View changes
Status: Patch (to be ported) » Closed (fixed)

We're not working on these issues any more, so I'm setting back to 8.x / long ago fixed.