Add MethodVisibilityOverrideRule

[jlherren]

Is this something that would fit into this repo? It adds a rule to report method visibility overrides from protected to public (the only combination that is problematic and not already a hard error). This is similar to PhpStorm's inspection "PHP | Code smell | Method visibility should not be overridden". The only difference is that it doesn't complain about constructors, since I don't think changing a constructor's visibility is necessarily a bad thing (up to discussion of course).

Note that phpstan-src violates this rule for a handful of method in the test suite, none of which seems justified.

[ondrejmirtes]

Hi, thank you, what I'm missing here is the explanation of how is this a code smell? I don't see any code smell, bugginess, or technical debt coming from this situation.

[jlherren]

Some devs on my team kept overriding method visibilities for no reason, so I started to enforce it with this rule. More often than not it was just accidentally exposing methods that weren't meant to be public.

I can see that this might be controversial though. In some (maybe rare) cases it would make sense, for example:

1. A sub-class indeed wants to expose a method whereas the parent class does not do so. While technically a valid situation, I would encourage a developer to write an additional public method that calls the protected one. This avoids having the same method name in a class hierarchy refer to a public method in one class, but to a protected one in another class, since that might lead to confusion.
2. A sub-class additionally implements an interface with a name and implementation that coincides with the name of a protected method of a parent class. A similar argument applies here.

I have personally never encountered any situations where I needed this, where it wasn't accidental or where there wasn't a better alternative. Of course this doesn't necessarily mean anything, but it's why my gut feeling tells me it is a smell. PhpStorm having an inspection for it and calling it a smell as well kinda cemented that view for me. But I cannot provide any arguments beyond this, it doesn't seem to be a widely known and established code smell. I won't be offended if people disagree with me on this and it doesn't get merged, as long as I can enforce it on my team :)

This might also be interesting: https://stackoverflow.com/questions/12780779/why-java-allows-increasing-the-visibility-of-protected-methods-in-child-class/12781210

[ondrejmirtes]

The accepted SO answer basically says it's fine to do it :)

Personally I consider protected being much closer to public than private. From the point of encapsulation it totally is. class Bar extends Foo is the same thing as new Foo().

Thank you for this contribution. It might be fine for your team to enforce it, but for my taste it's too opinionated and bound to mark code that I consider absolutely fine.

[jlherren]

That's fine, no worries :)

[mfn]

@jlherren I did run your code in a private codebase and immediately found unreasonable changes in visibility 😅

I therefore would be happy to have it available: are you planning on releases it on your own or something?

thanks

[jlherren]

@mfn I'm happy it was useful :) I currently don't plan to release this on its own, as it doesn't really make sense for just this one rule.

If you want to use it for a single project, it's very easy to just commit it directly into the project and add it to the phpstan config.