Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WrongNullable false positive #368

Open
hemberger opened this issue Mar 3, 2023 · 1 comment · Fixed by php-collective/code-sniffer#2 · May be fixed by #369
Open

WrongNullable false positive #368

hemberger opened this issue Mar 3, 2023 · 1 comment · Fixed by php-collective/code-sniffer#2 · May be fixed by #369

Comments

@hemberger
Copy link

LINE 229: ERROR [x] Doc block error: `object|array<mixed>|string|null $object` seems to be having a wrong `null` type hinted, argument is
                    not nullable though. (Spryker.Commenting.DocBlockParamAllowDefaultValue.WrongNullable)
----------------------------------------------------------------------------------------------------------------------------------------
   227: 
   228:     /**
>> 229:      * @param object|array<mixed>|string|null $object
   230:      */
   231:     public function escapeNullableObject(object|array|string|null $object): string {

This false positive occurs for any parameter that uses the "union with null" syntax in the interface typehint and is also included in the doc block. Using the "nullable type" syntax (i.e. ?type) does not trigger this false positive, but it is not possible to use this syntax in cases where the typehint is a multiple union (i.e. ?(type1|type2) or similar is illegal).

The cause of this false positive is the confusingly named nullable_type field in the parameter arrays returned by PHP_CodeSniffer\Files\File::getMethodParameters. It does not identify if the parameter accepts null as one might expect, but rather, it only identifies if the "nullable type" syntax is used. This is clarified in the PHP_CodeSniffer documentation in squizlabs/PHP_CodeSniffer@9a502fd.

hemberger added a commit to hemberger/code-sniffer that referenced this issue Mar 3, 2023
The `nullable` field returned from SignatureTrait::getMethodSignature
is used by DocBlockParamAllowDefaultValueSniff assuming that it means
whether or not the parameter accepts null, so we need to also check if
an explicit null is part of the typehint. This is because in the arrays
returned by File::getMethodParameters, the `nullable_type` field only
indicates if the typehint is prefixed with "?".

Fixes spryker#368.
@hemberger hemberger linked a pull request Mar 3, 2023 that will close this issue
1 task
hemberger added a commit to hemberger/code-sniffer that referenced this issue Mar 3, 2023
The `nullable` field returned from SignatureTrait::getMethodSignature
is used by DocBlockParamAllowDefaultValueSniff assuming that it means
whether or not the parameter accepts null, so we need to also check if
an explicit null is part of the typehint. This is because in the arrays
returned by File::getMethodParameters, the `nullable_type` field only
indicates if the typehint is prefixed with "?".

Fixes spryker#368.
@dereuromark
Copy link
Contributor

Nice find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants