Verify whether UnusedFormalParameter PHPMD rule exclusion is still needed

[rjkip]

https://github.com/ibuildingsnl/qa-tools/blob/1ef603269b55b1487307bf0a5ef092b68d885779/src/Tool/PhpMd/Resources/templates/phpmd-default.xml.twig#L20-L23

[rjkip]

@rpkamp wrote:

---

It looks like class inheritance/interface implementation is solved now, but there is a problem with callback methods.

This is what I used to test it:

foo.php:

<?php

namespace foo;

class Foo
{
    public function fooMethod($fooParameter)
    {
        echo $fooParameter;
    }
}

ban.php:

<?php

namespace ban;

interface Ban
{
    public function banMethod($banParameter);
}

bar.php:

<?php

namespace bar;

use foo\Foo;
use ban\Ban;

class Bar extends Foo implements Ban
{
    // should not warn for UnusedFormalParameters because this follows the signature of
    // Foo::fooMethod (class)
    public function fooMethod($fooParameter) {}

    // should not warn for UnusedFormalParameters because this follows the signature of
    // Ban::banMethod (interface)
    public function banMethod($banParameter) {}

    // should warn for UnusedFormalParameters because this method does not override
    // or implement anything - this method is to verify that PHPMD actually reports
    // UnusedFormalParameters
    public function barMethod($barParameter) {}

    // should not warn for UnusedFormalParameters because it is impossible to use
    // $callbackArgument2 without also defining $callbackArgument1
    public function callRunCallbackWithJustOneParameter()
    {
        $this->runCallback(function ($callbackArgument1, $callbackArgument2) {
            echo $callbackArgument2;
        });
    }

    private function runCallback($callback)
    {
        $callback(1, 2);
    }

}

And a composer setup that just installs phpmd/phpmd (version 2.5.0 was installed)

When I run ./vendor/bin/phpmd --exclude vendor/ . text unusedcode I get the following output:

bar.php:21  Avoid unused parameters such as '$barParameter'.
bar.php:27  Avoid unused local variables such as '$callbackArgument1'.

The warning about $barParameter is expected, but the warning about $callbackArgument1 should not be there.

However, it does seem that the inheritance/implements problem is solved, which is weird, since the ticket over at PHPMD is still open: https://github.com/phpmd/phpmd/issues/180 :confused:

@DRvanR @rjkip Throughts?

[rjkip]

@DRvanR wrote:

---

bar.php:27 Avoid unused local variables such as '$callbackArgument1'.

Seems like a valid message to me as it is indeed not used locally, which is the smell that is identified.

All in all, I'm in favor of removing the exception. We can always put it back if needed.

[rjkip]

@rjkip wrote:

---

Parameters in callbacks can be forced upon you by third-party APIs, resulting in unused parameters, but I think this issue is more and more an edge case. @SuppressWarnings is still an option in these cases.

We can always put it back if needed. :+1:

---

The UnusedFormalParameter rule still triggers when one implements an interface that is excluded from mess-detection. For example, PHPMD trips over this implemented interface method, because its interface is outside src/, namely inside vendor/. Not sure we should include the rule now.

---

Impact for QA Tools: https://github.com/ibuildingsnl/qa-tools-v3/compare/remove-unused-formal-parameter-rule-exclusion. I'm not sure the gains outweigh the pains for developers. Thoughts?