Problem/Motivation

This META issue is a spin-off from #1792992: Are typed @param / @return part of the documentation gate? where several people voiced the view that all @param and @return directives in docblocks MUST include type hints. However, some stated that it only SHOULD be a suggestion since "core does not follow this standard yet." The focus of this META issue is to add missing type hinting to @param and @return directives throughout all of existing core documentation.

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 (10-15 as a guess).

Proposed resolution

Add type hinting for all @param and @return directives in a series of "small" patches.

How to's

How to review patches for this issue:

# This is a one-liner to aid in reviewing patches on this Drupal.org meta issue:
# https://www.drupal.org/node/1800046
# You can use netbeansdrupalcomposed to gather the dependencies, which are phpcs
# and Drupal's Coder module
# https://www.drupal.org/sandbox/mile23/2197899
# Substitute any module path you want in order to check only that module.
# The resulting list will be a bit of gobbledygook in CSV, but will show you whether
# or not the patch you're evaluating successfully removed the errors we're looking for.

phpcs --standard="/path/to/drupal/coder/coder_sniffer/Drupal" --extensions="module/php,inc/php,php" --report-csv path/to/module/ | grep -F $'Missing param\nReturn type missing'

The above one-liner can also be found in this github gist: https://gist.github.com/paul-m/227822ac7723b0e90647
  1. Check that the parameters are identified correctly. For example, if the function signature is function_name($foo_bar), the parameter should be listed as $foo_bar in the docblock, not $foobar.
  2. Check that all parameters are documented, unless our standards explicitly specify omitting them (as with form constructors).
  3. Check that the parameters are in the order specified in the function signature.
  4. Read each parameter description and make sure that it is grammatically correct, clear English, with proper capitalization, punctuation, etc. (Each parameter description should form a complete English sentence if the words "This parameter is..." were to be added to the beginning, but those words should not be included in the docblock.) Keep an eye out for typos.
  5. Look up each function in the patch on api.drupal.org (D8). Read the function carefully and ensure that the parameter description properly documents how the parameter is used.
  6. Look at the type hint documented for each parameter. Make sure the types correspond to the allowed types at http://drupal.org/node/1354#param-return-data-type.
  7. Make sure all possibilities for the parameter are documented; for example, if the function optionally accepts a NULL value for a string parameter, then it should be documented as string|null.
  8. Also check the function's callers and string references (linked on api.d.o) and make sure the values being passed to the function are in the datatypes specified.
  9. Apply the patch to your local repository, and check that no other functions in these files are missing parameter documentation.
  10. Document what you reviewed and what you found specifically in your review comment.

Remaining Tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary noting if allowed during the beta Instructions
Module/File Issue # Notes
Action module #1802070: Add missing type hinting to Action module docblocks
Aggregator module #1807160: Add missing type hinting to Aggregator module docblocks
Ban module #1816732: Add missing type hinting to Ban module docblocks
Block module #1807674: Add missing type hinting to Block module docblocks
Book module #1807678: Add missing type hinting to Book module docblocks
Breakpoint module
Color module #1816760: Add missing type hinting to Color module docblocks
Comment module #1808478: Add missing type hinting to Comment module docblocks
Config module #1810002: Add missing type hinting to Config module docblocks
Contact module #1811226: Add missing type hinting to Contact module docblocks
Contextual module #1816764: Add missing type hinting to Contextual module docblocks
Dblog module #1811242: Add missing type hinting to Dblog module docblocks
Field module #1801998: Add missing type hinting to Field module docblocks
Field_ui module #1816782: Add missing type hinting to Field_ui module docblocks
File module #1811858: Add missing type hinting to File module docblocks
Filter module #1811328: Add missing type hinting to Filter module docblocks
Forum module #1811334: Add missing type hinting to Forum module docblocks
Help module #1811862: Add missing type hinting to Help module docblocks
Image module #1802060: Add missing type hinting to Image module docblocks
Language module #1811214: Add missing type hinting to Language module docblocks
Locale module #1807062: Add missing type hinting to Locale module docblocks
Menu_ui module #1800774: Add missing type hinting to menu_ui module docblocks
Node module #1800546: Add missing type hinting to Node module docblocks
Openid module #1816834: Add missing type hinting to Openid module docblocks
Overlay module #1816840: Add missing type hinting to Overlay module docblocks
Path module #1811874: Add missing type hinting to Path module docblocks
PHP module #1816846: Add missing type hinting to PHP module docblocks
Rdf module
Seach module #1811882: Add missing type hinting to Search module docblocks
Shortcut module
Simpletest module #1805340: Add missing type hinting to Simpletest module docblocks
Simpletest base test classes #1805346: Add missing type hinting to base test classes in Simpletest module docblocks
Statistics module #1811892: Add missing type hinting to Statistics module docblocks
Syslog module #1816858: Add missing type hinting to Syslog module docblocks
System module #1800330: Add missing type hinting to System module docblocks
Taxonomy module #1807002: Add missing type hinting to Taxonomy module docblocks
Toolbar module
Tracker module #1811888: Add missing type hinting to Tracker module docblocks
Translation module
Update module #1811202: Add missing type hinting to Update module docblocks
User module #1800174: Add missing type hinting to User module docblocks
Xmlrpc module
Bootstrap.inc file #1326456: Add missing @param in includes A-C, plus other corrections to some docblocks
Common.inc file #1326456: Add missing @param in includes A-C, plus other corrections to some docblocks
Database sub-system

Completed related issues

Related issues

Comments

jhodgdon’s picture

One more note... These patches take a *large* effort for writers, reviewers, and committers if they are done properly. In most (or at least many) cases, you will need to read the entire function code carefully to figure out what the argument and return value types are.

As I've said before, I personally do not think this is worth the effort to do for all of core. And just for the record, I'll just state here that I personally don't want to spend my volunteer Drupal time, as a committer, to doing final review and commit for these patches. I think I can find more valuable ways to spend my Drupal volunteer time, such as adding new features to the API module (for api.drupal.org) or reviewing/committing other patches (I'm already way behind on other clean-up efforts that I personally feel are more worthwhile).

This initiative has been tried before, and no one came up with the level of effort that would be needed to complete the project. I doubt it's available now... Sorry to be discouraging, but I really feel like the effort needed here could be better spent on other initiatives. But that's my bias... if you can come up with people who want to make it happen and are willing to spend the care and time necessary, I certainly won't stand in the way -- just don't assume that I'm making time available myself.

Lars Toomre’s picture

Issue summary:View changes

Updated the full set of modules in module table.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue for the User module. Also added links to patches for bootstrap and common.inc files.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added additional sprint meta issues to related issues section.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue for the node module.

Lars Toomre’s picture

Issue summary:View changes
Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub issue for image module.

Lars Toomre’s picture

Issue summary:View changes

Added sub issue for Action module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue for taxonomy module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue for Aggregator module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

TravisCarden’s picture

@jhodgdon, I see that the work here is going forward and that, despite your expressed wish not to spend time on it yourself, some of the issues are getting assigned to you. Is that going to be a bother or a distraction to you?

jhodgdon’s picture

If the patches have been carefully reviewed and marked RTBC by the reviewer, I can get assigned to commit them.

Lars Toomre’s picture

@TravisCarden Reviews in the sub-issues are welcome. Most to date deal with adding type hinting to *.api.php files.

Lars Toomre’s picture

Issue summary:View changes

Added comment sub-issue.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue for Config module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added Dblog sub-issue.

Lars Toomre’s picture

Issue summary:View changes

Added Filter module sub-issue.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue #1811858 for file module.

Lars Toomre’s picture

Issue summary:View changes

Added a sub-issue #1811862 for Help module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue #1811888 for the tracker module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue #1816840 for Overlay module.

Lars Toomre’s picture

Issue summary:View changes

Added sub-issue #1816846 for PHP module.

Lars Toomre’s picture

Issue summary:View changes

Updated issue summary.

jhodgdon’s picture

Issue tags:+coding standards

This issue forgot to have a tag. :)

ronan.orb’s picture

Issue Reminder Comment.

ronan.orb’s picture

Issue summary:View changes

Updated issue summary.

Mile23’s picture

Issue summary:View changes

Poll module is no longer in core.

Mile23’s picture

Issue summary:View changes

There's no menu module in core... Hijacking the menu issue to be about menu_ui instead.

Mile23’s picture

All child issues are either closed or have patches needing review.

Mile23’s picture

Issue summary:View changes

Added info about how to review the patches, available here: https://gist.github.com/paul-m/227822ac7723b0e90647

heddn’s picture

Issue summary:View changes
Issue tags:+Needs beta evaluation

Let's get a beta eval documented here. Update the issue summary noting if this allowed during the beta.

Mile23’s picture

Version:8.0.x-dev» 8.1.x-dev
Issue tags:-Needs beta evaluation

Bump up to 8.1.x.

Moving all child issues, too.

jhodgdon’s picture

Why are these being moved to 8.1.x? There is absolutely no reason why they cannot be committed to 8.0.x, that I'm aware of.

Mile23’s picture

Version:8.1.x-dev» 8.0.x-dev

OK.

I thought we were patching for 8.1.x and then cherry picking for 8.0.x, but whatever.

jhodgdon’s picture

Yes, we are patching 8.1 and cherry picking 8.0.

However, the current way to indicate that a given patch is suitable for both is to set the version to 8.0.x. Issues that are set to 8.1.x mean "This is not suitable for 8.0.x", under current procedures.

This is obviously not very sustainable going forward where we'll have multiple versions, and I didn't come up with the scheme, but that is what we are supposed to do for the moment. For what it's worth...

Mile23’s picture

Some of the child issues don't apply both ways, so some will need to be applied to 8.0.x and then need a port to 8.1.x.

jhodgdon’s picture

Ah, right. In those cases, we would want to make the 8.1 patch first, tag the issue as needing a backport to 8.0, and after 8.1 is committed do the backport. This would be similar to how we never patch 7.x unless 8.x already has the fix in place.

Mile23’s picture

Issue summary:View changes

The phpcs script I made manages to ignore .inc files. This hasn't been an issue in some modules, others maybe a bigger deal.

Updating issue summary with new phpcs.

Version:8.0.x-dev» 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.