Page MenuHomePhabricator

phpcs ObjectTypeHintParam is incorrect.
Closed, ResolvedPublic

Description

When using object as the type in a @param declaration, phpcs reports a MediaWiki.Commenting.FunctionComment.ObjectTypeHintParam error. The error message says that a class name or stdClass should be used instead. To quote:

| ERROR | `object` should not be used as a typehint. If the
|       | types are known, list the relevant classes; if this is
|       | meant to refer to stdClass, use `stdClass`
|       | directly.

This makes it impossible to correctly declare a parameter that does indeed take any object. stdClass should not and cannot be used for this purpose, since it implies that a "plain" object is expected, and at least phpstorm will complain if this isn't the case.

It seems to me that this rule should simply go away. "object" is a type hint supported by PHP natively, and it serves a valid purpose. It seems like quite a few extensions already disable the check for this reason.

Event Timeline

I believe the intention at T264948 and https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/632993/ was to have the issue suppressed in the few places that object is correct

Indeed. This is not a "this is wrong" rule, but an "are you sure this is correct" rule. Exceptions should be marked with // phpcs:ignore MediaWiki.Commenting.FunctionComment.ObjectTypeHintParam for a single line, // phpcs:disable … and // phpcs:enable … for larger blocks or files, or entirely disabled via <exclude name="MediaWiki.Commenting.FunctionComment.ObjectTypeHintParam" /> <severity>0</severity> when a codebase contains to many exceptions.

To be honest we have very few rules that behave like this. Pretty much all of them are "this is wrong and should be fixed" rules – which I believe explains the confusion. Personally, I try hard to not introduce rules that have to many exceptions. @daniel, what is your use case? How many exceptions are we talking about?

PS: The fact the rule is currently disabled in so many codebases simply is that we didn't had enough time to fix them. As far as we know most of them can be fixed, i.e. all object replaced with stdClass.

Various extensions get fixed in the past weeks

https://codesearch.wmcloud.org/search/?q=ObjectTypeHint&i=nope&files=&repos=

Some libs are not fixable, because there are valid use cases when working with ServiceContainer or such.

Indeed. This is not a "this is wrong" rule, but an "are you sure this is correct" rule.
...
To be honest we have very few rules that behave like this. Pretty much all of them are "this is wrong and should be fixed" rules – which I believe explains the confusion.
Personally, I try hard to not introduce rules that have to many exceptions. @daniel, what is your use case? How many exceptions are we talking about?

Perhaps then it can be demoted from ERROR to WARNING? Are warnings visible enough to not routinely be ignored?

In any case, I'd suggest to change the wording. How about this:

`object` should only be used as a typehint if any and all objects are allowed. If expected classes or interfaces are known, list them; if only plain anonymous objects are expected, use `stdClass`. If the intent is indeed to allow any object, use @phpcs:ignore or @phpcs:disable to bypass this rule.

My use case is a utility method that emits a warning when a deprecated method is overridden, see https://gerrit.wikimedia.org/r/c/mediawiki/core/+/644931/4/includes/debug/MWDebug.php#240. I already figured out that I need @phpcs:disable MediaWiki.Commenting.FunctionComment.ObjectTypeHintParam there, but the reported error seems off.

Change 654807 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/tools/codesniffer@master] Update error message about "object" type hints

https://gerrit.wikimedia.org/r/654807

Change 654807 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Update error message about "object" type hints

https://gerrit.wikimedia.org/r/654807

DannyS712 assigned this task to thiemowmde.
DannyS712 removed a project: Patch-For-Review.
DannyS712 moved this task from Untriaged to Accepted rule changes on the MediaWiki-Codesniffer board.