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?

Adulate

Answer 1

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.

Almatadema

Thanks. So, can I turn it off, or avoid it somehow? – Adulate

Oh, I should say I just changed to exception handling. Before I was just using die() but that felt wrong. – Adulate

I'm not intimatily familliar with PHPMD but it should understand the @SuppressWarnings annotation. – Almatadema

Answer 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."));

Adulate

Yes, PHPMD successfully spots the hidden dependency on InvalidArgumentException here. However like @Almatadema commented as well in his answer 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. – Jeter

@Jeter 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. – Exegesis

@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). – Jeter

@Jeter 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. – Exegesis

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. – Jeter

Answer 3

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.

Singh

Answer 4

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>

Delectate