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

I am in favor of piecemeal refactoring and picking low-hanging fruits first.
I actually did start to do more refactoring locally, but this can easily be a follow-up.

The suggestion picking is self-contained and not interdependent on the rest of the function. Or rather, the dependency can be expressed as parameters and return value.

With this code in the main function, a reader does not immediately see that those variables are only used in this section of code, and not further down. Splitting it out makes this obvious, and hence reduces (gradually) the mental burden of reading the beast function. It also shows what the suggestions depend on.

I actually did start to do more refactoring locally, but this can easily be a follow-up.

If you are curious:
For a follow-up, I plan to create an interface ThemeRenderInterface (name to be discussed) as parent of ThemeManagerInterface, which only has the ::render() function.
One implementation of ThemeRender with the logic from ThemeManager::render(), but where all the lazy-initialized object properties are already set in the constructor. So it would be immutable.

The first call to ThemeManager::render() would create this new immutable object, and then call the ::render() function of this object.
ThemeManager itself would still have all the mutable and resettable lazy initialization from global state mess, but the main logic would live elsewhere, in an immutable and thus predictable and more easily testable class (which is not by itself registered as a service!)

Once we are there, we could split parts of the ::render() function into decorator layers.

We could also split out the suggestion building into a separate object, possibly with an interface.
This would reduce the dependency of the ThemeRender object, because the module handler and theme manager are only needed for the suggestion building.
I personally don't mind to create such one-off interfaces. But in general, people seem not too happy about it.

I also started working on an AltererInterface, with implementations for theme-based altering and module-based altering. Again, these objects can be lazily created in ModuleHandler and ThemeManager, and can be mostly immutable (except for the cache/buffer of collected alter functions).

I then noticed that splitting out AltererInterface is only worthwhile if we also split out getImplementations() and invokeAll() from module handler into a separate interface. Then the suggestion-building would no longer depend on module handler and theme manager, but on AltererInterface and ModuleHookInterface (if this is what we would call it).

---------

All of this I expect to be highly controversial, whereas this patch is not.
Also, such bigger changes need to be proposed as a PR, where commits are separated, not as a huge patch. I tried to do this in the past, posted links to github with every patch, and posted the patches with format-patch for multiple commits. But most people at the time wanted to see one big diff instead.

Therefore I prefer to do things piecemeal, and hope that smaller issues have a chance to be committed faster.

Are there followups created we can link to this issue?

I like to "test the waters" before I put too much effort :)
I could push a POC branch to github if you are interested.

For a follow-up, I plan to create an interface ThemeRenderInterface (name to be discussed) as parent of ThemeManagerInterface, which only has the ::render() function.
One implementation of ThemeRender with the logic from ThemeManager::render(), but where all the lazy-initialized object properties are already set in the constructor. So it would be immutable.

The first call to ThemeManager::render() would create this new immutable object, and then call the ::render() function of this object.

If the object is created locally by ThemeManager, and not passed around elsewhere, we don't need an interface for it.
We can still add an interface later, but we can start with just a class. Maybe mark it as @internal so we can change it later?

But either way, this should be a follow-up.

@tim.plunkett Thanks for the link.

When I first saw your comment, I thought that the new class would do the actual rendering.
Somewhat like this: $registry->getThemeHook('page')->render($variables).
I thought about this before, but then decided it would not be such a good idea, because of all the suggestion and fallback logic. Such an object would not be self-contained, but still need the full registry to resolve suggestions.

Your proposal solves a different problem. I am not sure yet if I like it, it introduces a big (in lines of code) class.
Also, serializing objects is more costly and fragile than serializing / unserializing arrays.
More feedback over there.

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

Hi,
sorry, I was going to reply earlier, but somehow got held up.

@effulgentsia (#14):

I think this should be protected instead of private. We don't yet have resolution on #2727011: [policy, no patch] 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.

I could upload a different version where the method is protected instead of private, but I am skeptical about this.

Having a private method would allow very easy follow-up refactoring.
Making it protected would let us wonder if anything else is already using the method. So this would no longer be a nice preparation for a follow-up, but rather make such a step more difficult.

I suppose we could mark the thing as @internal, but it just feels silly to do this, if it would be so much easier (technically, not politically) to just make it private.

+  private function buildThemeHookSuggestions($hook, array $info, array $variables) {

It is unnecessary to pass the entire $info array to buildThemeHookSuggestions().
We only need to pass the value of $info['base hook'].
At first I thought we can use the variable $base_theme_hook for this, but this won't cover all cases correctly.
Instead, we would need a separate variable like e.g. $info_base_hook.

This is one reason why I prefer to keep this private: The signature of the method is tied to implementation details.