Missing typehints reports when suppressed

[simPod]

So far we have had disabled missing typehints rule:

- "~^(Method|Property) .+::.+(\\(\\))? (has parameter \\$\\w+ with no|has no return|has no) typehint specified$~"

Because it reported errors also for classes where we disabled the same reporting with @phpssSuppress used for CodeSniffer. And that was disabled for a reason.

For example, we have overriden PHPUnit method

    /**
     * Returns a test double for the specified class.
     *
     * @param string|string[] $originalClassName
     *
     * @throws Exception
     * @throws \InvalidArgumentException
     */
    protected function createMock($originalClassName): MockObject
    {
        return $this->getMockBuilder($originalClassName)
                    ->disableOriginalConstructor()
                    ->disableOriginalClone()
                    ->disableArgumentCloning()
                    ->disallowMockingUnknownTypes()
                    ->getMock();
    }

with

    /**
     * @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingParameterTypeHint
     */
    protected function createMock($originalClassName) : MockObject
    {
        throw new \BadMethodCallException('Use Mockery instead.');
    }

We suppresed type hint checks because we have to follow parent signature from external library so types cannot be added.

However, this PHPStan strict check is very handy but also it's very exhausting to manually enumarete all places to ignoreErrors in config.

As example I have modified the MissingMethodParameterTypehintRule rule in a following way

    private function checkMethodParameter(MethodReflection $methodReflection, ParameterReflection $parameterReflection, ?Doc $doc): ?string
    {
        $parameterType = $parameterReflection->getType();

        if (
            $parameterType instanceof MixedType &&
            !$parameterType->isExplicitMixed() &&
            (
                $doc === null ||
                preg_match(
                    '~(@phpcsSuppress SlevomatCodingStandard\.TypeHints\.TypeHintDeclaration\.MissingParameterTypeHint|@inheritdoc)~',
                    $doc->getText()
                ) === 0
            )
        ) {
            return sprintf(
                'Method %s::%s() has parameter $%s with no typehint specified.',
                $methodReflection->getDeclaringClass()->getDisplayName(),
                $methodReflection->getName(),
                $parameterReflection->getName()
            );
        }

        return null;
    }

But I don't see how it would fit in the existing codebase when PR is sent. So I came to ask for ideas/share this snippet for anyone who would like to use it. Feel free to close this, I don't see a way how this could be integrated here in a clean way myself. IMO combining with PHPCS rules is exceeding the responsibility of this library.

[ondrejmirtes]

Correct way is this:

    /**
     * @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingParameterTypeHint
     * @param string|string[] $originalClassName
     */
    protected function createMock($originalClassName) : MockObject
    {
        throw new \BadMethodCallException('Use Mockery instead.');
    }

[ondrejmirtes]

You really shouldn’t ignore this rule, it’s telling you PHPStan does not have enough types info 😊

On Mon, 26 Nov 2018 at 16:24, Šimon Podlipský ***@***.***> wrote: So far we have had disabled missing typehints rule: - "~^(Method|Property) .+::.+(\\(\\))? (has parameter \\$\\w+ with no|has no return|has no) typehint specified$~" Because it reported errors also for classes where we disabled the same reporting with @phpssSuppress used for CodeSniffer. And that was disabled for a reason. For example, we have overriden PHPUnit method /** * Returns a test double for the specified class. * * @param string|string[] $originalClassName * * @throws Exception * @throws \InvalidArgumentException */ protected function createMock($originalClassName): MockObject { return $this->getMockBuilder($originalClassName) ->disableOriginalConstructor() ->disableOriginalClone() ->disableArgumentCloning() ->disallowMockingUnknownTypes() ->getMock(); } with /** * @phpcsSuppress SlevomatCodingStandard.TypeHints.TypeHintDeclaration.MissingParameterTypeHint */ protected function createMock($originalClassName) : MockObject { throw new \BadMethodCallException('Use Mockery instead.'); } We suppresed type hint checks because we have to follow parent signature from external library so types cannot be added. However, this PHPStan strict check is very handy but also it's very exhausting to manually enumarete all places to ignoreErrors in config. As example I have modified the MissingMethodParameterTypehintRule rule in a following way private function checkMethodParameter(MethodReflection $methodReflection, ParameterReflection $parameterReflection, ?Doc $doc): ?string { $parameterType = $parameterReflection->getType(); if ( $parameterType instanceof MixedType && !$parameterType->isExplicitMixed() && ( $doc === null || preg_match( ***@***.*** ***@***.***)~', $doc->getText() ) === 0 ) ) { return sprintf( 'Method %s::%s() has parameter $%s with no typehint specified.', $methodReflection->getDeclaringClass()->getDisplayName(), $methodReflection->getName(), $parameterReflection->getName() ); } return null; } But I don't see how it would fit in the existing codebase when PR is sent. So I came to ask for ideas/share this snippet for anyone who would like to use it. Feel free to close this, I don't see a way how this could be integrated here in a clean way myself. IMO combining with PHPCS rules is exceeding the responsibility of this library. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#51>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuL5f-OZSvnM6fVcSE5Jg3BONOrvZks5uzAfAgaJpZM4YzP-C> .

-- Ondřej Mirtes

[simPod]

You really shouldn’t ignore this rule, it’s telling you PHPStan does not

Sure, trying hard :D

Correct way is this:

Mm, that didn't come to my mind lol. What about @inheritdoc? Do I have to re-specify all types that are inherited anyway then?

[ondrejmirtes]

Try before asking.

[simPod]

Ofc I tried

Parent

    /**
     * Returns the SQL to create an index on a table on this platform.
     *
     * @param \Doctrine\DBAL\Schema\Index        $index
     * @param \Doctrine\DBAL\Schema\Table|string $table The name of the table on which the index is to be created.
     *
     * @return string
     *
     * @throws \InvalidArgumentException
     */
    public function getCreateIndexSQL(Index $index, $table)

Child

    /**
     * @inheritdoc
     */
    public function getCreateIndexSQL(Index $index, $table) : string

Reports

Method ...\PostgreSqlPlatform::getCreateIndexSQL() has parameter $table with no typehint specified.

[ondrejmirtes]

Please always include a phpstan.org link so I can show you how to do it right.

In 0.11-dev, when the phpDoc is missing, it's inherited automatically. But once it exists, it has to list the typehints explicitly or at least include {@inheritdoc} which is the correct syntax.

In 0.10.x, you always have to include the {@inheritdoc}.

[simPod]

I see. It seemes that it works just sometimes when on stable. Tried in playground and also with latest and the issue seems to be fixed there. Thanks.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.