Add NoUnneededFinalMethodFixer

[Slamdunk]

 final class Foo {
-    final public function foo() {}
-    final protected function bar() {}
-    final private function baz() {}
+    public function foo() {}
+    protected function bar() {}
+    private function baz() {}
 }

[keradus]

Not sure about the rule itself.
Same way, one could say that public in public function is unneeded

[julienfalque]

You can replace these isAnyTokenKindsFound() calls with a single isAllTokenKindsFound() call.

[Slamdunk]

Done, thank you

[Slamdunk]

Not sure about the rule itself.
Same way, one could say that public in public function is unneeded

@keradus you are right; my reasoning is:

1. I want a standard, be it with or without final; having final classes with half final methods and half non-final methods is nonsense

2. What standard should we pursue?

Well, visibility needs to be explicit because in this example:

class A {
    protected function foo() {}
}
class B extends A {
    function foo() {}
}

PHP has strictly rules about the result, but the developer could be confused by what should be the outcome (hit: B::foo() becomes public)

On the other side, final keyword in a final class has no interaction with possible parent classes and as far as I can see there is no misunderstanding of what the outcome is, that is the same with or without final.

So my choise is to raise verbosity for visibility, and to reduce verbosity for final

[keradus]

why raising verbosity in one place and reducing in another? this looks not consistent

[Slamdunk]

Because as I said in one context it helps, in the other it doesn't.

BTW I can make it configurable if you prefer.

[keradus]

I agree that some enforced standard to have about this is nice.
While final methods in final class could look superfluous, they follow defensive approach:
when having final class + final methods, if one drop final from class, then he need to decide which methods he want to open as well. comparing to final only on class level - droping one final would open every method.

[Slamdunk]

This is a good point.

In our business this can't happen because we adopt two harsh custom fixers:

1. All non-abstract classes MUST be final
2. All abstract classes MUST have public methods marked ad final

To reduce inheritance nightmares and increase template method pattern.

I told you this not advertise something, but to drop some thoughts about the topic.
We would never benefit from a fixer that marks methods as final in a final class.

[keradus]

FYI: #1389 ;)

If you go extra defensive like that, it's cool and not needed for you indeed.
But when one don't follow that approach and has only some of the class marked as final, he would benefit from it.

[Slamdunk]

Knew #1389 👍 but never used because it requires an annotation to be written; I use PHP-CS-Fixer in CI to get rid of lazy developers 😸

It seems we all understood the topic, but what about the solution?
Is it ok for you to make it configurable, with default behaviour as adding the final keyword?
If yes, @julienfalque please help me to find a better fixer name, you words master 🙏

[keradus]

To be honest, not sure... yet ;)
I do see the value of the fixer, I find your reasoning valid in your case for sure.
On the other side, I want to promote only good, generic rules. That's the reason why we don't have option to remove public before function.
Not claiming this rule in current form is bad, don't take it like that. I just need to think a bit more about it ;)

Other opinions are welcome of course ;)

[julienfalque]

IMO this PR can be merged as is. I don't see the benefits of a defensive approach with final keyword on each method when the class itself is final. I would not make the fixer configurable, and its name looks good to me :)

[keradus]

btw, if you would add it to .php_cs.dist, what will happen ?

[keradus]

you basically traverse whole tokens of single class twice - one to find it's end, one to fix it's content.
not needed, you may determine it's end while fixing it.
pass only $classOpen here,
then, in fixClass, if you see nested {, it's some method body opening, you skip whole block.
if you find }, it's close } of class. voila, you just found a $classClose, no need to pass it to fixClass method, no need to determine it in applyFix, and you could return it via fixClass so you could still do the jump $index = $classClose

[Slamdunk]

New logic applied.

FYI, this code was copy-pasted from ProtectedToPrivateFixer, and the algorithm is a @SpacePossum optimization: Slamdunk#1

After this, we should refactor other fixers too.

[Slamdunk]

btw, if you would add it to .php_cs.dist, what will happen ?

Nothing because the only final method here is this one, and is legitime:

https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/63661f3add3609e90e4ab8115113e189ae547bb4/src/AbstractFixer.php#L66

[keradus]

please add it to .php_cs.dist file

[Slamdunk]

Already done

[keradus]

then, my other concert is still in place ;)

[Slamdunk]

I'm working 🏃

[keradus]

end is never reached.
then, when you have open for class, end for file is weird ;)

[Slamdunk]

Yes end is never reached, but without it we have to use a while and I dislike loops without a declared end, it is far easier to end in an infinite loop.
I'll change the name in the next commit.

[keradus]

if $end === $tokens->count(), no need to pass duplicated data

[Slamdunk]

Done

[keradus]

if class is final we could just jump over whole class, right? no need for iterating it manually, jumping over single methods and so

[Slamdunk]

Right. I just tried to write less code, but it seems is less readable.

[julienfalque]

Is this empty line intended?

[Slamdunk]

I find it usseful to separate to-be-changed cases from dont-touch ones

[SpacePossum]

@keradus can you look at the failing utest? https://travis-ci.org/FriendsOfPHP/PHP-CS-Fixer/jobs/266908788#L757 (not sure who to ping)

This PR looks good to me :)
I would however like to see a utest to be added with multiple candidates in one test/file (like <?php class A {} class B{}). Pretty sure the fixer will work correctly, but the added test will prevent regression.

[keradus]

you already approved a fix for that failure #2989

[keradus]

Thank you @Slamdunk.