[policy, no patch] Private vs protected, and the role of inheritance

Well, I would have some idea how the plugin system could have been done.. but this is too late now I suppose.
Just had a quick look at BlockPluginInterface. extends 5 other interfaces, and adds 8 additional methods into the mix. This system demands inheritance as a primary tool for code reuse, and thus demands the protected keyword.

But in some cases we are actually writing new code, and have a choice.

I opened #2956549: Policy: Atomic behavior objects , which proposes to add more "behavior objects" as opposed to other object types.
For such objects, final and private keywords are a natural fit.

I found a nice example of this problem.

Drupal\Core\Utility\ThemeRegistry and some other classes currently inherit from Drupal\Core\Cache\CacheCollector.

All properties of CacheCollector are protected instead of private.
Classes that inherit from CacheCollector can and do all read from and write to these properties.

This means, on "Find usages" for e.g. CacheCollector::$storage, the IDE shows me 8 distinct files/classes.
I was originally trying looking for refactoring options for ThemeRegistry, but instead I am now looking at CacheCollector.

If those properties were private, it would force a nice separation between the base class and its child classes.
The method CacheCollector::get() would have to write the result of ::resolveCacheMiss() into ->storage, so that ::resolveCacheMiss() no longer needs to do that.

Even better, we could have an injected dependency provide the functionality of ::resolveCacheMiss(), so that we wouldn't need to inherit from CacheCollector all the time.

Re #11: This also makes it impossible, or very hard, to ever refactor CacheCollector, because its implementation details leak to child classes.
The only thing we could do, without breaking child classes, is to create a CacheCollector2 with private properties, which could be used as an alternative.

E.g currently we initialize CacheCollector::$storage with NULL values, and then check isset() + array_key_exists() for ->has().
When looking at this code, I had the idea to introduce a separate property ::$knownKeys which would be initialized with TRUE instead of NULL, so that one isset() would be enough, and ::$storage would be used exclusively for actual values.

It can be debated whether this would be an improvement. But the current architecture with "protected" instead of "private" shuts this down before we even try.

I agree, private works when you have small classes, but we aren't there yet,

Just had a quick look at BlockPluginInterface. extends 5 other interfaces, and adds 8 additional methods into the mix. This system demands inheritance as a primary tool for code reuse, and thus demands the protected keyword.

I have to correct myself.

Indeed for big classes, inheritance is the only viable way to extend or modify functionality.
For this we want overrideable protected methods.

However, a subclass does not necessarily need access to all properties.
Instead of accessing parent properties directly, a subclass can intercept them in the constructor and write its own copy in its own private variables.
This may ring an alarm of "Oh no, memory usage doubled!" But most of these are just object pointers, and the containing class will be instantiated only once per request if it is a service.

And for those who just find it awkward to have multiple copies of the same value in an object:
The benefit of private properties outweighs this supposed awkwardness.

All usages of a private property, read and write, will be in the same file. This would be a great benefit compared to the current situation.

Such a change does require some rethinking, but it does not require small classes.

@Sam152 (#14)

Is there anything preventing someone from writing a core patch with private methods right now or providing that feedback in a review?

It has been brought up in code reviews.

#2953921-14: Refactor out theme hook suggestion building from ThemeManager::render() into a separate function.

I think this should be protected instead of private. We don't yet have resolution on #2727011: Policy: private vs protected, and the role of inheritance, and until we do, I think we should stick with Drupal core's existing convention of virtually never using private methods. I don't see any reason for this one to justify a deviation from the current convention.

#2954402-28: Refactor ThemeManager::render(), split into smaller immutable objects.

All of these private scopes are a BC break. This is supposed to be abstracting the public render method, a method that has and can be, subclassed by replacing the service.

By moving everything to the private scope, this is completely obscuring functionality that was previously accessible.

#2954402-32: Refactor ThemeManager::render(), split into smaller immutable objects.

There are actually very rare use cases to make properties/methods private in core. Make them protected please.

Perhaps there could be valid reasons for protected properties in some cases..
I personally doubt it, I have not introduced a protected property my own code in ages, and never missed it.
But if someone makes a solid argument, there should of course be a case-by-case discussion.
Also, a lot of times we will need to keep existing protected properties for BC (which, ironically, is one of the main reasons to avoid them).

The main goal of this issue is to remove what seems to be an implicit convention.

Note:
In #2954402-28: Refactor ThemeManager::render(), split into smaller immutable objects. @markcarver is actually making such a case-specific argument.
(unlike the other comments which appeal to implicit convention)
I happen to disagree with his argument, but totally approve that this should be openly discussed.

Sadly that's not true either. If you have a subclass and you redefine the variable, you suddenly end up competing about it.

This is true.
A private property of parent class can coexist with a property (private, protected or public) of a child class if they have the same name, containing distinct values. https://3v4l.org/X2ZRv

Once the parent property is made protected or public, while the child property is private or has lower access level than the parent property, there is a "Fatal error: Access level to C::$x must be protected (as in class B) or weaker". https://3v4l.org/L11de

If both are made protected, their values are no longer independent.
https://3v4l.org/aELVv

Of course similar clashes can occur for any new non-private method that you introduce in a parent class, if there happens to be method with the same name in an inheritor. Afaik we are not currently considering this a BC break, or do we?
Perhaps the name clash with private properties made protected is more likely, if people have a habit of redeclaring the property in inheritors.

My response to this would be: There is no reason to ever make a private property protected.
For an immutable property that is only set in the constructor and then never changed, the child class can simply make a copy in the constructor.
For a mutable property (which only occurs in mutable objects), the current value can either be exposed via a protected getter, or the child class can somehow intercept the method that leads to the modification.

Overall the necessity to access (mutable) parent properties should be the exception rather than the norm.
Of course this depends on the design of those classes.

Another huge issue with private properties is serialization, basically dependency serialization trait can't restore private properties so a lot of services & the trait will need rework

I am not very fond of this object serialization anyway, or of traits.
But obviously since this already happens we do need to support it.
I am confident it can be done, one way or another.

One idea I have for now is to have a new trait that would be embedded in each inheritance level of the class.

class B {
  use AdvancedDependencySerializationTrait;
}

class C extends B {
  use AdvancedDependencySerializationTrait;
}

Not 100% sure yet how this would look like, but I think it could get us somewhere.

There would need to be a mechanism to prevent that non-private properties are initialized more than once.
This problem would only occur for classes that have a mix of protected and private properties.
Otherwise the AdvancedDependencySerializationTrait could be used for class hierarchies with only private properties, while the regular DependencySerializationTrait could be used for class hierarchies with only protected properties.

Other ways to access private properties of a parent class are reflection and, I think to remember, some trick with closures. But I think those would add confusion and would harm performance.

@xjm
I mostly use private on properties, rarely on methods.

If a method is private, I very often find that it can be public static instead, and live in a utility class.
But a private method can be ok as an intermediate solution, while I am still undecided about the final shape of everything.

If a method is protected and meant to be overridden, then it is often possible to split out parts into a base class where this method is abstract protected.

One case where private methods are a good choice is for internal methods with fragile signatures, which have very specific expectations on the parameters and assume that sanity checks are already done by the calling code.

I finally found a good example in the Drupal codebase!

Drupal\Component\Plugin\Derivative\DeriverBase.

An alternative implementation of this class would look like this:

abstract class DeriverBase implements DeriverInterface {

  /**
   * Base plugin definition.
   *
   * Subclasses don't need access to this.
   *
   * @var array|null
   */
  private $basePluginDefinition;

  /**
   * Derived plugin definitions.
   *
   * Subclasses don't need access to this.
   *
   * @var array[]|null
   */
  private $definitions;

  /**
   * {@inheritdoc}
   */
  final public function getDerivativeDefinition($derivative_id, $base_plugin_definition): ?array {
    $definitions = $this->getDerivativeDefinitions($base_plugin_definition);
    return $definitions[$derivative_id] ?? NULL;
  }

  /**
   * {@inheritdoc}
   */
  final public function getDerivativeDefinitions($base_plugin_definition): array {

    if ($this->definitions === NULL
      // In the unexpected case where a deriver is called with a different base
      // plugin definition than in a previous call, all derivatives need to be
      // reset.
      || $this->basePluginDefinition !== $base_plugin_definition
    ) {
      $this->definitions = $this->buildDerivativeDefinitions($base_plugin_definition);
      $this->basePluginDefinition = $base_plugin_definition;
    }

    return $this->definitions;
  }

  /**
   * Builds derivative definitions for a given base plugin definition.
   *
   * @param array $base_plugin_definition
   *   The definition array of the base plugin.
   *
   * @return array
   *   An array of full derivative definitions, keyed by derivative id.
   */
  abstract protected function buildDerivativeDefinitions(array $base_plugin_definition): array;

}

With a base class like this, subclasses don't ever need to touch the properties of the base class.