Skip to content

integer type hints appearing as TypeHintMissing instead of ScalarTypeHintMissing #1394

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

Closed
h-bragg opened this issue Mar 20, 2017 · 6 comments

Comments

@h-bragg
Copy link

h-bragg commented Mar 20, 2017

Since upgrading to 2.8.1 integer types on method arguments have been throwing TypeHintMissing instead of ScalarTypeHintMissing.

Config:

<?xml version="1.0"?>
<ruleset name="graze/standards">
    <description>The graze PHP coding standard as defined in graze/standards.</description>
    <rule ref="vendor/graze/standards/PHP/CodeSniffer/Graze/ruleset.xml" />

    <rule ref="Squiz.Commenting.FunctionComment">
        <exclude name="Squiz.Commenting.FunctionComment.ScalarTypeHintMissing" />
    </rule>
</ruleset>

Code:

    /**
     * @param callable $worker
     * @param int      $limit
     */
    public function receive(callable $worker, $limit = 1)
    {
        ...
    }

2.8.0:

$ docker run --rm -t -v $(pwd):/opt/graze/queue -w /opt/graze/queue graze/php-alpine:test vendor/bin/phpcs -p --warning-severity=0 -s src/ tests/
......................................

Time: 1.49 secs; Memory: 10Mb

2.8.1:

$ docker run --rm -t -v $(pwd):/opt/graze/queue -w /opt/graze/queue graze/php-alpine:test vendor/bin/phpcs -p --warning-severity=0 -s src/ tests/
FILE: /opt/graze/queue/src/Client.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 71 | ERROR | Type hint "integer" missing for $limit
    |       | (Squiz.Commenting.FunctionComment.TypeHintMissing)
----------------------------------------------------------------------
@h-bragg h-bragg changed the title integer type hints appearing as TypeHintMissing instead of ScalarTypeHintMissing integer type hints appearing as TypeHintMissing instead of ScalarTypeHintMissing Mar 20, 2017
@gsherwood
Copy link
Member

2.8.0 didn't generate any type hint missing errors for that code. So it wasn't that it was a different code - it didn't generate any codes.

2.8.1 did fix this, but the code you get (scalar or not) seems to depend on if you used int or integer in your comment. I need to check why that is because it should be consistent.

gsherwood added a commit that referenced this issue Mar 21, 2017
@gsherwood
Copy link
Member

I've pushed up a fix for this inconsistency when using PHP7+. The sniff was expecting you to write integer and generating an error for that (Expected "integer" but found "int" for parameter type (Squiz.Commenting.FunctionComment.IncorrectParamVarName)). But it then ignored the PHP7 check to make the type hint int because int was not in a whitelist of types.

The PHP7 check is now done even if the first check fails to find the value in the whitelist. The result is that int is suggested as the type hint when both int and integer are used in the comment, and when running under PHP7.

I think this fixes your specific issue. But please let me know if I've misunderstood and there is another issues as well.

@h-bragg
Copy link
Author

h-bragg commented Mar 22, 2017

This fixes my issue, thanks!

@h-bragg
Copy link
Author

h-bragg commented Mar 22, 2017

Although running through the tests with 5.6 it will still complain about this (I guess because it is actually expecting integer). Meaning it still throws TypeHintMissing when running on 5.6.

gsherwood added a commit that referenced this issue Mar 23, 2017
…P5, and with the wrong hint text (integer vs int)
@gsherwood
Copy link
Member

@h-bragg Thanks for that extra report. The unit tests had this behaviour coded into them, but it doesn't make sense to show scalar type hints for PHP5. It's left-over code from before the sniff supported the php_version config var, and has now been fixed.

@h-bragg
Copy link
Author

h-bragg commented Mar 23, 2017

@gsherwood This is now working in 5.6, 7.0, 7.1 and hhvm 👍

Thanks for the excellent responses and speedy fixes! 👏

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

No branches or pull requests

2 participants