Avoid using static access to Exception

Question

I've just fired up PHPMD for the first time and, predictably, I've got an error I can't figure out. The error is

Avoid using static access to class 'InvalidArgumentException' in method 'setLang'.

and the code is

public function setLang($val0) {
    switch ($val0) {
    case ENG:
    case FRE:
    case SPA;
        $this->lang = $val0;
        break;
    default:
        throw new InvalidArgumentException("Invalid language choice.");
    }
}

I've tried a variety of different things but I think at the end of the day Exception is a static factory (???) so it must have static access. But, the PHPMD guys are for sure smarter than me so that wouldn't have fazed them.

Why does this warning exist, and how to solve it?

complex857 · Accepted Answer · 2017-09-17T11:43:11.383

12

The idea behind this warning is that if you embed the class names inside your code with the new keyword it will be hard to swap out these classes in testing and mock or stub methods that the code under test might call on them. See the explanation in the PHPMD rules.

I think in your case this is false positive since exceptions usually doesn't have much behavior on their own but the class name (and class hierarchy behind it) of them is pretty much the only thing important about them.

If you want to get rid of the warning here, you can use the @SupressWarnings annotation here.

edited Sep 17 '17 at 11:43

answered Sep 03 '13 at 06:03

complex857

20,425
6
51
54

Thanks. So, can I turn it off, or avoid it somehow? – Ben Sep 03 '13 at 06:06
Oh, I should say I just changed to exception handling. Before I was just using `die()` but that felt wrong. – Ben Sep 03 '13 at 06:07
1

I'm not intimatily familliar with PHPMD but it should understand the [`@SuppressWarnings`](http://phpmd.org/documentation/suppress-warnings.html) annotation. – complex857 Sep 03 '13 at 06:08

Ben · Answer 2 · 2013-09-03T07:11:34.120

2

Ahh, after a bit of digging around I have found the answer, straight from the horse's mouth.

In the configuration files, located at yourphpdir\data\PHP_PMD\resources\rulesets, the cleancode.xml has CDATA comments which explain the setting.

This one says:

Static acccess causes inexchangable dependencies to other classes and leads to hard to test code. Avoid using static access at all costs and instead inject dependencies through the constructor. The only case when static access is acceptable is when used for factory methods.

The way to solve it is just pass the exception as a parameter, so it's declared outside of the class.

$foo->setLang("Oh noes!", new InvalidArgumentException("No lang for you."));

edited Sep 03 '13 at 07:11

answered Sep 03 '13 at 06:15

Ben

54,723
49
178
224

2

Yes, PHPMD successfully spots the hidden dependency on `InvalidArgumentException` here. However like @complex857 commented as well in his [answer](http://stackoverflow.com/users/1515540/complex857) I'd say that this is an edge-case as throwing an exception normally requires a non-injected class-name. Like within the factory, the exceptions class-name is a property of your code, not an inject-able dependency (at least not on this low level). And you did right replacing `die()` in the first place. Concentrate on killing all `die()`s first :D. – hakre Sep 03 '13 at 06:41
@hakre the explanation is good. However, I have to agree that this seems a false positive, as you might return various exceptions from a library routine indicating the issue (Setting language might not have the language installed, user security, no translations, etc...) and the exception could therefore be of different types so the caller could continue/abort based on the issue. I think this needs to be adjusted in the PHP_MD to help ignore false positives, as I do not want to have a lot of @ SuppressWarnings(PHPMD.StaticAccess) in the code. – Steven Scott Dec 12 '13 at 22:28
2

@StevenScott: I find it a bit too harsh to coin it "false positive". As PHPMD does static code analysis, it can only report points within the code to you that violate (or are catched) by some rule. It's up to the programmer to both read this output and understand the rule, then decide on what to do. A static code-analysis can never to these programmers steps, it's only a tool that can help in keeping an overview about the codebase at large. Saying so, there might be exceptions to rules that can make sense (sometimes). – hakre Dec 16 '13 at 10:22
@hakre True about the "false positivi" but if it causes an automated build some problems, it will waste a bunch of time until the remove is removed, but may still be valuable in other cases. – Steven Scott Dec 19 '13 at 02:52
I would not consider that time wasted. It's either a better configuration of the built or valuable feedback that is of use in other projects as well. It's perhaps worth to not use that check in the built if you have this reporting each time but you want to have it ignored. – hakre Dec 19 '13 at 09:37

score 0 · Answer 3 · answered May 25 '14 at 15:37

Because I use a lot of self:: for constants, change phpmd code to accept self:: and parent::.

In the program PHP/PMD/Rule/CleanCode/StaticAccess.php at line 36, change to:

if ($this->isReferenceInParameter($reference)
    || $reference->getImage() === 'self' 
    || $reference->getImage() === 'parent' 
    ) {
    continue;
}

Maybe you can use that to refine the code.

score 0 · Answer 4 · answered Jul 30 '20 at 06:10

If you use an XML ruleset, you can exclude certain classes (full namespace) from this rule:

    <rule ref="rulesets/cleancode.xml">
        <exclude name="StaticAccess"/>
    </rule>
    <rule ref="rulesets/cleancode.xml/StaticAccess">
        <properties>
            <property name="exceptions" value="\Drupal\views\Views,\Drupal\Core\Access\AccessResult" />
        </properties>
    </rule>

Avoid using static access to Exception

4 Answers4