(I don't know if this should go into core issue queue, or into coding standards. Please move if applicable.)
It seems that in Drupal core development, there is an unwritten consensus that all non-public class members (methods, properties) should be protected, not private.
in 8.2.x core/lib/ (with some local diff, but more or less these numbers):
- 1106x "protected function".
- 25x "private function".
- 1671x "protected $".
- 31x "private $".
In vendor/:
- 1327x "protected function".
- 790x "private function".
- 1251x "protected $".
- 1649x "private $".
I would prefer not to have this conversation on every issue I participate in, but rather have it once.
I also would like to avoid turning this into a meaningless bikeshed or a purely opinion-based war with two camps. I hope we can at least achieve a minimal consensus.
A lot of arguments about this can be found on the web. Here I am trying to summarize the points, make it a bit more Drupal-specific, and also shift the focus of the question to something which is hopefully more productive. Instead of "which access modifier is preferable" we should ask "which architectural style is preferable" or "which architectural style are we practically following", and base the decision on that.
General thoughts
The protected access modifier (on a non-abstract member) allows child classes to reuse or override a part of the base class.
On the other hand, these members can no longer be refactored without breaking the child classes (which we may not know about).
In unit tests, public methods can be tested without special tricks. protected and private methods can be tested with reflection (but generally this is not wanted or needed).
Small, "SOLID" classes
If your classes are small and based on SOLID principles, then there is little need for inheritance to reuse parts of a class. The class is already the atomic unit to either reuse or replace as a whole. If any part of a class should be reusable, without reusing the whole class, then this part should probably be factored out of the class: Into a static method (a "pure function", ideally), or into a new, separate class (possibly with an interface, so it can be cleanly injected).
If the respective functionality is too awkward or too one-off to deserve to be in a public static method, or a dedicated class (+ interface), then it is ok to have a common base class with (abstract or not) protected methods. The protectedness of these methods is not because someone might want to override or call them. They are specifically designed for this purpose.
Refactoring this code means either changing private methods (which is ok and risk-free), or creating alternative implementations or even new interfaces, while leaving the old classes and interfaces in place for BC.
Members that are "protected by design" are considered API, and changing them would be considered a BC break.
Bigger, non-SOLID classes
Most of the classes in Drupal are quite big, and far from SOLID. When trying to reuse parts of their functionality, inheritance is often the only or the cheapest option. The "private" access modifier would make this very hard. I think this is how this general unwritten standard came into existence.
On the other hand: Whenever someone tries to refactor these protected methods, the question comes up whether some child class might exist that we would break with this change.
Here, the protected keyword is a sign that we don't really know what we are doing, we don't really have a clear design in mind. Someone might want to override or reuse this in a child class. But we don't really know why or how.
Test classes
Test classes generally are not meant for inheritance.
Helper methods in test classes often have a reuse value. But the ideal way to reuse them is not by inheriting another test class. Instead, they should be moved to a shared base class, or moved to a helper class, or become public static methods (on the test class itself, or an "utility" class).
A question of architectural style
To summarize: There is one style where we stuff a lot of stuff into a class and then someone might want to write a child class. We use protected for everything, because we don't really know what is required for inheritors.
The other style is inheritability by design. For every member we make an intentional choice whether it is designed for reuse or overriding, or not. With this style, "private" is the default, and in the few cases we use "protected", it can be understood as a promise.
Question / proposal
The question is whether the "inheritability by design" approach really has any place in core development.
So far it seems not.
If it does, we need to be able to use "private" more often.
Web references
http://programmers.stackexchange.com/questions/312425/why-have-private-f...
http://programmers.stackexchange.com/questions/162643/why-is-clean-code-...
http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private....