The following methods in node.module are missing the return type in their PHPDoc comments:

  • node_title_list
  • node_revision_delete
  • node_is_page
  • node_get_recent
  • node_view
  • node_view_multiple
  • node_access_needs_rebuild
CommentFileSizeAuthor
#9 interdiff-7-9.txt1.61 KBnicrodgers
#9 2599156-9.patch2.59 KBnicrodgers
#7 interdiff-2599156-2-7.txt822 bytesnicrodgers
#7 2599156-7.patch3.46 KBnicrodgers
#5 interdiff-4-2.txt486 bytessnehi
#4 2599156-4-D8.patch2.41 KBsnehi
#2 2599156-020-user-module-return-types.patch2.41 KBnicrodgers

Comments

nicrodgers created an issue. See original summary.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
commented
Status: Active » Needs review
StatusFileSize
new 2599156-020-user-module-return-types.patch2.41 KB

Patch attached.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks! A few items to address:

  1. +++ b/core/modules/node/node.module
@@ -730,7 +730,7 @@ function node_user_predelete($account) {
+ * @return array
  *   An array of node entities or an empty array if there are no recent nodes

    This should be more specific than "array" for the type. Should be something like

    \Name\Of\The\Interface[] 

  2. +++ b/core/modules/node/node.module
@@ -1081,7 +1081,7 @@ function node_query_node_access_alter(AlterableInterface $query) {
+ * @return mixed
  *   The current value of the flag if no value was provided for $rebuild.

    A flag is normally a Boolean value?

snehi’s picture

snehi commented
Status: Needs work » Needs review
StatusFileSize
new 2599156-4-D8.patch2.41 KB

Hi jhodgdon,
Thanks for the review, i am not able to understand what have your written for 1.
Meanwhile changing 2 as mentioned by you.

snehi’s picture

snehi commented
StatusFileSize
new interdiff-4-2.txt486 bytes

Adding interdiff.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Regarding comment #3 item 1 see:
https://www.drupal.org/node/1354#types

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2599156-7.patch3.46 KB
new interdiff-2599156-2-7.txt822 bytes

Thanks for reviewing jhodgson and the feedback. I hadn't seen that doc page before, so it was really useful to see when we should mention the interface - thanks!

I've updated the patch from #2 to include your first comment, but with regards to the second comment and snehi's patch, I'm not sure it'd be accurate to say it returns boolean, as the function will only return a boolean if no value was provided for $rebuild (as per the comment). If a value for $rebuild is provided, the return statement is omitted. As per the PHP manual, "If the return() is omitted the value NULL will be returned". I've tried to address this in the updated patch by specifying return type as either bool|null which is more specific than the original "mixed".

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Thanks, looking better! Still needs a bit of improvement though.

  1. +++ b/core/modules/node/node.module
@@ -1081,7 +1081,7 @@ function node_query_node_access_alter(AlterableInterface $query) {
+ * @return bool|null
  *   The current value of the flag if no value was provided for $rebuild.

    I agree that this is the right type designation on the return line.

    Can we also add a sentence to the return documentation that follows, saying something like:

    If a value was provided for $rebuild, nothing is returned. 

  2. +++ b/core/modules/system/system.module
@@ -443,6 +441,7 @@ function system_authorized_batch_processing_url(array $options = array()) {
+ * @return RedirectResponse

    This line is still not quite right.

    First, if you put a class in a return value type, you must include the namespace. This class has no namespace.

    Second, if you make a @return line, it must have documentation, and this one doesn't.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
commented
StatusFileSize
new 2599156-9.patch2.59 KB
new interdiff-7-9.txt1.61 KB

Thanks for the comments @jhodgdon. I've added a sentence to the comment as per (1) in your feedback.

I also realised that I erroneously included changes to system.module as part of the patch on this issue. I was working on that separately (and there are still quite a few other phpdoc changes needed in that file), so I've removed those changes from this latest patch, and I'll create a new issue for it.

This interdiff therefore reflects the new sentence, and the removal of the system.module changes. Hope that's ok.

nicrodgers’s picture

nicrodgers
Primary language English
Location Monmouthshire, UK
commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good now, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed bfef184 and pushed to 8.0.x. Thanks!

  • alexpott committed bfef184 on 8.0.x 
    Issue #2599156 by nicrodgers, snehi, jhodgdon: Several methods in node....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.