Namespaces.ReferenceUsedNamesOnly & Throwable

[fprochazka]

Hi, can you tell me please what is the best way to solve the following problem? We want to check if Throwable is used absolutely in catch statements, same as other exceptions.

I know I can solve it simply using the specialExceptionNames property like this

<rule ref="SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly">
    <properties>
        <property name="specialExceptionNames" type="array" value="
            Throwable
        "/>
    </properties>
</rule>

But wouldn't it be cleaner to have a condition for it in the sniff? Since Throwable is not really a "special case" it should imho have same behaviour as other exceptions and so should the other new exceptions in PHP7.

What do you think? How should this be solved? I'd like to discuss this before I send a PR and spend time writing tests.

[ondrejmirtes]

I think it's not enough to change just ReferenceUsedNamesOnly, but also FullyQualifiedExceptionsSniff.

There is a clear omission in these sniffs that didn't occur to me once we started using PHP 7 - because no one ever put Throwable in a use statement here 😄

Adding mention about Throwable to ReferenceUsedNamesOnly will help when allowFullyQualifiedExceptions is set to false.

Adding Throwable to FullyQualifiedExceptionsSniff will mark catch (Throwable $e) as an error which is your desired behaviour.

You have to do both to have consistent behaviour.

Feel free to open a PR (with tests), otherwise I will do it in a few days and merge it into master (no reason to wait until 2.0 with full PHP 7 support, because this is backwards-compatible).

[fprochazka]

Thanks for the hint! I have something like this - which is good enough for me right now.

    <rule ref="SlevomatCodingStandard.Namespaces.FullyQualifiedExceptions">
        <properties>
            <property name="specialExceptionNames" type="array" value="
                Throwable
            "/>
        </properties>
    </rule>
    <rule ref="SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly">
        <properties>
            <property name="allowFullyQualifiedExceptions" value="true"/>
            <property name="specialExceptionNames" type="array" value="
                Throwable
            "/>
        </properties>
    </rule>

I wanted to send the PR myself, but got confused by your tests. If you'd have time to solve this, that would be great :)

[ondrejmirtes]

Can you verify that PR #42 fixes your issue without the need for special configuration mentioning Throwable? Thanks.

[ondrejmirtes]

It just occured to me that I should implement checking for types ending with Error too.

[ondrejmirtes]

Alright, updated.

[ondrejmirtes]

I merged the PR #42: d2989ad

[VasekPurchart]

Thanks! :)

[github-actions]

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