Exception naming sniffs #63

SenseException · 2018-06-08T11:36:46Z

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.

greg0ire · 2018-06-09T09:31:33Z

phpunit.xml.dist

+        3) To run the tests against the database type the following from within the
+           tests/ folder: phpunit -c <filename> ...
+           Example: phpunit -c mysqlconf.xml AllTests
+-->


Probably needs to be removed since this repo does not have to do with dbms'es

True. I guess we don't need the hint to copy the dist-file into a xml either, so I removed it.

alcaeus · 2018-06-11T04:55:40Z

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 · 2018-06-11T09:02:38Z

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

Yes, they are :)

alcaeus · 2018-06-11T09:14:52Z

Wonderful, thanks!

SenseException · 2018-06-11T10:24:24Z

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

Majkl578

This sniff will be really helpful (not only) for ORM 3.0, DBAL 3.0, Migrations 2.0 and other libraries.

I'd also love to see a sniff that validates actual throw that we don't throw i.e. SPL exceptions. But that'd be separate task/sniff. :)

tests/Doctrine/Tests/Sniffs/Classes/ExceptionInterfaceNamingSniffTest.php

phpunit.xml.dist

lib/Doctrine/Sniffs/Classes/ExceptionInterfaceNamingSniff.php

.travis.yml

lib/Doctrine/Sniffs/Classes/ExceptionInterfaceNamingSniff.php

SenseException · 2018-06-18T07:36:00Z

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

SenseException · 2018-07-10T21:07:59Z

lib/Doctrine/Helpers/UseStatementHelper.php

+use PHP_CodeSniffer\Files\File;
+use SlevomatCodingStandard\Helpers\UseStatementHelper as UseHelper;
+
+class UseStatementHelper


I'm not a fan of Helper classes or a helper-namespace, but I kept naming and usage of static methods for consistency. If we should use a different approach for outsourcing logic, please let me know.

It actually fits it better to use non-static class accepting File $phpcsFile in its constructor.

Majkl578 · 2018-07-11T02:10:13Z

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 · 2018-07-17T20:26:10Z

tests/Doctrine/Sniffs/Classes/ExceptionClassNamingSniffTest.php

        ];
    }

+    /**
+     * @return string[][]|int[][]


How does the correct syntax for multidimensional arrays look like? Seems like the sniff is satisfied with multiple possible formats.

This is correct. (string|int)[][] would be more correct, but it is not supported by most tooling yet :-\

SenseException · 2018-07-22T20:50:54Z

Addition to Exception classes: They have to be final.

Ocramius · 2018-08-31T14:52:14Z

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 · 2018-09-01T18:44:06Z

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

Majkl578

I did a more thorough review - let me know if something is not clear, reviewing sniffs isn't easy since different people choose different approaches for the same outcome. :)

Majkl578 · 2018-09-01T20:29:42Z