The data type should be explicitly stated for all Drupal core @param and @return documentation.

We are planning to wait on patches of this size (that affect everything in a file and would make many patches need rerolls) until the sprint the week of November 1st, after the big patch that will move all the files into core.

That is not really an improvement to have 50 separate issues (or however many there are) -- it would still make all patches related to file.inc need a reroll.

Please just wait until November. We do plan to have a big "fix up the docs" sprint the first week.

There is an event calendar on gdo. I just haven't posted this event to it. Planning to post to gdo in the documentation-team group in mid-October.

See #7 "I haven't posted...yet".

Oh, sorry! Just go to groups.drupal.org and click on "Events" in the top nav. To add an event, go to the group you want to add it to, and add new content of type "Event". :)

While I think it'd be great to try to schedule patches like this for that rough period, it'd also be good to get more reviews on this one. For example does api.drupal.org handle this properly?

Sorry I should've clarified more as well.

I think it makes tonnes of sense to try to get patches like this in around the week or two after November 1st - that way the issue queue massacre happens all at once.

However any patch that should go in for that window really needs to be reviewed in advance - since if it starts getting reviewed around November 1st it could end up not being ready until the end of that month, at which point people will already be trying to re-roll patches again.

I think there's a reason (not necessarily a good reason but some kind of reason) why we don't have data types in @params but I can't remember what it is. This is the sort of issue that might be worth cross posting in the coding standards group and or g.d.o/core to get some more reviews.

And... that doesn't mean the patch needs to be re-rolled, but we should agree the coding standards change or not (or maybe we're just breaking the coding standard everywhere already, I didn't look but this information is missing in the issue ;).

OK so to me it seems sensible just to document all data types of parameters. We often end up saying int/string/bool in the description anyway and could drop those. But I'd like to see more people chime in on this issue either way.

Very +1 on documenting all return and parameter types, unconditionally. Doxygen is the only major PHP documentation format I know of that does not expect it. All IDEs I've used provide them in their template auto-generated docblocks. It's useful information for anyone trying to understand an API, and also, I believe, can help us encourage putting a useful description in the "description" part of the docblock, rather than just "an int" or "an id", which is a largely useless docblock.

I think some parts of the database layer already do that, but I'm not certain.

We already verified in another issue that NetBeans, at least, and I presume Eclipse, is surprisingly smart about figuring out what to do with parameter and return types, even multiple-choice ones like int|SomeClass.

Once again, huge +1 here.

I am a strong believer that all @param data types should be documented.

Since this patch will apply to both D8 and D7, shouldn't this patch (and all other documentation patches like it) be applied (when RTBC) before the great divergence due to the moving to /core file structure patch gets applied?

Should I bother reviewing this patch now or wait until it is re-rolled after November 1st?

Yea, I used to document it, but ended up trying to fit the standard...completely agree with documenting them all. What catch said in #18 is true as well and more reason to do so.

It's obvious that all @param should be documented. It's faster to look at one place to know precisely what data types are needed. One undocumented param maybe obvious and easy to guess for someone and not for someone else. Good docs are exhaustive.
I already started to do this in my code and noticed my IDE (NetBeans 7) didn't know what to do with a data type in @return though (it was fine with @param).

I have gone through and reviewed the patch and the doc changes all look appropriate.

I would mark this RBTC except for confusion on my part about when to use '@return mixed' vis-a-vis say '@return string|boolean'. I thought the latter example is preferred when there are only two possible return types so that IDEs knew what to expect. Any guidance on when to use each @return alternative?

Update status.

catch implied there might be some ambiguity around this point, so I'm here to clarify. This patch, despite its unquestionable awesomeness, is not the kind of thing we can backport to D7 since it's in effect changing the coding standards. Since 7.0 didn't enforce a rule of specifying data types, we can't introduce such a rule in 7.9.

Huge +1 for D8 though!

Sorry. Unintentional cross-post.

Update summary.

Also, tagging as DX. Having these datatypes will be super helpful.

Yeah, update the coding standards and live with it. That's what we've done in the past. There's no harm in specifying the 8.x coding standards early in D7 for contributed modules.

We did the same thing for string concatenation. I'm not sure if coder actively discourages adding data type now but if it does it'd be worth an issue/patch to coder to stop doing so for D7, then people can follow this if they want to in their modules without triggering warnings.

@webchick re #27 - I think there is some confusion in the community about coding standards and what is applicable and when. For instance, @sun in #1159356-9: Better error handling asserts:

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for current standards may too huge of a task. However, code in current versions should follow the current standards. Lastly, when following this practice, make sure to not squeeze coding standards updates/clean-ups into otherwise unrelated patches.

Based on your comment #27 above, does this mean patches for D7 will be rejected if they now include @param and @return type hinting? Or will D7 have some combination of the old and newer coding standards? If the latter is true, I would argue that this is a 'nice to have' patch that will help D7 DX now (while D8 is in development for the next several years).

@pillarsdotnet re: "when to use '@return mixed' vis-a-vis say '@return string|boolean'."

I cannot find the issue now, but there was discussion about the @return format like @Crell referenced in #20 above. Perhaps @Crell or @jhodgdon can give some guidance here?

> I believe, can help us encourage putting a useful description in the "description" part of the docblock, rather than just "an int" or "an id", which is a largely useless docblock.

I don't think it does that. We do want people to write more than 'an int' in the description, absolutely. But making them do this first is fixing the wrong thing.

Furthermore, I think that adding a type like 'string' or 'integer' tells you nothing. You still have to read the description to know what the integer represents, or what kind of strings are valid. And as for arrays... in Drupal, you *definitely* need to read the documentation to know what to put in an array parameter ;)

So adds no useful information, but creates noise in the documentation. I'm a -1 on this.

I have never once viewed type information in a docblock as noise. I've viewed it as a developer who cared enough to leave me breadcrumbs to figure out what is going on.

This is the issue I mentioned previously: #711918: Documentation standard for @param and @return data types. In it we opted to favor int|string over "mixed", and determined that NetBeans, at least, is really damned good with @return data types.

The current coding standards say that the @param type is not required, but it's also not disallowed. I see no problem with patches going forward, starting right now, for D7 or D8, specifying types.

  * @param $foo
  *  Description lorem ipsum...
  * @param $bar
  *  Description lorem ipsum...


  * @param string $foo
  *  Description lorem ipsum...
  * @param int $bar
  *  Description lorem ipsum...

It's a variable-length piece of text that means my eye can't predictably find the place to scan down for the names of the variables.

It adds little useful meaning, as I still have to read the description to know what the int or string should actually be.

Hence I call it noise. But I could say a drop in readability for little added meaning.

@pillarsdotnet - re: #25 - Based on #39 and what is defined in Documenting functions and methods, I can confirm now that the use of '@return mixed' is deprecated. Instead, the form should be '@return x|y' where x and y are types like string and boolean. This patch will need to be re-rolled for the preferred @return doc.

@pillarsdotnet - re: bool|boolean - Heck, if I know which is appropriate. However, that core discourages abbreviations in variable and function names, I would recommend using 'boolean'.

Perhaps we should open a new issue for 's/@param bool /@param boolean /'? My fear is that such a documentation correction will not be implemented before November 1st nd the re-rolling of all D8 patches.

There have been about 30 comments or more since yesterday on this issue....

Have you made changes to the coding standards? If so, what were they and can they be shown in the issue summary? And have they been added to the coding standards docs?

AND why didn't this issue have the "coding standards" tag if coding standards were going to be discussed????? We now have a list of standard issue tags (see link below the tags field and its sub-pages). This is a standard tag that should be on ALL coding standards issues... sigh.

I am confused about when @param and @return type hinting should be used.

Say we have a patch that needs to be applied to both D8 and D7. As I understand now, the D8 patch should include the type hinting. Assuming that the patch also applies to D7 without change, should a separate patch nonetheless be rolled removing all of the type hinting documentation for D7?

Yes, that's correct. It's the same as the concatenation differences between D6 and D7+.

So. Has everyone (except me of course) already agreed that this is a good change do docs standards for D8? Is someone actually planning to go through every single core function and method header and put these things in? And have the key developers agreed that they are going to follow this for every new function and every change to every function? I personally think this is not a realistic idea unless the core dev team has bought into it.

So I don't have time to read this discussion, sorry. Busy day. Can someone please summarize who agreed to this and whether it's actually been agreed to? And not to mention why it's so necessary all of a sudden, when in d7 it was like pulling teeth to even get people to agree that we could add it as a "suggested good idea" rather than even close to a mandated standard?

@jhodgdon - I am sorry that you feel like you have not been included. However, based upon your comment #711918-52: Documentation standard for @param and @return data types, it never even occurred to me that you might object to type hinting being implemented for D8 docblocks. I thought you were proposing that all of the docblocks be updated. Is this no longer how you feel?

I am *not* planning to recruit volunteers for this task. I am planning on recruiting volunteers to do some more basic cleanup, like making sure there is a return between @param and @return, and that every function has a docblock, and the summaries are one line less than 80 characters. This is a much more involved task that requires a lot more inspection and thought. It cannot just be whipped out in a short session by novices.

And it's not that I feel that I wasn't included. I'm just trying to find out what you all think has been decided without reading through the 30 comments I missed between yesterday and today (which I have NO TIME TO READ, sorry), so I asked for a summary and so far no one has provided one :(

Document preference of 'bool' over 'boolean'.

@webchick - re: #51 - Thanks for your clarification. For D7 patches, there should not be any type hinting for @param or @return doc statements.

Your response also clarifies that @sun's assertion in #1159356-9: Better error handling is not true. This means one should not follow @sun's statement, "Drupal coding standards are version-independent and 'always-current'. All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be."

I do not see comments here by all of those people stating they are in favor of fixing all of core and volunteering to review/roll the patches? I started at the top of the list, and there is no comment from chx on this issue at all?

And I want to state that I think the standard is a good idea in the abstract, I just do not think (as I explained on the other issue -- why are we discussing this in two places??!? I am getting confused) it is practical to apply to all of core/contrib Drupal code as a standard that suddenly all code will be out of date on and need to be patched. This is not an automatic patch. It requires thought to write, and thought to review. It's not like "oh we need a space around ." or "we need a blank line between @param and @return sections".

Just to clarify something...

"And not to mention why it's so necessary all of a sudden, when in d7 it was like pulling teeth to even get people to agree that we could add it as a "suggested good idea" rather than even close to a mandated standard?"

In D7 it was like pulling teeth because of the timing of the initiative to introduce it (post code freeze), since it effectively represented an "API change." In D8 it's completely acceptable to revamp coding standards recommendations. Just as we introduced a different concatenation separator in D7.

Oops.

Re-read jhodgdon's comment -- she's neither in favor nor opposed; she just thinks it is not practical.

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?

Jebus these threads are moving fast...

#1296842: Clarify scope of PHP coding standards for core (major version) and contrib code

Should have used preview; sigh.

plach +1

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

EDIT: sorry, this is the dupe, re-posting to the active issue.

Adding myself as a supporter for the change.