Clean up API docs for overlay module

Tagging

I'm happy to help.

Updated description with more details.

I attempted to ensure that all files have an @file docblock that conforms to standards.

Setting the status to 'needs to review'.

Thanks @aLearner! Patch review:

+++ b/core/modules/overlay/overlay-child-rtl.cssundefined
@@ -1,4 +1,9 @@
+/**
+ * @file
+ * This is a JavaScript file that's titled 'overlay-child-rtl.css'
+ */

This is actually a CSS file, not a JavaScript file. However, it would be better to instead describe the file's purpose. In this case, I'd suggest:
RTL styling for Overlay child pages.

Let's also change the other docblocks similarly to describe the files' purposes. (If you aren't sure what to put, try looking at examples from other core modules.)

+++ b/core/modules/overlay/overlay-child.cssundefined
@@ -1,4 +1,9 @@
+ */
+ ¶

At the end of each docblock, you have a character of trailing whitespace. This will need to be removed. I suggest configuring your editor to display the trailing whitespace so it doesn't get added accidentally.

Once you've made those changes, you can create a new patch, like we did earlier today. (Make an additional commit on your overlay branch, and then do git diff 8.x to get the patch.) Then, you can submit your updated patch at Needs Review again, and if it looks good we'll move on to the next bullet point. Good luck!

aLearner,

I'm uploading a new patch that includes some todo's. I didn't go through all of the files in depth but rather glanced through them. I hope this is useful, but xjm/jhodgdon are the experts in this area.

interdiff from #7.

1. +++ b/core/modules/overlay/overlay.moduleundefined
   @@ -300,6 +300,8 @@ function overlay_page_alter(&$page) {
    /**
     * Menu callback; dismisses the overlay accessibility message for this user.
   + *
   + * @todo - Update this to current standards.
   

This should follow http://drupal.org/node/1354#menu-callback

2. +++ b/core/modules/overlay/overlay.moduleundefined
   @@ -383,6 +387,8 @@ function overlay_disable_message() {
     * Returns the HTML for the message about how to disable the overlay.
     *
     * @see overlay_disable_message()
   + *
   + * @todo - Update this to current standards.
     */
    function theme_overlay_disable_message($variables) {
   

I think this should follow http://drupal.org/node/1354#themeable or see http://drupal.org/node/1354#hookimpl for additional guidance.

3. +++ b/core/modules/overlay/overlay.moduleundefined
   @@ -473,6 +479,8 @@ function overlay_preprocess_maintenance_page(&$variables) {
     * Preprocesses template variables for overlay.tpl.php
     *
     * @see overlay.tpl.php
   + *
   + * @todo - Update this to current standards.
     */
    function template_preprocess_overlay(&$variables) {
   

I think this should follow http://drupal.org/node/1354#hookimpl

The rest of the @todo's in this document should be similar to the ones listed above.

will give this a shot

the attached patch implements the comments from #11

cannot reroll the patch from #8

What problem are you having rerolling the patch? See
http://drupal.org/patch/reroll

And if you no longer plan to work on this issue, please go ahead and un-assign it. Thanks for trying!

not going to give up, yet.

re-assiging this to myself

#16: doc-cleanup-overlay-module-1398404-16.patch queued for re-testing.

Apologies again for the delay! I'm finally getting back to reviewing this... It mostly looks really good! I only found:

a)

+++ b/core/modules/overlay/overlay.module
@@ -322,7 +322,7 @@ function overlay_user_dismiss_message_access() {
 }
 
 /**
- * Menu callback; dismisses the overlay accessibility message for this user.
+ * Menu callback; Dismisses the overlay accessibility message for this user.

That ; should be a :

b)

+ * @param $variables
+ *   An associative array containing:
+ *   - profile_link: The link to this user's account
+ *   - dismiss_message_link: Link to dismiss the overlay
+ *
+ * @ingroup themeable
  */
 function theme_overlay_disable_message($variables) {

These two list items need to end in ".". Also I think the second one should also start with "The"?... Hm. Actually, I don't think this is right. Looking at the code, it looks like $variables only has $variables['element']. Those two array items are part of $element. So it should probably say that the only array item is element, and the documentation would probably be 'A render element for the message'... however this type of render element isn't documented anywhere or even declared in a hook_element_info as far as I can tell, so I think it's rather bogus the way this theme function is set up... So probably it would be best to do something like:

An associative array with an 'element' element, which itself is an associative array containing:

(and then have the two items you had in there).

c)

/**
- * Preprocesses template variables for overlay.tpl.php
+ * Implements hook_preprocess_HOOK() for overlay.tpl.php
  *
- * @see overlay.tpl.php
+ * If the current page request is inside the overlay, add appropriate classes
+ * to the <body> element, and simplify the page title.
  */
 function template_preprocess_overlay(&$variables) {

Based on the name of this function, it's actually implementing template_preprocess_HOOK(), not hook_preprocess_HOOK().

d)

+ *
+ * @todo - Update this to current standards.
  */
 function template_process_overlay(&$variables) {

I guess this and the other @todo sections still need to be done?

e)

+++ b/core/modules/overlay/overlay.tpl.php
@@ -11,8 +11,8 @@
  *   (e.g., the view and edit tabs when displaying a node).
  *
  * Helper variables:
- * - $classes_array: Array of html class attribute values. It is flattened
- *   into a string within the variable $classes.
+ * - $classes_array: Array of html class attribute values. It is flattened into
+ *   a string within the variable $classes.
  *

html -> HTML

Remarks a) - e) from #19 were incorporated in this new patch

This is mostly great! A couple of minor fixes and we'll be done here:

a)

/**
- * Processes variables for overlay.tpl.php
+ * Implements template_process_HOOK() for overlay.tpl.php
  *
- * @see template_preprocess_overlay()
- * @see overlay.tpl.php
+ * Places the rendered HTML for the page body into a top level variable.
  */
 function template_process_overlay(&$variables) {
-  // Place the rendered HTML for the page body into a top level variable.
+  //
   $variables['page'] = $variables['page']['#children'];
 }

I agree with moving the comment into the documentation block, but rather than leaving an empty comment behind in the function body, just remove the line completely.

b) Also I don't see why the @see to the preprocess function was removed? That looks useful, and maybe a @see in the reverse direction should be added to the preprocess function?

c)

/**
- * Delivery callback to display an empty page.
+ * Callback to display an empty page.

I'm not sure what function this is documenting, but it should probably follow the standards of
http://drupal.org/node/1354#functions or
http://drupal.org/node/1354#callbacks or
http://drupal.org/node/1354#menu_callback
whichever is most appropriate. As it is, it doesn't seem to follow any standard.

attached patch incorporates the comments from #21

as for comment c) : I'm not sure what this documentation should look like. It seems an a-typical function and I don't know what example to follow

#22: doc-cleanup-overlay-module-1398404-22.patch queued for re-testing.

OMG this patch still applies -- committed to 8.x. THANKS!

Now for a bit of followup that is still needed:

overlay.module
- overlay_user_dismiss_message_access() - non-standard and too long first line -- see
http://drupal.org/node/1354#menu-callback
for standard -- it's a hook_menu() access callback.
- overlay_user_dismiss_message() -- same standard, it's a page callback.
- overlay_display_empty_page() - needs different first line... looking at the @return (and the way the function is defined and used), I think something like "Stores and returns whether an empty page override is needed" would be better?
- overlay_close_dialog() -- also needs a different first line. How about just "Requests that the overlay..." (taking out the "Callback to request..."?
- overlay_request_refresh() - verb tense first line
- overlay_request_page_refresh() - same
- overlay_trigger_refresh() - same

I'll see if I can dedicate some time for this in the next days, so leave this assigned to me.

If there is no action within a week, feel free to assign this to someone else

patch incorporates comments from #24

#27: doc-cleanup-overlay-module-1398404-26.patch queued for re-testing.

Thanks! Committed to 8.x. I think we're ready to port both the patch in #27 and #22 to 7.x now (they can be combined).

Backported #22 and #27 to D7.

Thanks, looks good! I'll get it committed shortly.

Thanks again to all who worked on this! This is now committed to 7.x.