[META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*

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.

@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?

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

This issue forgot to have a tag. :)

Issue Reminder Comment.

Updated issue summary.

Poll module is no longer in core.

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

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

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

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

Bump up to 8.1.x.

Moving all child issues, too.

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.

OK.

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

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...

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.

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.

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.

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

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

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.

Moved the work from the child patches to #3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType. When applying the patches I found only one or two changes to @return the rest were to @param statements.

Thanks for the work done here!

The community has decided that the best way to approach this is by fixing a rule at a time, rather than a file at a time. See #2571965: [meta] Fix PHP coding standards in core, stage 1 for the meta issue where this effort is being organized, The FunctionComment work is in #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.

Closing this as a duplicate