Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a sub-issue of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* focused on correctly adding @param and @return type hinting to the Overlay module.
Documentation patches that include type hinting are time consuming to both review and commit because one must dig into the actual code to confirm that the type hints are both correct and complete. Hence, please be patient and try to limit type hint patches to covering only a limited number of docblocks (20-25 as a guess).
Related sprint issues:
Comment | File | Size | Author |
---|---|---|---|
#11 | overlay-add_missing_type_hinting_to_docblocks-1816840-11.patch | 7.56 KB | hgoto |
Comments
Comment #1
bleen CreditAttribution: bleen commentedI have a couple questions here so I'm adding this patch so I can use Dreditor to assist in asking...
Comment #2
bleen CreditAttribution: bleen commentedI have seen some places where "optional" is here and others where it is in the description immediately below. Which is correct?
is mixed correct for "string or null"
These are PHP types, right? not canonical types... In other words, this should be "string" and not "html", right?
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #4
bleen CreditAttribution: bleen commentedBased on the answers from #3 here is an updated patch...
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #6
bleen CreditAttribution: bleen commentedhmm ... the two patches do have two different URLs so that shouldnt happen... anyway...
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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:
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.
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?
I agree with this addition of @return directive to overlay_disable_message(). This is correct.
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?
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.'
I agree with this @return type hint since NULL cannot be returned.
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.
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.'
This type hint addition is correct.
Please add 'Defaults to an empty array.' to its description.
This type hint addition in overlay_regions() is correct based upon what _overlay_region_list() returns: an array.
This type hint addition in overlay_supplement_regions() is also correct.
This type hint addition is correct.
This type hint addition to @return in _overlay_region_list() is correct.
This type hint addition in overlay_get_regions_to_render() is correct.
This type hint is incorrect. Since the variable defaults to NULL, it should be 'string|null'.
This type hint addition in overlay_set_regions_to_render() is correct.
These type hint additions in overlay_render_region() are correct.
This type hint addition to overlay_get_rendered_content() is correct.
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.'
This also is incorrect. See the comment above for the changes needed.
This is a correct type hint addition.
This type hint addition is correct based upon what drupal_region_class() accepts as its input.
This type hint addition in overlay_ajax_render_region() is correct.
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.
Comment #8
nod_Overlay is dead to D8 #2088121: Remove Overlay.
Comment #9
Mile23Comment #10
naveenvalechaThat meta was for 8.1.x #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* and overlay is dead in 8.x So I it should not be the child of that meta.
Comment #11
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commentedThis issue may not be active any more but I wrote a patch for Drupal 7.x-dev with the latest API documentation standard. I'd like someone to review this patch. Or it's better to close this issue if it's not active, I think. Thank you.
Comment #12
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented