Clean up API docs for shortcut module

Yay! The docs sprint is now officially awesome.

+++ b/core/modules/shortcut/shortcut-rtl.cssundefined
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * Shortcut rtl styling
+ */

+++ b/core/modules/shortcut/shortcut.admin.cssundefined
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * Shortcut admin styling
+ */

+++ b/core/modules/shortcut/shortcut.admin.jsundefined
@@ -1,3 +1,8 @@
+/**
+ * @file
+ * Shortcut jquery behaviours
+ */

+++ b/core/modules/shortcut/shortcut.cssundefined
@@ -1,3 +1,8 @@
+/**
+ * @file
+ * Shortcut styling
+ */

I think these should probably have periods?

For the access callbacks (or menu callbacks generally), we probably want to add their paths and @see as per the new standard: http://drupal.org/node/1354#menu-callback.

Awesome. Two more things:

1. We can omit the @return for the access callbacks. It's always the same.
2. After looking at a few other sprint issues, I'm wondering if we should spell things out a little more in the @file directives. It seems to be preferred to say "...for the Foo module." E.g.:
   • RTL styling for the Shortcut module.
     Or, hmm. RTL should definitely be capitalized if we use it, but maybe we want to explain further?
     Right-to-left language styling for the Shortcut module.
     We should check and see what the HTML5 CSS cleanup docblocks are using, maybe?
   • jQuery behaviors for the Shortcut module.
   • Styling for the Shortcut module administration pages.
   • Styling for the Shortcut module.

   Etc. What do you think?

Well, per http://drupal.org/node/1354#menu-callback -- the parameter and return values are omitted for menu callbacks (like with hook implementations and form constructors), because they are predefined. In the case of access callbacks, it's always "TRUE if the user can access this path." But maybe in the example you found we should put clarification in the extended description instead?

Due to above comments, changing status so I don't keep looking to see if this needs a review. :)

Thanks! A few things still need to be fixed here:

a)

+ * Form validation handler for shortcut_set_switch().
+ *
+ * @see shortcut_set_switch()
  */
 function shortcut_set_switch_validate($form, &$form_state) {

The @see should point to the submit function, and the submit function should @see the validate function. See
http://drupal.org/node/1354#forms
This needs to be fixed in several other form/validate/submit function groups in the patch. If there is no validate, then the submit handler does not need @see at all.

Also, in a couple of places in the patch, it says "form submit handler" instead of "form submission handler"

b)

 /**
- * Menu page callback: builds the page for administering shortcut sets.
+ * Menu page callback: Builds the page for administering shortcut sets.

Menu page callback -> Page callback
(this is in a couple of places in the patch). See
http://drupal.org/node/1354#menu-callback

c)

 /**
- * Helper function for building a form for adding or editing shortcut links.
+ * Helper function: Builds a form for adding or editing shortcut links.

In other patches in this cleanup effort, we've gotten rid of saying "Helper function:". It's not really very informative. Can probably just omit it.

d)

+ * Access callback: Edits a shortcut set.
+ *
+ * Path: admin/config/user-interface/shortcut/%shortcut_set/%

This is not really the path. It looks like this is used for several paths -- list them rather than using "%" where it is not really part of shortcut_menu() -- use the exact string that's in hook_menu(). See
http://drupal.org/node/1354#menu-callback

Also, the first line should be "Access callback: Checks permission for editing a shortcut set." I think, since this function is not editing a shortcut set. This applies to the next few access callbacks too.

e)

 *
  * Title callback for the editing pages for shortcut sets.
  *
- * @param $shortcut_set
+ * @param object $shortcut_set
  *   An object representing the shortcut set, as returned by
  *   shortcut_set_load().
+ *
+ * @return string
+ *   The sanitized title of the shortcut set.
  */
 function shortcut_set_title($shortcut_set) {

This function should be documented as a hook_menu() title callback.

- * Form callback: builds the confirmation form for deleting a shortcut link.
+ * Form constructor for the confirmation for deleting a shortcut link form.

There are a few places in the patch that use this sentence structure, but this one illustrates the problem better than most of the others... I really have trouble reading and parsing the above sentence. How about "Form constructor for the confirmation form for deleting a shortcut link"? That's a little hard to read too, but it seems better. And the same pattern throughout the patch.

+ * Returns TRUE if the current user has access to a shortcut that contains the
+ * provided menu link

Needs a period at the end. Also, per #7, this sentence is incorrect; it should be "shortcut set" instead?

+ * @param array $menu_link
+ *   The menu link that wants to be accessed.

I do not believe that menu links are sentient beings (although sometimes after doing Drupal for a while I've had my doubts). How about "The menu link whose access will be checked"?

+ * @return string
+ *   The sanitized title of the shortcut set.
  */
 function shortcut_set_title($shortcut_set) {

Would be great to get #1363358: Shortcut set titles are double-escaped with check_plain() in; then we don't need the word "sanitized"...

Looks pretty close, but I think it's still missing a bit of the feedback from #11 ("form submit handler" vs. "form submission handler").

Also, the @see statements for shortcut_set_edit_form_validate() and shortcut_set_edit_form_submit() look like they're reversed...

Finally:

- * Pre-render function for adding shortcuts to the toolbar drawer.
+ * Adds shortcuts to the toolbar drawer.
+ *
+ * This is a pre render function.

Is the above really the correct way to do it?

An example of how we've been doing those is in http://drupal.org/node/1347890#comment-5284442. I don't think we've put it in 1354 yet though.

It's in 1354 now (reviews welcome):
http://drupal.org/node/1354#render

#17 looks good to me. I wonder if we should put the paragraph about how the callback is defined immediately after the summary for consistency? But a minor question and OT here. :)

Probably a good idea, but that wasn't how it was done in the example I copied from the other patch.

We recently had a discussion on [#1315992]and we have (unfortunately) changed the standard for hook_menu() callbacks:
http://drupal.org/node/1354#menu-callback

So this patch will need an update.

tagging

Patch according to the new standards for hook_menu() callbacks.

Thanks @vaidik for removing those!

I found a few other things:

1. +++ b/core/modules/shortcut/shortcut.admin.incundefined
   @@ -750,12 +776,16 @@ function shortcut_link_delete_submit($form, &$form_state) {
   + * Path: admin/config/user-interface/shortcut/%shortcut_set/add-link-inline
   

   This path still needs to be removed.

2. +++ b/core/modules/shortcut/shortcut.moduleundefined
   @@ -207,15 +207,16 @@ function shortcut_block_view($delta = '') {
   - *   TRUE if the current user has access to edit the shortcut set, FALSE
   - *   otherwise.
   + *   A boolean value.
   
   @@ -262,8 +265,9 @@ function shortcut_set_delete_access($shortcut_set) {
   - *   TRUE if the current user has access to switch the shortcut set of the
   - *   provided account, FALSE otherwise.
   + *   A boolean value.
   

   I am actually not sure these are legitimate changes. It seems to be replacing an infomative description with a vague and unhelpful one.

3. +++ b/core/modules/shortcut/shortcut.moduleundefined
   @@ -230,14 +231,16 @@ function shortcut_set_edit_access($shortcut_set = NULL) {
   + *   A boolean value. TRUE f the current user has access to delete shortcut
   

   Typo: "TRUE f."

4. +++ b/core/modules/shortcut/shortcut.moduleundefined
   @@ -288,7 +292,16 @@ function shortcut_set_switch_access($account = NULL) {
   + * @param array $menu_link
   + *   The menu link whose access will be checked.
   + *
   + * @return
   + *   A boolean value. TRUE if the current user has access to a shortcut set
   + *   that contains the provided menu link.
   

   Hmm, if we are adding the data type to the @param here, shouldn't we add it on the @return too? Though IIRC we wanted to leave those off the sprint. Either way. :)

I'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!

This looks really good -- and thanks for picking it up again! I only found a couple of things that need to be fixed:

a)

+++ b/core/modules/shortcut/shortcut.admin.inc
@@ -507,7 +522,10 @@ function _shortcut_link_form_elements($shortcut_link = NULL) {
 }
 
 /**
- * Validation handler for the shortcut link add and edit forms.
+ * Form validation handler for the shortcut link add and edit forms.
+ *
+ * @see shortcut_link_add_submit()
+ * @see shortcut_link_edit_submit()
  */
 function shortcut_link_edit_validate($form, &$form_state) {

This needs to have a link to shortcut_link_edit() and shortcut_link_add(). Normally that would be in the first line ("Form validation handler for shortcut_link_add() and shortcut_link_edit().").

b) By convention, form constructor, validation, and submission handler functions do not contain @param/@return for the standard parameters and return values, so those should be removed (there are a lot in the admin.inc file). See http://drupal.org/node/1354#forms

c) Could we improve the description of this parameter (actually say what it is):

// shortcut.admin.inc
  * @param $shortcut_set
  *   Returned from shortcut_set_load().
+ *
+ * @see shortcut_menu()
  */
 function shortcut_link_add_inline($shortcut_set) {

d) shortcut.module

@@ -588,7 +603,7 @@ function shortcut_sets() {
 }
 
 /**
- * Check to see if a shortcut set with the given title already exists.
+ * Checks if a shortcut set with the given title already exists.

if -> whether

e) I took a quick look at some of the other files in the core/modules/shortcut directory that are not included in this patch. They all look good, except that a few files have a capitalization problem in their @file headers. Since modules are considered proper nouns, they should say "the Shortcut module", rather than "shortcut module", as in this example in shortcut.admin.css (the other CSS files and shortcut.install have the same problem):

/**
 * @file
 * Admin styling for shortcut module.
 */

The most recent test run above (about 8 hours ago) said that this test failed:
Drupal\statistics\Tests\StatisticsBlockVisitorsTest->testIPAddressBlocking()

So I'm going to preserve that test run and file an issue (in case there's an actual problem), and re-upload the patch above so we can try the tests again.

#33: shortcut_doc_cleanup-29.patch queued for re-testing.

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!