Add missing type hinting to Overlay module docblocks

I will try to answer as best I can.

a) The correct form for optional variables is to start the description with '(optional)' and make sure to state what the default value is like 'Defaults to FALSE.

b) Look at the type hints defined on http://drupal.org/node/1354. Mixed is used for more than two type hints. One never should use ' string or null'. Instead, that case should be written as 'string|null'.

c) Yes, that should be the PHP type string. Again, look at http://drupal.org/node/1354 for more info on the basic types like 'string', 'array', 'int' and 'bool'. Note the standard for the last two; they are not fully spelled out.

@bleen18 Can upload again the patch in #4 using a different name like '1816840-XX.patch'.

When trying to download and apply the patch from #4, I was getting the patch from #1 since they are named the same. Hence, I could not apply the right patch to check everything locally.

Thanks in advance.

Thanks @bleen18 for your work in creating patches for this issue. I have now downloaded and locally installed the patch from #6.

Here is another nit-picky review so that we can get to a pristine patch for RTBC status. Thanks for bearing with me in this process:

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -313,7 +313,7 @@ function overlay_page_alter(&$page) {
  *
- * @return
+ * @return bool
  *   TRUE if the user has permission to dismiss the accessibility message or if
  *   the user is anonymous. FALSE if otherwise.

This type hint addition to overlay_user_dismiss_message_access() is correct.

Checking all of the module for occurences of 'boolean', I note that we did not catch two issues of incorrect type hinting in the file overlay-parent.js. I am going to set this to 'needs work' for this reason.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -337,7 +337,7 @@ function overlay_user_dismiss_message_access() {
  *
- * @return
+ * @return array
  *   A render array for a page containing a list of content.

Please carefully review the overlay_user_dismiss_message() function. My reading of the code is that there should be no @return directive at all. What do you think?

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -373,6 +373,9 @@ function overlay_user_dismiss_message() {
  *
+ * @return array
+ *   A renderable array representing a message for disabling the overlay.
+ *
  * @see http://drupal.org/node/890284
  */
 function overlay_disable_message() {
@@ -428,12 +431,15 @@ function overlay_disable_message() {

I agree with this addition of @return directive to overlay_disable_message(). This is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -428,12 +431,15 @@ function overlay_disable_message() {
- * @param $variables
+ * @param array $variables
  *   An associative array with an 'element' element, which itself is an
  *   associative array containing:
  *   - profile_link: The link to this user's account.
  *   - dismiss_message_link: The link to dismiss the overlay.
  *
+ * @return string
+ *   The HTML for the message about how to disable the overlay.

The addition of the array type hint here in theme_overlay_disable_message() is correct.

The way theme() functions return statements are not like other functions. Our standards specificly call for no @return directive here so this addition should be removed.

Finally, I think we should open up a follow up issue to address the addition of array $variables to the function declaration. That issue could address many functions in core that are missing the array declaration. Or maybe we should just use #318016: Type hint array parameters which has been around forever. Thoughts?

The functions overlay_preprocess_html(), overlay_preprocess_maintenance_page(), template_preprocess_overlay() probably need to document the @param array $variables like theme_overlay_disable_message(). I know that [#1354] does not call for it, but comments in other document issues has suggested that all preprocess functions need to follow the theme() example. What do you think?

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -574,12 +580,12 @@ function overlay_preprocess_page(&$variables) {
  *
- * @param $value
+ * @param bool|null $value
  *   By default, an empty page will not be displayed. Set to TRUE to request
  *   an empty page display, or FALSE to disable the empty page display (if it
  *   was previously enabled on this page request).

The type hint here in overlay_display_empty_page() is correct.

I note though that this is a variable with a default. Hence, the explanation needs to start with '(optional)' and explictly state 'Defaults to NULL.'

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -574,12 +580,12 @@ function overlay_preprocess_page(&$variables) {
- * @return
+ * @return bool
  *   TRUE if the current behavior is to display an empty page, or FALSE if not.

I agree with this @return type hint since NULL cannot be returned.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -630,8 +636,8 @@ function overlay_get_mode() {
  *
- * @param $mode
- *   To set the mode, pass in one of the following values:
+ * @param string $mode
+ *   (Optional) To set the mode, pass in one of the following values:
  *   - 'parent': This is used in the context of a parent window (a regular
  *     browser window). If set, JavaScript is added so that administrative
  *     links in the parent window will open in an overlay.

@@ -646,9 +652,9 @@ function overlay_get_mode() {
  *
- * @return
+ * @return string|null
  *   The current mode, if any has been set, or NULL if no mode has been set.

There are several things to address with this @param directive.

The type hint should be 'string|null' since it defaults to NULL.

In the explanation, it should be a lowercase '(optional)'. This change needs to be further down in the patch as well.

Finally, the list keys like 'parent' should be formatted as parent (i.e. no single or double quotes).

The type hint for the @return directive of this docblock is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -740,13 +746,13 @@ function overlay_overlay_child_initialize() {
  *
- * @param $redirect
- *   (optional) The path that should open in the parent window after the
+ * @param string $redirect
+ *   (Optional) The path that should open in the parent window after the
  *   overlay closes. If not set, no redirect will be performed on the parent
  *   window.

This @param directive in overlay_close_dialog() needs several changes:

The is a default variable so it should be 'string|null'.
'(Optional)' needs to start with lowercase 'o'.
Needs to state 'Defaults to NULL.'

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -740,13 +746,13 @@ function overlay_overlay_child_initialize() {
- *   (optional) An associative array of options to use when generating the
+ * @param array $redirect_options
+ *   (Optional) An associative array of options to use when generating the
  *   redirect URL.

This type hint addition is correct.

Please add 'Defaults to an empty array.' to its description.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -775,7 +781,7 @@ function overlay_close_dialog($redirect = NULL, $redirect_options = array()) {
  *
- * @return
+ * @return array
  *   An array of region names that correspond to those which appear in the
  *   overlay, within the theme that is being used to display the current page.

This type hint addition in overlay_regions() is correct based upon what _overlay_region_list() returns: an array.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -797,7 +803,7 @@ function overlay_regions() {
  *
- * @return
+ * @return array
  *   An array of region names that correspond to supplemental overlay regions,
  *   within the theme that is being used to display the current page.

This type hint addition in overlay_supplement_regions() is also correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -810,11 +816,11 @@ function overlay_supplemental_regions() {
  *
- * @param $type
+ * @param string $type
  *   The type of regions to return. This can either be 'overlay_regions' or
  *   'overlay_supplemental_regions'.

This type hint addition is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -810,11 +816,11 @@ function overlay_supplemental_regions() {
  *
- * @return
+ * @return array
  *   An array of region names of the given type, within the theme that is being
  *   used to display the current page.

This type hint addition to @return in _overlay_region_list() is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -835,7 +841,7 @@ function _overlay_region_list($type) {
  *
- * @return
+ * @return array
  *   An array containing the names of the regions that will be rendered when
  *   drupal_render_page() is called. If empty, then no limits will be imposed,
  *   and all regions of the page will be rendered.

This type hint addition in overlay_get_regions_to_render() is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -851,14 +857,14 @@ function overlay_get_regions_to_render() {
  *
- * @param $regions
+ * @param array $regions
  *   (Optional) An array containing the names of the regions that should be
  *   rendered when drupal_render_page() is called. Pass in an empty array to

This type hint is incorrect. Since the variable defaults to NULL, it should be 'string|null'.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -851,14 +857,14 @@ function overlay_get_regions_to_render() {
  *
- * @return
+ * @return array
  *   The current list of regions to render, or an empty array if the regions
  *   are not being limited.

This type hint addition in overlay_set_regions_to_render() is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -885,10 +891,10 @@ function overlay_set_regions_to_render($regions = NULL) {
- * @param $region
+ * @param string $region
  *   The name of the page region that should be rendered.
  *
- * @return
+ * @return string
  *   The rendered HTML of the provided region.

These type hint additions in overlay_render_region() are correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -934,7 +940,7 @@ function overlay_render_region($region) {
  *
- * @return
+ * @return array
  *   An array of all rendered HTML that was stored earlier in the page request,

This type hint addition to overlay_get_rendered_content() is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -951,15 +957,16 @@ function overlay_get_rendered_content() {
  *
- * @param $id
+ * @param string $id
  *   (Optional) An identifier for the content which is being stored, which will
  *   be used as an array key in the returned array. If omitted, no change will

This type hint addition in overlay_store_rendered_content() needs to be corrected. This vairable has a default so that it should be 'string|null'. Also the description should add 'Defaults to NULL.'

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -951,15 +957,16 @@ function overlay_get_rendered_content() {
+ * @param string $content
  *   (Optional) A string representing the rendered data to store. This only has
  *   an effect if $id is also provided.

This also is incorrect. See the comment above for the changes needed.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -951,15 +957,16 @@ function overlay_get_rendered_content() {
  *
- * @return
+ * @return array
  *   An array representing all data that is currently being stored, or an empty
  *   array if there is none.

This is a correct type hint addition.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -976,7 +983,7 @@ function overlay_store_rendered_content($id = NULL, $content = NULL) {
  *
- * @param $region
+ * @param string $region
  *   The name of the page region to refresh. The parent window will trigger a

This type hint addition is correct based upon what drupal_region_class() accepts as its input.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -1030,9 +1037,12 @@ function overlay_trigger_refresh() {
  *
- * @param $region
+ * @param string $region
  *   The name of the page region to render.

This type hint addition in overlay_ajax_render_region() is correct.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -1030,9 +1037,12 @@ function overlay_trigger_refresh() {
+ * @return obj
+ *   An http response containing the rendered markup of a single region.

Thanks for catching this missing @return directive. The type hint needs to be correct though. It should be: '\Symfony\Component\HttpFoundation\Response'.

======== Other observations with this patch locally applied:

1) As stated above, we need to look at the @param directives in overlay-parent.js. It also looks like there are six missing type hints for @return in this file too.

2) The docblock for overlay_user_dismiss_message() is missing a @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException''. This should be on its own line after a blank line after the @return directive.

3) Thanks again @bleen18 for rolling this patch. Let me know if you have any questions with any of my comments above.