False positive clean code static access for Static Factory Methods

[maxmeffert]

Hi,
I'm getting false positive warnings from clean code static access for Static Factory Methods, e.g.:

<?php
class Foo
{
    public static  function create()
    {
    }   
}

abstract class Bar
{
    public static function foo1()
    {
        Foo::create();
    }

    public function foo2()
    {
        Foo::create();
    }
}

Foo.php:19  Avoid using static access to class 'Foo' in method 'foo1'.
Foo.php:24  Avoid using static access to class 'Foo' in method 'foo2'.

I don't know whether this is intended or not.

[ravage84]

Are you using the StaticAccess rule? If so, then this is correct and to be expected.

[maxmeffert]

Hi,
thanks for the advice.
As you've might have guessed, i'm new to phpmd.
The StaticAccess description says:

Static access causes unexchangeable 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.

So i assume now, the objective of this rule is to detect static dependency injections only, correct?
Personally, i don't consider access to stateless static methods particularly harmful.

Regards.

[ravage84]

Well, PHPMD is a static analysis tool. Means it has limited access in what and how it can analyze it.
Even though the documentation states what you should do, that does not mean that the tool can distinguish between those cases when analysing some code.

For you, this means you should make use of the rules' exception list as documented here:

exceptions Comma-separated class name list of exceptions

[maxmeffert]

Hi,
I did some more reading and I'm quite pleased with PHPMD now.
I was just confused by the StaticAccess description above.

However, after looking at the code, I don't think the CleanCode rules produce helpful results.
At least not for me.

For instance StaticAccess literally only scans for static method calls.
I think this is quite questionable, since stateless/side-effect-free static methods should not be more harmful than simple function calls.
Consider date_create_from_format and DateTime::createFromFormat.
The former is just an alias for the latter, however the latter raises a warning.
It is also not clear to me, why the latter is considered to be an "unexchangeable dependencies" and the function call isn't.

Regarding your statement above:

PHPMD is a static analysis tool. Means it has limited access in what and how it can analyze it.

I think we could improve StaticAccess by checking, if a static method or its subsequent method calls write to a static field or global variable.
Combined with LCOM, this could lead to a strong indicator for static side-effects, which is what the description of StaticAccess discourages.

Regards.

[ravage84]

Most of the rules are very strict and analyze the code in a very simplistic way, thus producing potentially many false positives.
This is not totally unwanted, as you rather want to see too much, instead of risking seeing too few.

When you have (potential) false positives, you have a look at them and then decide in each case, whether this is a candidate for refactoring or if you have an actual false positive (at least in your opinion) and put it onto the exception list.

Changing the behavior of the StaticAccess rule is only possible in a backward compatible way, as some people might depend on its current behavior.

If you want, you can try to implement a setting (which defaults to the current behavior), which makes the rule smarter the way you describe it.

PRs are always welcome.

[maxmeffert]

Hi @ravage84,
sorry for the late come back.
I just had some time browsing through my GitHub history and just wanted to say that I see my errors now.
After some years of practice working with legacy software, analysis tools and continued training in clean code, software design, etc. I can say now from a new perspective that PHPMD behaved correctly and that I did make wrong assumptions. :-)

[ravage84]

Hallo @maxmeffert ,

Thank you for your feedback (better late than never 😁 ).