Clean up API docs for openid

Thanks for the patch! I noticed a couple of things:

a)

/**
- * Creates a js auto-submit redirect for (for the 2.x protocol)
+ * Creates a JavaScript auto-submit redirect for (for the 2.x protocol)

Needs to end in ".".

b)

+ * @return $identifier

Should not have $identifier there. You can put a data *type* on the @return line (like string, array, boolean, etc.), but not a variable name. This problem occurs several times in the patch.

c)

+ *   The rel attribute of the link element you are targetting.

Spell check -- targeting should only have one T in the middle.

d)

+ * @param $association
+ *   Object loaded from openid_association or openid_server_association table. 

Could use some "the" in there. Also normally we put database table names in {} brackets.

e)

+ * @param array $message_array
+ *   The entire message about to be sent
+ * @param $keys_to_sign
+ *   Keys in the message to include in signature (without 'openid.' appended)

Both param descriptions need to end in "." This problem appears several times in the patch.

f)

+/**
+ * Gets a binary version of the provided shared secret, and runs an exclusive OR 
+ * operation on it.

The initial description needs to be cut down to one 80-character line. This problem occurs at least a couple of times in the patch.

g)

 /**
  * Calls the mul function from the available math library for OpenID.

Try to avoid abbreviations in documentation -- maybe this should be multiply? I'm not sure about this... maybe "mul", "div", "cmp", etc. are OpenID standards -- in which case the technical terms can be used I guess... but I think the docs would be clearer with "multiply", "divide", "compare", etc.

h)

+/**
+ * Helper for hook_form_FORM_ID_alter() implementations.

Should start with a verb.

i)

 /**
- * Login form _validate hook
+ * Form validation handler for the user login form.
+ *
+ * Sets up redirection and starts the OpenID authentication process.
+ *
+ * @param $form
+ *   Nested array of form elements that comprise the form.
+ * @param $form_state
+ *   The state of the current form.
  */
 function openid_login_validate($form, &$form_state) {

See http://drupal.org/node/1354#forms

j)

+/**
+ * Generate an OpenID association request.

Wrong verb tense. Applies to next doc block too.

k)

 /**
- * Remove expired nonces from the database.
- *
  * Implementation of hook_cron().
+ *
+ * Remove expired nonces from the database.
  */
 function openid_cron() {

Implementation of -> Implements

l)

 /**
  * Menu callback; Manage OpenID identities for the specified user.

See http://drupal.org/node/1354#menu-callback

m)

 /**
- * Menu callback; Delete the specified OpenID identity from the system.
+ * Form constructor: Delete the specified OpenID identity from the system.

See http://drupal.org/node/1354#forms

tagging.

a) pretty sure I got all of those
b) replaced most of those with $string, if any of those are still wrong please say so.
c) Easy fix
d) Got that too
e) managed to find all the missing periods (I think)
f) managed to cut down the longer comments but I'm unsure as to wether they are better or not
g) didn't touch this because I was unsure if it was openid terminology or not
h) rewrote the line hopefully for the better
i) removed the @param's because I thought that is what you meant
j) used generates
k) another easy one
for both l) and m), I didn't have the time to get to those so if someone would be willing to finish what I started that would be much apreciated, otherwise I'll take another look at it some other time.
thanks ZenDoodles for the help and thanks jodgdon for the notes.

Looks like it needs to be rerolled. (LearnDrupal.org has a good reference on rerolling if the reroll turns out to be difficult). Could you also provide an interdiff? Thanks @Zgear!

I tried to reroll it but patch said that it was malformed. Couldn't spot why.

redid it manually and did a git diff, also fixed the last of the issues with it I believe.

Zgear,
I started to review this, but there are peculiar issues like doc blocks pasted onto the incorrect function. It probably came from manually editing the first patch and/or trying to apply the patch by hand. Let's try again...

1. Get a fresh 8.x checkout
2. Apply the patch in #5
3. Make the changes suggested by jhodgdon in #6 to the now patched 8.x checkout.
4. Create your diff.
5. Create an interdiff

See also the awesomeness xjm wrote here: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

+++ b/core/modules/openid/openid.incundefined
@@ -80,7 +85,12 @@ function openid_redirect_http($url, $message) {
+ * @param array $message
+ *   Array of query string variables.Creates a js auto-submit redirect for (for the 2.x protocol)

Did you paste this badly? Otherwise it needs some rephrasing and at least one more space...

+++ b/core/modules/openid/openid.incundefined
@@ -296,7 +351,15 @@ function _openid_url_normalize($url) {
+ * Normalizes URL identifiers.
+ *
+ * This is one of this module's OpenID normalization methods.
+ *
+ * @param $identifier
+ *   The identifier to normalize.
+ *
+ * @return string
+ *   The normalized identifier.

This is probably not in the right place. The result of manually applying a patch perhaps?

+++ b/core/modules/openid/openid.incundefined
@@ -363,7 +446,10 @@ function _openid_nonce() {
- * Pull the href attribute out of an html link element.
+ * Generates a nonce value, formatted per OpenID spec.
+ *
+ * @return array
+ *   The generated nonce.

Again, probably misplaced.

There is also a fair chance the original patch will need to be rebased. See this lesson on rerolls on learndrupal.org for detailed instructions.

Attaching 3 files:
Rebased first patch to apply against current 8.x.
Patch with applied changes from #6.
Interdiff between the two.

Thanks zendoodles for doing most of this, I only did the rebasing part.

Thanks for the rebasing work! I took a look at the final patch from #19, and needs some work:

a) openid.inc

@@ -80,7 +85,12 @@ function openid_redirect_http($url, $message) {
 }
 
 /**
- * Creates a js auto-submit redirect for (for the 2.x protocol)
+ * Creates a JavaScript auto-submit redirect for (for the 2.x protocol).

Wording is bad here: "...for (for..."

b)

@@ -296,7 +352,16 @@ function _openid_url_normalize($url) {
 }
 
 /**
- * Create a serialized message packet as per spec: $key:$value\n .
+ * Creates a serialized message packet as per spec: $key:$value\n .

We try to avoid abbreviations like "spec" in docs [this applies to several other places in the patch too!], and anyway what "spec" is this following? The next function in the file refers to this "serialized message packet as per spec" as both a "direct communication message" and a "serialized message" (without the word "packet"). The two functions should use one consistent wording.

Also, there is no space added after \n in the code, so the way this line is written is confusing to me... How about actually putting the information about the return value ($key:$value\n) into the @return section, and making sure it doesn't have a confusing space in it?

c)

+ * @param $keys_to_sign
+ *   Keys in the message to include in signature (without 'openid.' appended).
+ *
+ * @return
+ *   The base64 encoded signature.
  */
 function _openid_signature($association, $message_array, $keys_to_sign) {

This @param needs some "the" etc. added to it so it reads more like English. :)

d)

@@ -547,6 +731,15 @@ function _openid_get_bytes($num_bytes) {
   return $bytes;
 }
 
+/**
+ * Obtains the response from the OpenID provider.
+ *
+ * @param $str
+ *   The string to be populated with raw response data.
+ *
+ * @return array
+ *   The prepared response data.
+ */
 function _openid_response($str = NULL) {

I don't think the @param doc is correct here. I see what the function is doing, but $str is not passed by reference, so there is no way it will be "populated" in such a way that the calling function can see it, right? It seems like a function bug, but really the $str argument is I think really quite useless...

e)

@@ -692,7 +896,11 @@ function openid_extract_ax_values($values, $uris) {
 }
 
 /**
- * Determine the available math library GMP vs. BCMath, favouring GMP for performance.
+ * Determines the available math library GMP vs. BCMath, favouring GMP for
+ * performance.

Needs to be shortened to one 80-character line. If necessary, put additional information into a new paragraph.

f) openid.module now...

 /**
- * Login form _validate hook
+ * Form validation handler for the user login form.
+ *
+ * Sets up redirection and starts the OpenID authentication process.
+ *
+ * @see openid_login_form_submint()
  */
 function openid_login_validate($form, &$form_state) {

- First line should use the form function name, not "the user login form".
- typo -- submint -> submit in the last line... oh, actually I don't think that function exists at all?
- I think this could use an @see to the two user login form alter functions in this file, and they should have an @see here and to the submit function (if there is actually one).

g)

@@ -300,14 +315,18 @@ function openid_login_validate($form, &$form_state) {
 }
 
 /**
- * The initial step of OpenID authentication responsible for the following:
+ * The initial step of OpenID authentication.
+ * Setup of the OpenID authentication is responsible for the following:
  *  - Perform discovery on the claimed OpenID.
  *  - If possible, create an association with the Provider's endpoint.
  *  - Create the authentication request.
  *  - Perform the appropriate redirect.
  *
- * @param $claimed_id The OpenID to authenticate
- * @param $return_to The endpoint to return to from the OpenID Provider
+ * @param $claimed_id
+ *   The OpenID to authenticate
+ * @param $return_to
+ *   The endpoint to return to from the OpenID Provider

- First line should start with a verb.
- List formatting is not correct here (wrong indent).
- param descriptions should end in .

h)

@@ -368,12 +387,13 @@ function openid_begin($claimed_id, $return_to = '', $form_values = array()) {
 }
 
 /**
- * Completes OpenID authentication by validating returned data from the OpenID
- * Provider.
+ * Completes authentication by validating response from the OpenID Provider.
...
+ * @return
+ *   Response values for further processing with
  *   $response['status'] set to one of 'success', 'failed' or 'cancel'.
  */
 function openid_complete($response = array()) {

- First line needs a "the" in there.
- Return section needs rewrapping to near-80-character lines
- We put commas before and/or in a list of items (success, failed, or cancel)

i)

@@ -443,7 +463,7 @@ function openid_complete($response = array()) {
 }
 
 /**
- * Perform discovery on a claimed ID to determine the OpenID provider endpoint.
+ * Performs discovery on claimed ID to determine the OpenID provider endpoint.

Needs "the"

j)

@@ -479,7 +499,7 @@ function openid_discovery($claimed_id) {
 }
 
 /**
- * Implementation of hook_openid_discovery_method_info().
+ * Implements of hook_openid_discovery_method_info().

Remove "of"

k)

+ * @param $claimed_id
+ *   The Extensible Resource Indicator (XRI) to attempt discovery on.

Ah finally, someone defined what XRI is! It would be great if this was added to the other places in the docs where XRI is used without defining the acronym. Similar with other acronyms.

l)

@@ -532,13 +555,17 @@ function _openid_xri_discovery($claimed_id) {
 /**
  * OpenID discovery method: perform a XRDS discovery.
...
  * @return
  *   An array of discovered services and claimed identifier or NULL. See
  *   openid_discovery() for more specific information.

- First line needs to start with a verb.
- @return section needs a rewrite. I can't figure it out at all.

m)

@@ -696,9 +725,10 @@ function openid_association($op_endpoint) {
 }
 
 /**
- * Authenticate a user or attempt registration.
+ * Authenticates a user or attempt registration.

attempt -> attempts

n)

+++ b/core/modules/openid/openid.pages.inc
@@ -24,7 +24,13 @@ function openid_authentication_page() {
 }
 
 /**
- * Menu callback; Manage OpenID identities for the specified user.
+ * Page callback: Manages OpenID identities for the specified user.
+ *
+ * @param object $account
+ *   The user account object to manage.
+ *
+ * @return
+ *   A render array for user/%user/openid containing a list of user identities.
  */
 function openid_user_identities($account) {

See http://drupal.org/node/1354#menu-callback for specs on how to document page callbacks. In particular, do not mention a path specifically.

o)

/**
- * Menu callback; Delete the specified OpenID identity from the system.
+ * Form constructor for the delete OpenID identity confirmation form.
+ *
+ * @param object $account
+ *   The account to delete the identity from.
+ * @param int $aid
+ *   The ID of the OpenID identity to delete.
+ *
+ * @see openid_user_delete_form_submit()
+ * @ingroup forms
  */
 function openid_user_delete_form($form, $form_state, $account, $aid = 0) {

Needs @see to openid_menu() added.

p)

+/**
+ * Form submission handler for openid_user_delete_form().
+ *
+ * @see openid_user_delete_form()
+ */
 function openid_user_delete_form_submit($form, &$form_state) {

Remove @see. Redundant with first line.

Here's a reroll of the one above. Once this passes, I'll upload the comments and an interdiff.

#23: openiddocs-1405948-23.patch queued for re-testing.

(sorry, I'm not finding lots of spare time to review these issues and a patch got committed yesterday that probably conflicts with this one; this one probably needs a reroll)

Re-roll.

This mostly looks pretty good, thanks! I had a couple of concerns:

Starting with openid.inc...

a) I'm wondering about:

+ * @param $url
+ *   The URL to redirect to.
+ * @param array $message
+ *   Array of query string variables.

This appears in several places in the patch... Do you think the docs for $message are sufficient to tell people what they are? I looked at one of the functions' code, and found it that it needs to be an associative array of name => value, and furthermore that the values are url-encoded by the function. None of this is in the documentation. Also, we might as well make the first line @param string $url (kind of funny to add the type on the $message param but not the $url param).

b)

 /**
- * Determine if the given identifier is an XRI ID.
+ * Determines if the given identifier is an XRI ID.

What is an XRI? I have no idea. Acronym should probably be spelled out (on the theory that if I don't know what it is, probably I'm not the only one). URL and HTML and PHP are acronyms we should all know, but I'm not sure about XRI.

Oh, later in the patch I found this explanation of what an XRI is:
"The Extensible Resource Indicator (XRI) to attempt discovery on." -- let's use that. Wherever XRI appears in function docs, it should first be spelled out within that function doc (and later uses within the same function doc can use the acronym). Or at least *somewhere* in the function doc it should be spelled out (there may not, for instance, be room in the first line of the function).

c) It doesn't look to me as though _openid_nonce() returns an array. I think it's a string?

+ * @return array
+ *   The generated nonce.
  */
 function _openid_nonce() {

d) Fix wrapping:

+ * @param $keys_to_sign
+ *   Keys in the message to include in the signature
+ *   (without 'openid.' appended).
+ *
+ * @return
+ *   The base64 encoded signature.
  */
 function _openid_signature($association, $message_array, $keys_to_sign) {

e) We should fix this parameter name -- it's a long but the parameter is called $str. As the function is only one line long, fixing it could be considered a documentation change.

+ * @param $str
+ *   The long.
+ *
+ * @return
+ *   The same value, converted to a base64 encoded string.
+ */
 function _openid_dh_long_to_base64($str) {
   return base64_encode(_openid_dh_long_to_binary($str));
 }

f) Here, the docs do not match the parameter name:

+ *
+ * @param $str
+ *   The long to convert.
+ *
+ * @return
+ *   The same value, converted to a binary string.
+ */
 function _openid_dh_long_to_binary($long) {

g) Needs to end in .

 /**
- * Determine the available math library GMP vs. BCMath, favouring GMP for performance.
+ * Determines the available math library

h) How about saying @return string The name of the math library, either 'gmp' or 'bcmath'. ?

+ * @return
+ *   The math library that is to be used.
  */
 function _openid_get_math_library() {

i) Now in openid.module:

+ * @param $claimed_id
+ *   The OpenID to authenticate.
+ * @param $return_to
+ *   The endpoint to return to from the OpenID Provider.
  */
 function openid_begin($claimed_id, $return_to = '', $form_values = array()) {

Misisng the third parameter here.

j)

+ * @return
+ *   $response['status'] - 'success', 'failed', or 'cancel'.
  */
 function openid_complete($response = array()) {

Not accurate. The function returns $response, after changing it in various ways.

Here is the patch file. I have corrected the file #29 comments. Thanks to @jhodgdon. Please review it.

Status needs review

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!