Documentation standard for @param and @return data types

Was this discussed anywhere before it was changed? I don't remember seeing anything about it and it would be a widespread change.

We already established previously that it was OK to do so, and it doesn't break API module: #648744: @return type definitions for auto-completion in IDEs

Based on that issue, webchick and I and a few others discussed it in IRC last night (since it was already the de facto standard) and I went ahead and made the change.

The format being used is from PHPDoc. Parentheses are not part of any documentation standard as far as I'm aware, so that's a total non-starter. This moves us closer to widespread industry standards.

I believe @todo is also part of PHPDoc.

Right now API.module is based on our own bastardized variant of Doxygen, which is a really lousy documentation format for a loosely typed language. We've already started to migrate closer to PHPDoc, so this is a natural extension of that.

Gah, now I have to change all my php docs again. Are we getting this right finally and won't make any major changes for a while? :)

side note on array vs Array, etc:

It's always 'array', 'stdClass' or 'NameOfClass'.

To have PHP documentation 'standard' that's Drupal-only is mildly insane, especially when it's some kind of almost-PHPDoc. I've always assumed that we were using PHPDoc, and I don't think that I'm alone in doing that. People will probably continue to assume that it's PHPDoc that's being used, basically because there aren't any obvious reasons to suspect that it's not. They will probably also be using editors and IDE:s that support PHPDoc, either by making it easier to write, or by providing completion/help from the doc-comments.

The Drupal community should always embrace standards when they exist, not bastardize them. And as a standards-hugging choice this one is actually a no-brainer – as we're almost following the standard already.

Sorry, but I'm not excited to do a whole-sale re-tweaking of our documentation standards during a code freeze. I support the amendment of the coding standards to reflect what already got into core, but any new initiative to change everything is going to need to wait until D8.

http://api.drupal.org/api/function/db_query/7 has been using typed parameters and return values for a while now.

For Drupal 7, we should be allowing typed parameters and encouraging them for anything OO, for the simple reason that it makes the code more self-documenting and enables useful auto-completion for IDEs.

webchick is correct that we should not be doing a total shift to PHPDoc at this point. D7 like D6 will be our own screwy hybrid. That said, for Drupal 8 I am very much in favor of moving to PHPDoc wholesale, and making the necessary adjustments to our coding formats (and writing extensions (not forks!!!) to PHPDoc if necessary). There's no good reason for us to be training up new PHP developers on a documentation format no one in the universe uses except us.

Also, as a point of fact, our coding/doxygen standards documentation is NOT versioned with versions of Drupal, and neither is the API module. Maybe they should be, but they aren't.

Right, but that's merely a limitation of the handbook tools. Major pushes, such as #245115: Fix Drupal's awkward coding standards for the . operator, only happen during code thaw, because it's a change that affects all module developers. It's too late to do any more such changes in Drupal 7.

The @todo thing sounds like it needs its own issue. This one is about param/return types. Personally, I'd advocate a feature request for API module to either strip these from display or do something more nice with them.

The examples in the phpDocumentor handbook appear to be using "array", so we should as well I suppose. I haven't found an example of "object" yet, but note that an object of class Foo is NOT also going to be an instance of stdClass. stdClass is not a global parent. So "object" would be more correct there as well.

The pipe delimiting makes sense as well if PHPDoc supports it, although I'm not certain if we want to go that far before D7 is tagged final.

I'm fine with 'array' and 'object' on stdClass().

Not sure about pipes. I've never seen that elsewhere. I would just used 'mixed'.

Or better yet, nothing at all, since that's what got into Drupal 7 before code freeze.

And we continue to just say @return $var and explain in the docs that it returns either a string or false.

At bare minimum we want to specify a return type if it's a class/interface. Any IDE that does autocompletion is going to need that for the entire D7 life cycle if you want to type db_query()-> and have it give you a list of the possible fetch methods.

Right. That's something that made it into D7, so that's fine. I was talking about the int|boolean stuff, which did not. But something to take up in D8.

Great, thanks jhodgdon!

3 people - partially even disagreeing - are too few to change this essential coding standard.

The coding standards were therefore weakened to an "optional recommendation" for the "non-obvious case", as stated earlier in this issue:

At bare minimum we want to specify a return type if it's a class/interface.

Bumping to 8.x for further discussion. And most importantly, tagging, as we don't have a dedicated component/queue for coding standards.

Note that I'm not opposed to this change. Actually, I was used to do that for all data types, prior to adhering to Drupal's current coding standards.

But if we want to change this from a opt-in "you may, if required" recommendation into a "you should" rule, then all or at least most of Drupal core should be updated to match this.

Furthermore, Coder's review rules would have to be adjusted to report the inverse of what it currently does, i.e. warn about missing data types.

@webchick: Actually, not sure why we bumped this to D8? If we manage to reach consensus here, then I guess this could be happily considered for D7 still? Sure, we don't want to waste time on this PRIOR to Drupal 7.0 ;) But perhaps soon after that magic, yet unknown date? :)

This change, if enforced, would require probably a 200K patch to core to bring us inline. That means similar changes in every single contributed module. We only do big changes like this during a code thaw as a result; while it's not an "API change" per se, it is a change that needs to be made to all PHPDoc headers in every module, and in that respect it qualifies.

subscribe (I'm sure I'll live to regret this decision)

Why this is important:

This is Netbeans, and each major IDE that I've tried as the same issue.

@mikey_p: +1, that picture is worth ten thousand words. Without it, you have no method/property autocomplete, and you have no quick jump to the class definition.

On the 'object' vs. 'stdClass' question, I just want to make sure that we're clear on the way to do this, which as far as I'm aware, is standard:

• object: any type of object; this is an intentionally non-type-specific identifier.
• classname: an object of the type named. therefore...
• stdClass: an object of type stdClass.

object != stdClass. Not not not. jhodgdon's example in #12 illustrates that perfectly - stdClass has no methods.

As far as I can tell, the php.net standard is actually "be as specific as you can with respect to class/interface types." For example:

http://us2.php.net/manual/en/class.iteratoriterator.php

specifies that its constructor takes a single argument of type Traversable. I don't know if this standard is de facto or de jure for php.net, but it's a very sensible one, IMO. Which means that the example you linked to, jhodgdon, might actually be wrong. DatabaseConnection::query() actually takes a mixed variable (string or an object) for its first parameter, $query. For the sake of discussion, though, let's say it just takes the object - in which case, db_query() should actually specify its type as DatabaseStatementInterface, because that's the most specific interface a good/expected parameter is guaranteed to satisfy.

And yes, I agree that these should be standards going forward, not something we should try to retroactively apply everywhere right away. Way too much ground to cover there.

OK, then agreed. I was only picking nits because phrasing like this (sorry webchick):

I'm fine with 'array' and 'object' on stdClass().

was concerning me, and I wanted to be sure it was all mutually understood. Maybe I just misread :)

Agreed with what's being said here. It looks like the docblock for db_query() in that example is just flat out wrong, since db_query() only takes a string as its first parameter. DatabaseConnection::query() may take an object or a string, but db_query() is just a string. That's a separate bug to check into elsewhere. :-)

#608124: Link function parameter types and return types to class/interface pages appears to be a duplicate of this, now that this one appears to have more traction and future relevance.

Ah, I had to reread it twice to figure out that you had essentially marked the issue fixed and then reopened it as a "make the class references in the web api documentation a web link that goes to the part of the web documentation where that class is explained" in the same post. I retract my posit that it might be a duplicate issue and will do my best to recover from the whiplash. :)

Could we get an consensus on this for D8 and higher?

Personally I'd really like to make the type required for D8 and higher, and I'm willing to work on patches that would do that for D8.

I'm ok and would be in favor of what @jhodgdon digged out in #11 -- to summarize:

 * @param object $myobject
 *   Any kind of object.
 * @param stdClass $myclass
 *   A class instance object of type "stdClass".
 * @param FooBarInterface $myclass
 *   Any kind of class instance object that implements FooBarInterface.
 * @param array $myarray
 *   An array.
 * @param int $myinteger
 *   An integer.
 * @param string $mystring
 *   A string.
 * @param bool|string $foo
 *   A Boolean to signify bar, or a string denoting which foo to do.
 * @param mixed $whatever
 *   Discouraged. "mixed" usually does not make sense for @params; except for
 *   Drupal's module_invoke() functions and similarly weird stuff.
 *
 * @return int|string
 *   An integer denoting what, or a string denoting ever.
 * @return Parser|false
 *   A class instance object of type Parser, or Boolean FALSE.
 * @return mixed
 *   Boolean TRUE on success, FALSE on failure, or an array of doom.

Agree. Thanks mikey_p for offering to work on a patch.

Could someone test in IDEs whether the int|string type declarations are properly picked up?

I know that phpDoc and also php.net uses this syntax, so it should be supported by IDEs.

We should also test what API module produces out of that, to be sure that the new standard doesn't hi-jack api.d.o output.

Netbeans 7.0:
- Code completion for parameters is based on the type hinting, not the docblock, so int|ClassName on a @param is ignored anyway.
- Popup descriptions of a docblock show the type as a literal string, so "int" and "int|ClassName" are both displayed as-is, as is "mixed".
- A return type of "@return int|ClassName" does auto-complete as if it were ClassName. I was very surprised at this, but pleasantly so.
- A return type of "@return ClassA|ClassB" offers autocomplete methods from *both* classes. Wow. NetBeans++.

So #44 looks good to go from a NetBeans perspective.

I love we have found a way to denote bool|string. That's so helpful. Truly. Let's go with it.

mikey_p, chx, and I talked about this on IRC. If everyone is settled on sun's standards in #44, we think it might be a good idea to make these changes on a per-module basis, and that most would make pretty good novice issues. I'll wait for jhodgdon's blessing before opening a zillion sub-issues, though. :)

Sounds like fun!

+1 on #52. That would actually be a great "newbie patcher sprint" to try and get new people involved, as the patches themselves would be rather hard to screw up on technical grounds. Any issues would be mechanical, which we can talk people through.

Subscribing as a newbie patcher.

@jhodgdon:

Maybe if we announce it a few weeks ahead, we can get lots of people to commit to making a patch for a single file, and it won't be too big of a burden on any one person? Thoughts?

I'm up to patching all the includes/*.inc files, provided that I can get answers to the following questions:

1. Should it be @param bool $foo or @param boolean $foo? I see seven examples of the former and twelve of the latter in Drupal core, so there is no strong precedent either way.
   EDIT: webchick answered in #1296208: When in doubt, we do as PEAR does, which ... is 'bool'.
2. Why does the example read @return Parser|false rather than @return Parser|FALSE or even @return Parser|bool?
   EDIT: answered in #11; apparently this is a PHPDoc standard. Weird.
3. If a function may return an object or TRUE or FALSE, should the docs say @return object|boolean or @return object|true|false?
   EDIT: answered in #1295510-9. Apparently bool is preferred or true|false.
4. EDIT: new question: When a parameter or return value can be NULL, how do we document that fact?
   • @return foo|null
   • @return foo|NULL

   • @return foo
       (optional) ...

5. Can we change "get the verb tense right" to "starts with a third-person singular present indicative transitive verb" please?
   

   third-person

   The implied subject is "This function" or merely "It" rather than "I" (first-person) or "You" (second-person).

   singular

   The implied subject is "This function" (singular) rather than "Some functions" (plural).

   present

   Use Returns (present) rather than Returned (past) or Will return (future).

   indicative

   Use Returns (indicative or non-durational) rather than Is returning (gerund or durational). See Grammatical tense.

   transitive

   A transitive verb requires an object to receive the action, e.g. to answer the question, Returns what?

   Correct:

   Returns NULL if all goes well; throws an exception otherwise.

   Incorrect:

   Returns if all goes well; throws an exception otherwise.

Also see related #1290258: The data type should be explicitly stated for all Drupal core @param and @return documentation.

Posted: #1296208: Standardize on either '@param bool $foo' or '@param boolean $foo' and document the decision.

EDIT: Got an answer; see above.

I don't actually think requiring this for every param and every return is possible in core.

I'd roll a patch to prove you wrong if I didn't think it would start a fight.

Thanks @jhodgdon for your thoughtful response in #60. I am a bit of a loss about how to respond as I review various patches.

As I understand the 'Drupal process', patches bringing docblocks up to standard are suppose to be part of distinct documentation-only patches. Yet according to your response across a couple of issues today, one is only suppose to correct what is identified in the issue summary. Therefore, as I understand, if one identifies another problem in the same docblock that the patch is addressing, one should not correct the additional issue as well.

Is this your guidance as co-head of the documentation team? If so, what should a reviewer do as a result? Open up a new issue highlighting the additional documentation problem? Thanks in advance for your guidance.

Copied issue summary from newer duplicate issue.

Typo fix.

Closed duplicate: #1290258: The data type should be explicitly stated for all Drupal core @param and @return documentation.
Copied its Issue Summary here.

Copying over my comment from the other issue, accidentally posted after it was marked a dupe:

Boy this issue got busy...

Re Webchick in #51, I would agree with #60. I don't see anything in the current coding standards that forbids the use of a primitive type in a docblock, and core has some usage of param types scattered around. Various contrib modules (like, any I've written at least) already have them. I don't see why we would start not allowing them in D7 patches.

Going forward, I don't believe we need to have one massive uber-patch. This is the sort of thing that we can work on over time, hell even as part of other issues. (Changing this function? OK, fix its docblock, too, since I'm changing the function signature anyway.) Then we can do another walk-through later once we're done changing the code being documented to make sure any missing @param types are filled in.

I am not volunteering to write a mega-patch, but I have every intention of putting types on all of my @params going forward. I already do, but I intend to keep doing so. :-)

pillars: Um, Dave Reid hasn't commented in this issue that I see. Why do you have him listed as having an opinion?

Removed reference to patch from other issue.

Likewise copying:

Um, Dave Reid hasn't commented in this issue that I see. Why do you have him listed as having an opinion?

From his comments here and here, I got the impression that he just doesn't like change.

Again, feel free to make corrections. jhodgdon asked for a poll, so I did the best I could.

I am not volunteering to write a mega-patch

I've already volunteered to write a mega-patch (or a zillion sub-patches, if that is preferred) but nobody (especially not jhodgdon) has volunteered to review it, once written.

Not that I care much, either way. The important thing, I think, is that this page gets rewritten to reflect whatever decision is made.

Re Webchick in #51, I would agree with #60.

Note: that was #1290258-51 and #1290258-60. Copying the latter for easy reference:

For D7 patches, there should not be any type hinting for @param or @return doc statements.

I would tend to disagree. The published standard for D7 was and is: (deleted for brevity)

In other words, type-hinting is "recommended" in certain cases; "not needed" in others. It is never forbidden nor required. Removing all type-hinting from D7 core would be nearly as big a patch as adding it to all of D8 core.

Personally, I find the current standard to be ambiguous because I lack the particular variety of "common sense" that appears to be assumed.

Re-ordered Remaining Tasks in decreasing priority.

List the "various reasons" why mandatory type-hinting is desirable.

Updated summary to explain what type-hinting is and why mandatory type-hinting is desirable.

Explain at the beginning what "type-hinting" means.

Prefer 'bool' over 'false|true'.

For us coming from strongly typed languages (ex. java), this is very nice to have. Reading ahead *might* be necessary to understand the variable, but it's a bit of information that you can rely on (you know it's going to be there) and together with a meaningful name is a very nice starting point.

+1

Link to image provided by mikey_p illustrating IDE breakage.

But we do need buy-in from the core devs on all coding standards... and I'm not sure we're there yet on this idea.

If and when they do, should I submit 58 separate issues, or a single issue with 58 patch files attached?

bobvin@bowie:~/www/8.x$ ls -d */ modules/*/ profiles/*/ themes/*/ | egrep -v '^(modules|profiles|themes)/$' | wc -l 
58

I'm a little confused as to why I'm listed opposed to this. My only objects were that this had no discussion in the queue prior to being changed and that I want this to get it right and not keep changing since it affects a lot of code and docs. Now I see that we have an agreed upon standard, I'm absolutely *not* opposed to this at all. I'm changing my name in the list.

Moved jhodgdon to the "In favor" list as per her last comment.

I'm excited to see this. I've recently started using Jetbrains' PhpStorm IDE which offers a large number of 'inspections' — on-the-fly messages of varying severity about the code that you're writing/looking at, including suggestions and one-click fixes for many of them. This is extremely helpful, and having data types in the docblocks enables it to do even more amazing stuff. PhpStorm even creates (and continually type-checks) the docblocks for you, including the pipe return types based on all the returns you have in your functions.

An IDE like PhpStorm offers efficiency and quality benefits for programmers of all levels. Enabling type information in core will be a huge boost, especially for beginners, and it would be well worth doing this not just going forward but also for D7. I'm pretty sure we'd find a nice bunch of core bugs in the process, too...

Switching my name to the support list.

Added salvis to the "in favor" list and re-sorted according to Drupal uid.

This issue is currently blocking #1300494: Unify @param $field explanation

Just to clarify, the current docs only recommend type-hinting when the parameter is required to be an object of a particular class, and explicitly states that type-hinting is unnecessary where the type is obvious from the context.

To me, that means that if I add the word array to the parameter description, then I should remove it from the @param line, and vice-versa.

Or maybe the current standard means absolutely nothing. Maybe it is void for vagueness.

But since discussions of coding standards are a complete mis-use of time, energy, and focus, this issue should probably be closed (won't fix) or possibly closed (duplicate) in favor of #1299710: [meta] Automate the coding-standards part of patch review.

@#77 by jhodgdon on October 7, 2011 at 4:27pm:

- Primitive and non-specific types: use the lower-case type name; e.g., "string", "array", "bool", "object".

Please add null, true, and false to the list, or describe what to do when a parameter or return value is explicitly allowed to be NULL, or allowed to be only one of (TRUE, FALSE). I can provide links if necessary.

Please also specify in the overview (not just in the examples) that, e.g., string|int|true is preferred over mixed.

And realizing that newbies sometimes cut-and-paste from standards examples, can we PLEASE not submit the following as an example of acceptable coding style?

* @param array $myarray
*   An array.
* @param int $myinteger
*   An integer.
* @param string $mystring
*   A string.

I have seen dozens of places in core where the parameter descriptions literally followed the standard, and were no more helpful nor descriptive.

1) I think the proposal is to actually move #44 into the coding standards (doxygen/documentation) page.

2) As of now, I think this can only be documented as a SHOULD, not as a MUST.

3) From my perspective, the essential difference to now is that the current standards rather target OO code, not procedural/all code. And, that there's a precise definition for how to specify the type names (object/stdClass/etc), and that you can specify multiple types (and how).

4) I'm personally happy to include this in my patch reviews. However, related to 2), I'd suggest a soft-transition, meaning, I won't complain about missing data types in the currently existing 2,000+ patches for core, as that would be ridiculous for everyone involved. Instead, I'd limit it to patches in issues that were created after these rules have been taken into place (for clarity and simplicity, we can say "All new patches created after November 1st 2011").

re: #78 / #79

- Only TRUE and FALSE are Booleans. "bool" is specified for them, not true or false. We're documenting data types, not data values.

- If a function argument takes an explicit NULL (with meaning), it can be documented as "null". However, if NULL is the default value for an optional parameter, then I don't think I've seen that getting documented elsewhere on the net.

- If a function returns NULL, then "void" is specified (not "null").

- If a function returns NULL, then "void" is specified (not "null").

Ah; thanks. I'd never seen that before. Can we get that added?

1. In the example code block following "it is recommended to specify the data type in the @param or @return directive", the following example comment lines wrap. Is this a defect in Firefox/Linux or is it a defect in the page design?

    *   The prepared statement query to run. Although it will accept both named and
   

    *   unnamed placeholders, named placeholders are strongly preferred as they are
   

    *   unnamed placeholders (?), this is an indexed array and the order must match
   

2. This abomination still exists:

   * @param array $myarray
   *   An array.
   * @param int $myinteger
   *   An integer.
   * @param string $mystring
   *   A string.
   

Why are we talking about type hinting? What does that have to do with this issue?

@#86 by jhodgdon on October 7, 2011 at 11:24pm:

Thoughts?

MUCH better!

My only remaining suggestion (and it really is a nitpick, so feel free to ignore), is:

* @return string
*   Description of the return value, which is a string.
*   Return values use the same conventions as @param, but with no variable name.
*   Note that @return should just be omitted if there is no return value, or if
*   the return value is always NULL.

Type hinting allows you to require arguments be a specific object/class or array (doesn't work for bool, string, int, etc for PHP) and it throws an error if the param is the wrong type. See http://php.net/manual/en/language.oop5.typehinting.php

It doesn't have anything to do with inline documentation, other than it can be said that it makes the code "self documenting."

I'm definitely okay with it.

Nitpicking the nitpick:

*   Note that @return should just be omitted if there is no return value, or if it is always NULL.

The second part is illogical and unnecessary. Omitting @return if there's no return or just one or more return;s (note the semicolon — no return value!) makes sense and seems to be the common practice. I know that return; and return NULL; are the same thing internally, but that's geek knowledge that we don't need to cultivate.

If...

function foo() {
  if ($whatever) {
    return 'something';
  }
  return NULL;
}

... should be documented as @return string|null (which I believe we agree on), then it makes no sense to specify an exception for the rare case of returning only NULL. I'd like to revert this to #86.

For...

function foo() {
  if ($whatever) {
    return 'something';
  }
  return FALSE;
}

... I've seen both @return string|bool and @return string|false. The latter definitely carries much stronger meaning, and it accurately documents this common PHP pattern. It also fits well with the null case above. (If TRUE can be returned, too, then it should be bool, of course.)

Thus I'd like to keep the

* @return Parser|false
*   A class instance object of type Parser, or Boolean FALSE.

example that was taken from http://manual.phpdoc.org/PDFdefaultConverter/documentation.pdf (or here, see #11 and #44). It allows code inspection tools to accurately check whether the caller has covered the relevant cases.

 
(Wouldn't it be nice if the testbot did this some day? Let's start now to get our code ready for that day!)

What about...

function foo(array $bar = NULL) {
}

...? PhpStorm generates @param array|null $bar for this. Makes sense, doesn't it? Both, to humans and to tools...

Maybe we could add an example like

* @param array|null $options
*   Description of this optional parameter, which takes a PHP array value or defaults to NULL.

(EDIT: Change "and defaults to NULL" to "or defaults to NULL")

@#93 by salvis on Octobe 8, 2011 at 10:16am:

*   Note that @return should just be omitted if there is no return value, or if it is always NULL.

The second part is illogical and unnecessary.

You're right, of course.

...? PhpStorm generates @param array|null $bar for this. Makes sense, doesn't it? Both, to humans and to tools...

According to sun in #82:

- If a function returns NULL, then "void" is specified (not "null").

I'm okay either way, but I'd like to reach a consensus. Is it properly null or void?

Is the @return of 'void' in the case of of a NULL return another Drupalism?

There are several functions that return either a string, FALSE, or NULL. How are these to be documented? @return string|false|null seems quite appropriate vis-a-vis @return string|bool|void... Guidance appreciated.

I've found only two instances of @return void in Core (both in aggregator.pages.inc), and both functions don't have any return at all, so the @return should be omitted, according to common consensus.

We have no history of @return void for NULL returns.

void is a general industry standard and seen in API documentation of PHP, JavaScript, Java, and other languages.

Example: http://php.net/manual/en/function.exit.php

It is commonly documented as

  * @return void
  */

(without description)

We can choose to omit that, of course. But then again, the point of this issue is to clarify and precisely state what gets in and what gets out. A missing @return could be intentional, but it could also be a bug. Hence, unless you review the function body, you cannot be sure whether it's missing or if it indeed doesn't return anything.

Okay, I understand @return void now, and we do want to decide whether to include that as part of our standards, but I have a different question.

For the case where a function may return a string, FALSE, or NULL (with meaning), which of the following docs would be correct?

1.  * @return string|false|null
   

2.  * @return string|false|void
   

3.  * @return string|bool|null
   

4.  * @return string|bool|void
   

5.  * @return string|false
   

6.  * @return string|bool
   

I am on board with #91, give or take the tweaks suggested after it.

I cannot speak for other core devs, but I already specify type in the docblock as a matter of course in my own code. I am more than happy to start leaning on other people in patch reviews to do the same. :-)

RE #98, I would argue that examples 1-4 are a "code smell" that the function is too non-specific in the first place and needs to be refactored. Nothing should return "one of three different data types, maybe, depending on the phase of the moon." In fact I'd go as far as saying that we should add that to the coding standards; if you find yourself writing that kind of docblock, You're Doing It Wrong(tm). (And I'm sure core is doing that wrong right now somewhere. We should fix it.)

I think example 5 more accurately captures "this returns something meaningful or FALSE if there is nothing meaningful", but I can live with example 6 if we decide to go that way.

@sun: It's a long way from

void exit ( int $status )

(as on http://php.net/manual/en/function.exit.php)

to

 * @return void

and to

  return NULL;

http://en.wikipedia.org/wiki/PHPDoc says concerning @return

This tag should not be used for constructors or methods defined with a void return type.

I agree that "void is a general industry standard", but part of that general industry standard is also that a void function may not contain a return statement with something in between the reserved word "return" and the end of the statement (typically a semicolon), and that you're not allowed to write a call of a void function on the right side of an assignment operator. All this is not true for PHP and it does not apply to return NULL;, so we're on shaky ground here.

Technically, return; and return NULL; may be the same in PHP, but if you have both in the same function, then PhpStorm flags this as "Inconsistent return points."

PhpStorm will generate the following docblocks:

1. Function without any return: * @return void
2. Function with return;: * @return
3. Function with return NULL;: * @return null

For #1 and #2 it'll also accept a docblock without any @return (as has been the guideline for Drupal since 2004), but for #3 it'll say "Missing @return tag in function/method PHPDoc comment."

If I pair return NULL; and * @return void (as you seem to be advocating), then it'll say "The function/method declared void must not return a value."

I'm not saying that PhpStorm or WikiPedia are the references, but they are defined opinions out there.

Of course, Drupal coding standards are based on Doxygen, not PHPdoc.

re #98: I agree with @Crell that 1-4 are awkward and pointless on their own; if you have to document something like that and cannot fix it, you still have mixed.

Related to 5 vs. 6, I'm still on the fence that we're documenting data types (bool), not values (false). If you continue that pattern, then the next best question someone is going to ask is: "But hey, my function returns either integer 1 or 2, so I document @return 1|2, right?"

And to really make sure everyone knows what bool means in PHP:

> php -r "var_dump(is_bool(NULL));"
bool(false)

#100 is a very interesting research and question - thanks, @salvis! Curious, did you also test complaints against having a @return null? Aside from that, I'd agree that neither of both resources are setting the standard; additionally checking against a Doxygen parser might help -- and regardless of that, I guess and bet that we simply have to decide on something regarding void|null|nothing. That said, I'd be in favor of explicitly documenting no return value if a function is supposed to return nothing (for reasons outlined in #97), and would personally lean towards void.

@sun: For PhpStorm, @return null (and even @return AnyInterface) is compatible with all three forms listed in #100, as long as you don't mix them inside the same function body.

PhpStorm displays the docblock @return type in the code completion function selection drop-down list, and the @param types and names as hints while you're filling in the function call arguments. It doesn't do any cross-checking inside function bodies at this point beyond what I already mentioned. There's certainly room for improvement in this area.

The question you didn't ask is complaints against @return void. PhpStorm accepts both return; and no return. I'll go along with that, but we have to be aware that right now there are only two functions in all of Core that are documented this way. Making this a requirement will be a change of the coding standards, not just a clarification, and it will make Core hugely non-conformant.

PhpStorm agrees with me that @return void does not mix well with return NULL;. But, as explained in #93, the case #3 in #100 is no worth documenting — we should never have a function that has only return NULL; statements. If that's the intention then they should be plain return;s. OTOH, if one of the returns actually returns something, then we should have @return something|null.

Just to be clear, we never want @return something|void because that would be an oxymoron.

Why do you lean towards pedantic (no offense intended) about documenting types only rather than documenting what's useful? If I see @return Parser|false (e.g. in a code completion function selection drop-down list), then I know intuitively what it means, but if I see @return Parser|bool then I can only wonder...

I propose to amend #91

- Primitive and non-specific types: use the lower-case type name; e.g., "string", "array", "bool", "object".

to

- Primitive and non-specific types: use the lower-case type name; e.g., "int", "string", "array", "bool", "object".
- Special constants: use the lower-case value name: "null", "false", "true".
- Use "@return void" to signify that the function does not return anything.

This will preclude @return 1|2.

#103 makes more sense to me than what @sun has proposed. I particular like @return string|false|null type hinting as an example.

Just so we agree that @return string|bool is preferred over @return string|false|true.

Agreed on #105. But just to be clear, I prefer|recommend '@return string|false' when a TRUE return is not defined.

+1 for #109. I agree with all the points there. I especially like the last point! Good code documentation really does make for better programmers.

One minor tweak I might make:

The @return section should be omitted only if the function never has a return value.

...but it's not too important.

I am quite happy with your revised draft. Assuming others are as well, I think this is ready to commit.

I liked the idea of always requiring a @return and thus avoiding doubts on whether the function doesn't return anything or the @return has been forgotten.

#109 works for me and is clearly superior to the present documentation.

I think requiring @return in all cases should be discussed in a follow-up issue.

Thank you, Jennifer!

And instead of using, e.g., "string|null" to indicate an optional parameter, use (optional) at the beginning of the description.

I see where you're coming from — the trouble is that if you use a tool like PhpStorm, it'll react to "string|null", but not to "(optional)", which is part of the description. PhpStorm doesn't actually complain if I remove "|null", and it'll pick up the "= NULL" instead, but if I let it generate the docblock for me, then it puts "string|null" there, and having to fight against your tools is always a bit of a pain.

There's one case where the "|null" matters in the current version of PhpStorm:

/**
 * @param array|null $a
 */
function foo(array $a) {
}

function bar() {
  foo(NULL);
}

If I remove the "|null" above then PhpStorm will say "Expected array, got null," which is right on the spot.

This is not the exact case that we're discussing here, but it's close, and things become unnecessarily complicated if we require removing the "|null" in some cases and having it in others.

My ideal would be to have both the "|null" and the "(optional)". Maybe that's asking too much, but the two overlap only partially. Knowing whether a string parameter also accepts NULL is useful information, regardless of whether the parameter is optional or not. The fact that the parameter is optional is not a good reason for requiring the opposite behavior, namely requiring dropping the "|null" rather than requiring writing the "|null".

If a parameter is "(optional)", the default need not be NULL. It could just as well be '' or even 'default'. If it says "string", then passing NULL is an error, and if it says "string|null" then it the function obviously must handle the NULL case, no matter whether there's a default and what that default is.

I propose to drop the sentence and expect people to do the right thing, i.e. systematically document acceptable special values of differing type, whether they are default argument values or not. If I can call foo(NULL), then "null" should appear in the @param list, no matter whether it's optional or not. The latter aspect is documented by the presence or absence of "(optional)".

Some nits, if we're striving for perfection in documentation... ;-)

* @param null|bool|string $description
*   Description of this parameter, which can be NULL, Boolean, or a string.

* @param bool|string $foo
*   Description of this parameter, which takes a string or a boolean TRUE/FALSE value.

The "main" type is presumably string and the others are probably special cases. I'd reverse the order to consistently start with "string" (put the normal case first), as you did intuitively in your text further down.
I'd spell "boolean" in lower case, George Boole will forgive us. IAC it should be consistent.
I'm not sure we need both examples, but if we want to have both, then put them together and the simpler one first.
They are somewhat redundant, maybe we can change $foo to illustrate the "false" case?
So, here's ready for cut&paste, if all of this is acceptable:

* @param string|false $foo
*   Description of this parameter, which takes a string or FALSE.
* @param string|bool|null $description
*   Description of this parameter, which can be a string, a boolean (TRUE or FALSE), or NULL.
* @param DatabaseStatementInterface $statement
*   Description of this parameter, which is a database statement object.

I don't think suggesting that people use "|null" to indicate an optional arg is a good idea at all?

Hmm, I think I didn't make myself clear. I was trying to propose that we require both "(optional)" AND "|null" (if NULL happens to be the default), because they really are two separate aspects.

I agree completely with your example.

function abc($foo = 'hello')

is "string", not "string|null"

However

function def($foo = NULL)

should be "string|null" and

function ghi($foo = FALSE)

should be "string|false", provided that $foo is a string parameter. The description should start with "(optional)" in each case. abc() is self-explanatory, the others probably need some additional explanation on how NULL/FALSE is interpreted.

Likewise

function jkl(array $a = NULL)

is "array|null", but

function mno(array $a = array())

is just "array."

The @param should document the allowed type(s) and/or special values, independently of whether they are required or optional. And "(optional)" should document whether the parameter is optional, independently of its type(s)/value(s).

I don't think we should use |null unless the function expects you to pass NULL in as a possible argument value.

Replace "expects" with "allows" and we agree.

Replace "expects" with "allows" and we agree.

abc doesn't allow: abc(NULL) would be an error.
def allows: def(NULL) is ok.
ghi doesn't allow: ghi(NULL) would be an error.

That's why def has the "|null" and the others don't. This has nothing to do with the fact that you can call all three without any parameter.

Great.

I think we could just omit

And instead of using, e.g., "string|null" to indicate an optional parameter, use (optional) at the beginning of the description.

because "(optional)" is already covered by

 * @param $third
 *   (optional) TRUE if Third should be done. Defaults to FALSE.
 *   Only optional parameters are explicitly stated as such. The description
 *   should clarify the default value if omitted.

If you want to be explicit, then how about

In the case of optional parameters, if the default value is of the same type as the parameter, then don't mention it separately. Otherwise make it explicit, e.g., "string|null" or "int|false". The parameter description still needs to start with "(optional)".

* @param object $query
*   Description of this parameter, which takes a PHP stdObject value.

...

* @param DatabaseStatementInterface $statement
*   Description of this parameter, which is a database statement object.

This is backwards: It's the $query, which is a DatabaseStatementInterface! This should be

...

* @param object $account
*   Description of this parameter, which takes a PHP stdObject value.
* @param DatabaseStatementInterface $query
*   Description of this parameter, which takes an object of a class that implements the
*   DatabaseStatementInterface interface.

Wordsmithing:

In the introductory paragraph, drop the parenthesis and connect the thoughts in the two sentences.

If the data type of a parameter or return value is not obvious or is a specific class or interface type, specifying the data type in the @param or @return directive is required. Specifying the data type is also recommended for other @param or @return directives, including those with primitive data types, arrays, and generic PHP stdObject objects.

Let's be a bit more specific in the last note.

- In general, if there are three or more choices (e.g., "string|bool|null") for a parameter or return data type, the function should probably be refactored for simplicity. (For instance, return a TRUE/FALSE success flag, and use a by-reference parameter to return other information).

We're getting there! :-)

I agree that #125 is better.

We still have "Boolean" and "boolean."

We took care to invent differing parameter names — rename the second $foo to $bar.

I'm not a native English speaker, but "If there is only a return value in some cases" doesn't sound quite right to me. I propose

- If some code paths return a value and others return NULL or nothing, then document this as, e.g., @return string|null to indicate the function may return a string or nothing (and explain the circumstances in the description).
- If the function never returns anything, then omit the @return section completely.

@ #126 by salvis on October 15, 2011 at 9:21pm:

I would prefer "always returns NULL" to "returns nothing" or "never returns anything" but I understand that in PHP these phrases have the same meaning.

And I am a native English speaker, so here goes:

- If a function may return NULL in some but not all cases, then document this possibility. For instance, if the return value may either be a string value or NULL, then use @return string|null followed by a description of the circumstances in which each is returned.
- If a function lacks a return value (always returns NULL) then omit the @return section completely.

The change in #127 looks good to me as well.

Also, I believe it should say stdClass rather than stdObject.

Revised example for further discussion:
--

If the data type of a parameter or return value is not obvious or is a specific class or interface type, specifying the data type in the @param or @return directive is required. Specifying the data type is also recommended for other @param or @return directives, including those with primitive data types, arrays, and generic PHP stdClass objects.

Here are some examples of how to specify data types (see notes below):

* @param int $number
*   Description of this parameter, which takes an integer.
* @param string $description
*   Description of this parameter, which takes a string.
* @param bool $flagged
*   Description of this parameter, which takes a Boolean TRUE/FALSE value.
* @param array $args
*   Description of this parameter, which takes a PHP array value.
* @param object $account
*   Description of this parameter, which takes a PHP stdClass value.
* @param DatabaseStatementInterface $query
*   Description of this parameter, which takes an object of a class that implements the
*   DatabaseStatementInterface interface.
* @param string|bool $string_or_boolean
*   Description of this parameter, which takes a string or a Boolean TRUE/FALSE value.
* @param string|null $string_or_null
*   (optional) Description of this optional parameter, which takes a string or can be NULL.
*
* @return string
*   Description of the return value, which is a string.

Notes:

• For primitive and non-specific types, use the lower-case type name; string, array, bool, object.

• For special constants, use the lower-case value name: null, false, true. Use bool rather than true|false though -- only specify true or false if its opposite is impossible or unacceptable (e.g., @return string|false).

• For optional parameters, the default value should only be mentioned on the @param line if its type differs from non-default values. For instance, use @param string|null or @param int|false but never @param string|'' or @param int|0.

• For classes and interfaces, use the most general class/interface possible. For example, the return value of db_select() is properly documented as SelectQueryInterface rather than a particular class implementation such as SelectQuery.

• Return values are documented by @return and use the same conventions as @param, but with no variable name.

• If a function may return NULL in some but not all cases, then document this possibility. For instance, if the return value may either be a string value or NULL, then use @return string|null followed by a description of the circumstances in which each may be returned.

• The @return section should be omitted if the function lacks an explicit return value.

• In general, if there are three or more choices (e.g., string|bool|null) for a parameter or return data type, the function should probably be refactored for simplicity.

#127:

I would prefer "always returns NULL" to "returns nothing" or "never returns anything"

There's a subtle difference here — the result in PHP is the same, but the semantics are different.

A function that has no returns or only returns; without any value is called a procedure in other languages, and often its return type is indicated as "void". This is a function that "returns nothing" by design. Even if PHP allows you to write return NULL; in such a function (and it doesn't make any difference), it's a conceptual error to do so. This type of function should have either no @return tag or an empty one (as sun would prefer).

If another function has a return with a value (even if it's NULL!), then all of its code paths should end with returns with a value (even if it's just NULL), conceptually! I know that it's common to not follow this rule and to simply drop returns, because PHP is lax about this, and I'm not going to try to change this, BUT I've carefully worded my sentences so that they at least encourage clean coding.

We've already been over this earlier in the thread. PhpStorm checks this and complains, if you mix return; and return NULL; in the same function, for example.

So, I definitely prefer #126 over #127 because I would like to encourage clean thinking over not-so-clean language-specific concepts.

Is the revised example in #129 acceptable?

• The @return section should be omitted if the function lacks an explicit return value.

This avoids the question of how to document the following hypothetical function, or whether it violates coding standards.

function foo($bar) {
  if (empty($bar)) {
    return NULL;
  }
  do_something();
}

Although PHPstorm may distinguish between the preceding and following examples, PHP does not.

function foo($bar) {
  if (empty($bar)) {
    return;
  }
  do_something();
}

#129 looks great to me as were #109 and #124. Let's get this change in so that others are aware of this change about type hinting. I think further wordsmithing can be deferred to a follow-up issue. Great work all!

Minor pedant point:

- In general, if you find yourself wanting a lot of choices (e.g., "string|bool|null") for a parameter or return data type, this could be an indication it would be better to refactor your function so that it is more understandable (for instance, return a TRUE/FALSE success flag, and use a by-reference parameter to return other information).

I wouldn't encourage input/output parameters, as those are also a frequent source of issues as they by definition are based on side-effects. :-) Just leave out the parenthetical statement. Also, rather than "lots of choices" be more explicit: three or more. So:

- If you find yourself needing to specify three or more possible types (e.g., "string|bool|null") for a parameter or return data type, it is usually an indication that the code should be refactored to be more consistent and understandable.

@#133 by Crell on October 16, 2011 at 4:43pm:

I wouldn't encourage input/output parameters, ... Also, rather than "lots of choices" be more explicit: three or more.

Is the version in #129 acceptable?

Here is the source text from #129.

Re: #135 -- I'm curious; what's the reasoning behind not backporting the new standard as part of #1310084: [meta] API documentation cleanup sprint? Are we then going to have separate documentation standards for D7 and D8 henceforth? If so, how do we document that point?

Outside of that question, I'd I agree that the standard is sufficiently clear and complete, and we probably don't need to perfect the wording anymore. (Plus we can always make minor corrections later if something needs clarification.) Based on the past 20 or so comments, let's call this RTBC, unless someone identifies a significant issue with the proposed final text in #136?

I'm happy. :-) RTBC.

Update "Remaining tasks" to reflect jhodgdon's opinion that we don't yet have sufficient buy-in from core developers to make this change happen.

Updated issue summary.

Updated issue summary.

So if anyone wants to add these to Drupal 8 docblocks, that is fine, but the patches will need to be rerolled for Drupal 7 to omit them,

I for one am unwilling to follow an unwritten coding standard.

If we are never going to change the standard for D7 code, and if we are never going to publish a separate D7 coding standard, then the standard should just be set in stone forever (or at least until D7 is no longer supported) and this issue should be closed (won't fix).

For now, I'll just insert data types wherever I feel they would be appropriate. Since we can't agree on standards, my opinion is just as good as anyone else's.

Updated issue summary.

I've copied the latest edition into the summary, and minimally tweaked the wording. Wanted to do more, but had to resolve a critical issue elsewhere.

Overall, pretty much fine with me. Though I'd love to go over the wording some more, since my first impression was that it reads a bit wordy and due to that it wasn't very clear.

Updated #1161796: Initial d8 port of Coder and Coder Review modules.

This all looks great to me. Just to add my thoughts:

- I agree with Crell about not suggesting using references as part of refactoring (#133)
- I agree with the wording in #110 - about making the use of @return very explicit (even if that particular standard is already stated elsewhere). I think the wording in #110 is much clearer (less unambiguous) than the alternative.

@nicl -- Note that this issue is marked fixed and the updated wording may be found at drupal.org/node/1354#functions

Please open a new issue if you have further changes to discuss.

Thank you.

For those that were interested in this issue, two follow-up issues have been created to add variable type hinting to all functions in two larger and more prominent files: bootstrap.inc and common.inc. Patches with some questions contained therein are attached to both issues:
#1333474: Variable Type Hinting for bootstrap.inc file (about 30kb)
#1333470: Variable Type Hinting for common.inc file (about 70kb)

Perhaps the process of getting the variable type hinting added to all of the functions contained in these large and prominent files will give us some insight into how involved and complicated a process it will be to migrate all of D8 core to variable type hinting?

Reviews and comments on this experimental start are welcome.

Where are we with all this then? I'm working on the coder review for d8: #1518116: [meta] Make Core pass Coder Review ... would it be useful if I created a patch for say database.inc to add @param types (all the @return types are done)

@jhodgdon regarding #146:

Only using primitive data types (like int, string, object) because if we map concrete object the code will be harder to review, makes no sense to me. If we map the concrete classes to the params, we get type hinting and calltips from PHPStorm, which eases up the development flow. If you map an interface as object to a parameter, it is understood that it is an object.