Exception naming sniffs

[SenseException]

I've started to create sniffs for exception naming as described in the readme: Abstract exception class names and exception interface names should be suffixed with Exception.

This PR isn't a finished one yet and contains the first draft for naming exception interfaces. As this is about static code analysis, I had to make some assumptions about the parsed code.

Interfaces

• If an interface has the name suffix *Exception, I assume, that the interface is an exception interface and needs to be handled by the sniff.
• If an interface extends an exception interface, I assume, that the interface has to be an exception and needs to be handled by the sniff.
• If an interface class extends \Throwable, it is definitely an exception and needs the *Exception suffix.
• If one of the extended interfaces is an exception, I assume that the interface extending them is an exception interface too.

This one is covered for most cases with the creation of the PR. There are still forms of interfaces I need to cover and the sniff isn't finished either, but there's still room for feedback on the existing code.

The sniff is currently not covering use cases, where class alias, comma separation and group use declaration is used, but the dependency slevomat/coding-standard has helper classes like UseStatementHelper, that can help parsing those. Should I use the classes of this dependency for doctrine's custom coding standard sniffs or should I not introduce code from dependencies there?

Classes

~~This still needs to be handled with this PR.~~

• If an abstract class has the name suffix *Exception, I assume, that the class is an exception class and needs to be handled by the sniff.
• If a class extends an exception class or implements an exception interface, I assume, that the class has to be an exception and needs to be handled by the sniff.
• If one of the extended classes or implemented interfaces has the suffix *Exception in its name, I assume the class is an exception class and needs its name checked.

Question

Should an abstract class only extend other abstract exception classes and basic PHP exception classes? Otherwise how do I know that an extended class without *Exception in its name is a valid regular exception class as the coding standard allows? Should I check if the class is in an exception namespace? Does the standard expect all exceptions in an exception namespace?

Addendum: leave exception classes have to be final. Exception namespaces are not in the scope of this PR.

[alcaeus]

The sniff is currently not covering use cases, where class alias, comma separation and group use declaration is used, but the dependency slevomat/coding-standard has helper classes like UseStatementHelper, that can help parsing those. Should I use the classes of this dependency for doctrine's custom coding standard sniffs or should I not introduce code from dependencies there?

That depends on whether those classes are part of the public API: if they are, go for it. @kukulich could you clarify whether those helpers are public API covered under Semver rules?

Should an abstract class only extend other abstract exception classes and basic PHP exception classes?

The assumption would make the sniff considerably easier, but strictly speaking, this isn't the case. I believe that we should take the shortcut and then expand the sniff once it's necessary to do so.

[kukulich]

could you clarify whether those helpers are public API covered under Semver rules?

Yes, they are :)

[alcaeus]

Wonderful, thanks!

[SenseException]

Great. I'll refactor that part using the UseStatementHelper. :-)

[SenseException]

@Majkl578 I've updated the PR for the interface handling.

[Majkl578]

I'll try to provide a more detailed review in the afternoon, here are just a few issues I spotted:

• namespace should be Doctrine\CodingStandard, or if not possible, only DoctrineCodingStandard
• please make sure tests/ is covered by our CS too (except the tested files themselves) - this will point out some CS issues already there (missing declare()s, redundant @param. missing return types, missing spaces around concatenation)
• try to upgrade to PHPStan 0.10 with phpstan-strict-rules, putting configuration into phpstan.neon.dist

If you want, I can provide some base for writing/testing sniffs, similar to how slevomat/coding-standard and cdn77/coding-standard are currently tested. We should also finally rework the way we test our CS (not in this PR), because the current way of maintaining tests/input + tests/expected_report.txt is really messy and not maintainable. :)

Feel free to ping me on Slack if you want to discuss any of above or anything else related to the CS. 👍

[SenseException]

Addition to Exception classes: They have to be final.

[Ocramius]

Addition to Exception classes: They have to be final.

Disagree on exception classes being final - unless we really segregate all exception types to interfaces (which would be OK), exceptions do in fact match the typical inheritance-first approach.

This means that each exception is required to implement at least 2 interfaces.

[Majkl578]

Disagree on exception classes being final - unless we really segregate all exception types to interfaces (which would be OK), exceptions do in fact match the typical inheritance-first approach.

I'd say the ideal solution would be something like this:

• leaf exceptions must be final
• leaf exceptions must either:
  • extend an abstract ancestor exception, or
  • extend a root (SPL) exception and implement an exception marker interface
• exception marker interfaces must either:
  • extend Throwable, or
  • extend another marker interface from the same or any upper namespace
• intermediate (abstract) exceptions should be avoided, if used, they must either:
  • extend a root (SPL) exception and implement an exception marker interface, or
  • extend another intermediate (abstract) exception

Interface-based hiearchy should be generally always preferred and usage of intermediate (abstract) exceptions should be rather exceptional. Example:

Throwable
┗ [IM] Doctrine\DBAL\Exception\DbalException
       ┗ [IM] Doctrine\DBAL\Driver\Exception\DriverException

RuntimeException + Doctrine\DBAL\Driver\Exception\DriverException
┗ [A] Doctrine\DBAL\Driver\Exception\ContextualException (w/ i.e. `getSQLState()`)
        ┗ [L] Doctrine\DBAL\Driver\Exception\FailedQuery

[I] = exception interface
[M] = marker interface
[A] = intermediate (abstract) exception
[L] = leaf (final) exception

[SenseException]

Not ready yet, but I'm almost done.

[SenseException]

Ready for another review.

[SenseException]

This PR should also get an addition to the documentation for the new rules.

[SenseException]

phpcbf seems to change the docblock of tests/input2/test-case.php from

    /**
     * @test
     *
     * @covers MyClass::test
     *
     * @uses MyClass::__construct
     */

to

    /**
     * @uses MyClass::__construct
     *
     * @test
     * @covers MyClass::test
     */

The Apply fixes stage build fails.

(PR #91 handles this case)

[SenseException]

I've rebased to the current branch to keep this PR up to date.

[ostrolucky]

I agree with @Ocramius, not liking idea about any exception classes being final.

On different note, even though this PR seems like great work, it does too much rules at once, which is why people hesitate to review it. Having less rules at least initially would allow less disagreements and easier time to review and merge it.

[Ocramius]

@SenseException do you believe this to be ready for review or still WIP?

[SenseException]

I would prefer reviews for the changes @Majkl578 and I did. Feedback can help development on this PR. It does a lot of stuff and there might be still some uncovered cases. The question about having final a few months ago is still open, which started in Slack and wasn't discussed here much. This is a contribution that involves first custom code for this repository after all.

[SenseException]

I would prefer reviews before I rebase against master.

[Ocramius]

Deferred to 8.0.0

[carusogabriel]

What is the status of this PR?

[SenseException]

I'm currently not working on this WIP and don't know when I'll come back to it.