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.

Proposed resolution

Add type hinting for all @param and @return directives in a series of scoped 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. 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.
  2. 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.
  3. 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.
  4. Apply the patch to your local repository, and check that no other functions in these files are missing parameter documentation.
  5. Document what you reviewed and what you found specifically in your review comment.

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.

Mile23’s picture

Title: [META] Add missing type hinting to core docblocks » [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*
Parent issue: » #2571965: [meta] Fix coding standards in core

We should re-scope these child issues to fix docblocks based on adding phpcs sniffs to phpcs.xml.dist.

Specifically these:

  • Drupal.Commenting.FunctionComment.MissingParamName
  • Drupal.Commenting.FunctionComment.MissingParamType
  • Drupal.Commenting.FunctionComment.MissingReturnComment
  • Drupal.Commenting.FunctionComment.MissingReturnType

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

xjm’s picture

Issue summary: View changes

Removing some out-of-date info from the summary.

xjm’s picture

So in terms of the scoping for these issues, adding type hints is actually a bit different than most coding standards changes. One has to read the code very carefully to determine what the typehint should be. It's even written up as an example at https://www.drupal.org/core/scope#examples. The advice there is:

A check for missing documentation typehints is easily automated, and it is easy to check the documentation against the function signature when the signature also has a typehint.

On the other hand, scalars, mixed data types, or other non-typed parameters can require more careful study of the code to determine what values are allowed, and return data types require understanding all code paths.

Instead, create a plan with several children, addressing the easy/obvious cases first, then the more complex cases, and the @return docs entirely separately once the @param docs are known to be correct (since the inputs will affect the outputs).

I'm not entirely sure about "@return docs entirely separately once the @param docs are known to be correct (since the inputs will affect the outputs)" since when you're reading a method you're probably going to figure out both.

Once a reviewer is in the context of checking parameter data types, checking multiple different parameters is fairly straightforward. Splitting such improvements into many individual patches is very unnecessary overhead, especially if the patches are created by the same authors, and a frustration to committers.

The advice for documentation patches generally is:

[...]Writing new documentation requires understanding the code, technical writing skills, and possibly maintainer input. It is simply not feasible to add all the missing parameter documentation to core in a single patch, and furthermore it could require the conceptual expertise of every component maintainer for components missing this documentation.

  • For parameters, it's straightforward to determine the typehint if the function or method already has a typehint -- which only works for non-scalar data types.
  • If there is no method typehint (as will always be the case for scalar data types since we support PHP 5.5 and up), it's more difficult.
  • To understand what data types are allowed as parameters, one needs to read callers as well as the method code.
  • To understand return data types, one often has to know what the data types of the parameters were, but when one is carefully reading the code to understand what data types are being used, this also becomes evident.
  • Knowing the return data type provides information that helps add the return comment documentation.

So, my recommendation for issue scoping:

  1. Fix Drupal.Commenting.FunctionComment.MissingParamName throughout core. That one can be automated.
  2. Identify what remains to be fixed for Drupal.Commenting.FunctionComment.MissingParamType and Drupal.Commenting.FunctionComment.MissingReturnType. Document the violations here.
  3. Treat the param and return data types as documentation patches, where the scope is by concept (e.g. a whole class with its callers and implementations, or a particular component, or tests, etc.). Allow return comments to be added in these smaller patches if they are missing as well.
  4. Fix the remaining Drupal.Commenting.FunctionComment.MissingReturnComment, again scoping them as documentation patches.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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