RFC Declare @throws in phpdoc

[tractorcow]

This suggestion discusses the adding of standard @throws phpdoc to any method that is known to throw exceptions (or calls methods that throw these exceptions).

Pros:

• better visibility to users of what exceptions code can throw.
• Ability to track semver violations where exception types change

Cons:

• If we adopt this, we'll need to essentially add these everywhere we throw exceptions in a big PR.
• We'll need to maintain this going forward also

I suggest that we vote to either go all in on this, or agree to avoid adding this to core.

cc @silverstripe/core-team for discussion.

[kinglozzer]

Strong +1 for adding these

[dhensby]

If we adopt this, we'll need to essentially add these everywhere we throw exceptions in a big PR.

I'm not sure the fact we'd need a PR is a "con"...

[flamerohr]

or calls methods that throw these exceptions

I'm hoping it's a "common sense" type thing here, otherwise this could turn into a maintenance nightmare

[robbieaverill]

+1 for documenting exceptions throw directly by methods. We can't document exceptions thrown by dependant methods really, otherwise everything would probably have an annotation

[maxime-rainville]

I'm in favor, but how does this impacts our commitment to semver. Do we consider this part of our official API?

If it is, then you better think things through before saying "this method throws Exception X", because you've got to wait until the next major release to change it if you change your mind.

[unclecheese]

The contract we have with semver, as I understand it, is to not break anyone's code. This change might affect developer ergonomics, but I don't see how it could break functional code.

[robbieaverill]

Yeah. Documenting existing exceptions doesn't affect semver. Adding or removing actual exceptions would though.

[tractorcow]

ok, just direct methods then. ;)

[sminnee]

Is there a linter / static analyser that can help us with this?

[maxime-rainville]

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="SilverStripe">
    <rule ref="Squiz.Commenting.FunctionCommentThrowTag">
    ...
    </rule>
</ruleset>

PHPCS will warn you if your function has a throwcall without a matching @throws, but not if your function calls a method with a @throws without a catch. For example, this will not throw a warning.

    /**
     * @throws \Exception Boom.
     * @return void
     */
    public function testHello()
    {
        throw new \Exception('boom');
    }

    /**
     * @return void
     */
    public function testWorld()
    {
        $this->testHello();
    }

[dhensby]

Is there a linter / static analyser that can help us with this?

PHPStorm does do this out of the box, but I'm not sure what linters replicate that

[wernerkrauss]

Is there a linter / static analyser that can help us with this?

https://plugins.jetbrains.com/plugin/7622-php-inspections-ea-extended- is a very good and popular plugin that helps me finding this kind of documentation error (and other stuff) in PHPStorm.

It can afaik also fix the missing @throws automatically.

[robbieaverill]

rfc/accepted for ensuring that all methods that directly throw exceptions (rather than methods they call e.g. parent::foo() or $this->bar() throwing them)?

[robbieaverill]

No feedback in 6 weeks, removing feedback required label

[maxime-rainville]

So are we good to start enabling that Squiz.Commenting.FunctionCommentThrowTag rule?

Is there a way to enable it without forcing us to immediately fix them every where?

[dhensby]

@maxime-rainville I think there's a setting to make it only test changed code - but that might mean having to run the function comment throwing rule under it's own process... I'm not sure

[sminnee]

Is there a way to enable it without forcing us to immediately fix them every where?

How much time would it take to add it everywhere in a single package?

[maxime-rainville]

Not sure if this is the best way of doing this, but travis has a TRAVIS_COMMIT_RANGE variable. We could use that to pick out only the files that have changed on the current PR build and run some stricter tests on those files only.