Refactor ThemeManager::render(), split into smaller immutable objects.

First attempt.

The format-patch contains multiple commits.

Failing tests are FileUpdateTest, FieldUpdateTest, ImageUpdateTest, possibly more (still running).
But overall I see a lot of tests passing, which is reassuring.

I suspect that I do not properly reset the ThemeRender object on ThemeManager::reset() and other events where it needs a reset.

Ugly trick with Registry->getRuntime().

While this may seem to turn everything into OO, it feels like its really just glorified wrappers for an outdated procedural hook system.

This is a refactoring that attempts to keep existing functionality intact (once all tests pass). So yes, this means most of the old logic is still there, just in different places. I also think the term "hook" or "theme hook" won't and shouldn't go away, as long as theme hooks work the way they do. Which is for the rest of D8 life cycle, at least.

What you call "glorified wrappers" is, Imo, the the kind of OO that we should have a lot more of in core, as opposed to the verbose and crowded classes and interfaces that we currently have. The granularity of decorator layers can be discussed. E.g. we could collapse all the layers of ThemeHookRenderer into one, if it feels too much.
In fact if we were working with PRs instead of patches, I could show a version of this where the ThemeHookRenderer* classes don't exist.

The goal is to have (mostly) immutable self-contained pieces of behavior, with only few public methods, and a guarantee of completeness as soon as they are created.

Even the naming is using terminology that we should ultimately abandon.

The naming definitely needs work.
In my own projects I use a Vendor\Category\What_How naming scheme that works really well, but that is not compatible with Drupal or PSR-something naming standards. I think the naming scheme in Drupal does not scale well to different implementations of the same interface.

With Drupal's naming scheme it would be Vendor\Category\HowWhat or Vendor\Category\What\How. So it would perhaps be Drupal\Core\Theme\ThemeHookRenderer\PreprocessDecorator or Drupal\Core\Theme\PreprocessDecoratorThemeHookRenderer. I don't have a strong opinion here.

As said above, the term "Hook" in the name is really just honesty, because we are not changing this.

I'm really not a fan of this... I'd much rather we go the route of #2869859: Refactor ThemeHook into a proper plugin manager and use annotated @Theme and @Template plugins.

As I see it, the plugin system gives us this:
- Some kind of registry or list to map machine names (plugin ids) to classes, and then objects.
- Human labels on those machine names, which allow to build a form with a select or radios element to choose one of the plugins.
- Every implementation is a distinct class. (except for plugin derivatives).

The "machine name to object" is something we could also use for the theme system. The rest, not so much.
We never need to show theme hooks to a user.
We don't need a distinct class per theme hook. (so we would use a lot of derivatives).

This doesn't mean the idea is completely wrong.
Plugin objects are pieces of behavior with a common interface, so this could go into a similar direction as I propose here.
Unfortunately, the behavior of each theme hook is not completely self-contained, because of the suggestions and fallback mechanics, and the preprocess inheritance / cascade. This is why my patch only puts part of the logic into the ThemeHookRenderer* classes, whereas the initial suggestions still rely on the theme registry.

Do you think the plugins would have a similar role as the ThemeHookRenderer* objects?
Or what is the behavior that you would give them? What would be the plugin interface?
(Maybe we can continue in #2869859: [PP-1] Refactor theme hooks/registry into plugin managers)

I just mean that I think it's futile to attempt to "port" procedural based "hooks" into OO code.

I think there is nothing wrong with trying this. Or rather, it is the best and only thing we can do if we want to be backwards compatible and still have nice-looking testable OO architecture.
Currently we have a partly or mostly procedural API with a messy OO layer on top. We can, and absolutely should, improve the quality of the OO layer, without changing the underlying procedural API.

I think even D7 could look much better with some behavior-neutral refactoring, and without changing the API.
I think we too often replace APIs instead of refactoring the implementation and giving them a chance.

Instead, I think we should be more focused on creating a new (better) API/system that "converts/merges" the legacy hooks/arrays into a plugin equivalent.

This means changing the API, per definition, which will cause BC breaks.
Even if we do this, it can still be a good idea to first refactor the existing OO layer in a way that does not change behavior or the API.
This way, once we do want to change the API, we can understand better what we are changing.

And how far can we change the API within D8, anyway? How much BC is acceptable?
E.g. if your preprocess hooks suddenly don't work anymore (perhaps after being "deprecated" for a while), is this something that can be done within D8, or would this have to wait for D9?
If hook_theme() is no longer allowed to return arrays? Or if theme functions don't work anymore?

That "crowding" is what OO is all about. Creating type based objects that have set interfaces that indicated expected behavior and can be unit tested. An arbitrary hook based array system cannot (at least not very easily anyway).

By crowded I mean classes like ThemeManager / ThemeManagerInterface, which do various things with the only similarity that they are somewhat theme-related, and can change state at any time. Some of the functionality is for one specific theme (the "active theme"), which is lazy-initialized and can be reset and changed. Other parts of functionality are independent of the active theme. All of these are indicators that things should be split up.
The verbose interface also makes it harder to apply compositional patterns to split out partial functionality.

(for the rest, I left a comment in #2869859-10: [PP-1] Refactor theme hooks/registry into plugin managers)

Hi,
I am going to respond to some of your points.
But to move on, I need a better idea of your vision for the theme registry with plugins.

It doesn't justify making a grandiose and generalizing claim about all OO like was said prior.

The previous claim did reflect a general feeling I have, when comparing D8 code to my own projects. I could elaborate, but don't want to spread too much negativity in the room.

Maybe some background: The average class and interface in my own projects (those that I am happy with, e.g. renderkit) has one or two methods and very few private methods, is immutable, has a few private properties. Rarely any setters and getters (most of them are what I would call "behavior objects"). Whenever something is bigger than that, I try to split it up. This strategy has served me well in the last years. It means every class, interface and method fits into one chunk of mental short-term memory. And they make compositional patterns much easier.
The cost is having more interfaces and classes, some of which only have an internal purpose. Sometimes I went too far and had to collapse some of those layers again. But most of the time, some extra interfaces did not cause problems.
I rarely find this kind of architecture in D8. It is getting better, but even newly proposed classes and interfaces still look too big for my taste, on average.

No one's "changing" the existing procedural APIs. Nor can we. We should be focusing on creating a new system that the existing procedural APIs can be grandfathered in, seamlessly.

(I think this summarizes a number of your other points)
So we will extend the API, without breaking it. And we will end up with some kind of hybrid system, which still supports the old ways of doing things.
I still think that to build such a system, it is a good idea to first refactor the old logic, before we extend the API. I don't see a conflict here, necessarily, just a difference in priorities and order of doing things.

I, for one, can never remember which one does altering and which one checks if a theme is available.

I have a local branch with an AltererInterface, which has implementations for themes and modules..
I am sure this would be controversial, so I did not open an issue. This one is already enough.

Imo we should have interfaces for behaviors, not for stuff that somehow belongs together.
I want to alter something? Use an AltererInterface object, which mostly just needs one ->alter() method.
I want to get html from a render array? Use a RendererInterface object, which mostly just needs one ->render() method. Well, the core RenderInterface has 8 methods, not 1. I am sure this has good reasons, but I somehow suspect that I would have split this up in some way.

The above code is literally creating new standalone objects, arbitrarily named with antiquated terminology

I agree that those classes can be renamed, but I don't see the problem in having standalone objects.
I don't see how these objects, the way they are in the patch, would benefit from from a plugin system. They are just behaviors that were split out of the bigger thing. Which is good enough for me.

and likely non-performant due to the separation of all these elements that aren't easily cachable.

I would expect the new code to be faster than the old ThemeManager::render(). A lot of stuff in ThemeManager::render() is executed once for each call, when in fact it only needs to happen once per theme hook, the first time it is called. So if the same theme hook is executed more than once, these extra steps don't need to run again.
These are inclusion of files, the function_exists() call, the composition of the template file path, and all the array lookups with isset(). Most of the ThemeHookRenderer* objects don't keep a copy of the $info array, instead they have the explicit values as private properties. The only reason to keep a copy of $info around is to pass it as an argument to preprocess functions.
There is some overhead due to initial object construction and then object property lookups and method calls. So it could still be a close race.

separation of all these elements that aren't easily cachable.

If you understand correctly, you are not claiming that it does slow down the current system, but that it prevents future optimization by caching, right?
We need to distinguish runtime cache (which would be static variables in D7) vs persistent caching (e.g. in the database).
Runtime cache for the ThemeRender object and for ThemeHookRenderer* objects is already provided by this patch.
A persistent cache exists for theme registry entries (definition arrays).
A persistent cache for the ThemeRender object or ThemeHookRenderer objects would involve object serialization, which I am not a big fan of. I also don't know how much benefit it would bring.

Do you have something else in mind?

antiquated terminology

Getting back to this.
Which part of the name is antiquated in your opinion? Maybe the term "theme hook"?

Currently we use the term "theme hook" in a number of situations:

• The value in ['#theme' => $hook] in render arrays.
• The first parameter to ThemeManager::render().
  Maybe we are going to move this method elsewhere, but the signature will remain.
• Machine names of theme registry entries.
• Part of the name of a template file (before the extension), which uses the name to identify itself as a template for this theme hook.
• The suffix of a theme function.
• The suffix of a preprocess function.

The latter two are closest to Drupal's usual meaning of "hook", as something procedural with magic function name matching. Perhaps this is the original reason it was called "theme hook".
Perhaps we want to change or deprecate some of these procedural mechanisms.

But we are quite used to the term "theme hook" for the other situations above, which are not going away any time soon.
Or are you proposing that?

I hope this time all 3 patches pass.

"CS only" contains only code style fixes, a preparation for the following work.

The "part I" introduces only one new class (and one interface), and leaves most of the render logic as it was, just in a new place.
@markcarver Perhaps this one is more acceptable for you?

The "part II" does the controversial micro-splits.

I want to test all 3.

I expect that steps I and II both improve performance, because they both make it so that some logic that used to be executed once for each ThemeManager::render() call is now executed only once per average request, or once at the first time a specific hook is used.

class ThemeRender implements ThemeRenderInterface
I think this is a poor name for a class. But I am keeping it like this for now, for my local git history.

The patches are green, which is awesome!

However, the changes themselves are still controversial - even to myself.

There are different "food chain" (*) problems in the current ThemeManager::render():

• It always obtains a fresh "runtime registry" for the "active theme".
  Anything we want to cache between theme calls needs to be discarded, if
  - the active theme changes.
  - the theme registry is being cleared / reset.
• There are different levels of theme suggestion resolution, and different intermediate results of this have to be kept for later, or written into the $variables array.
  The $info array passed to hook_theme_suggestions_*() is a different info array than that used to do the actual rendering.

The splitting in part II exposes the problems, but also makes them part of the architecture.
At the same time, the suggestion resolution is still one big chunk of logic, and not nicely split into smaller methods.

The split into "ThemeHookRenderer*" objects would be nice if we really had one such object per hook name. But this model is limited thanks to the suggestion logic.

I would say the first part is less controversial.
But it shows some problems of the ThemeRegistry and Theme\Registry components.
Maybe we should fix this first, e.g. in #2957451: Some parts of Theme\Registry are written as if they support multiple themes

Proposed next steps

The issue #2957451: Some parts of Theme\Registry are written as if they support multiple themes has a weird name, but it might lead somewhere useful.
I think we should improve the Theme\Registry and ThemeRegistry ("runtime registry"), and then get back here.

I think the "part I" of this issue is a decent idea, and it is big enough of a change for one issue.

The "part II" contains some interesting ideas, but I feel it is not fully thought through. It could be a follow-up, but then I also want the suggestion resolution to be better organized.

@markcarver: I think the "theme hooks as plugins" from #2869859: [PP-1] Refactor theme hooks/registry into plugin managers should still be possible after part I, and after some improvements in Theme\Registry and ThemeRegistry. I don't see a conflict there.

@Pol (#14): Sorry I did not take the time to respond!

+++ b/core/lib/Drupal/Core/Theme/ThemeHookRendererFunction.php
@@ -0,0 +1,35 @@
+    $output = $function($variables);

Shouldn't we use call_user_func() instead?

$function() is faster (last time I read about it) and it is also what the current code does.
So we keep it.

I think we should be consistent with empty lines at the start of methods. Either we add empty lines everywhere or we don't, but mixing it is not very advised.

My personal rationale: If the method itself is split into "sections" by empty lines, then I also put an empty line before the first "section".
If the method has no sections or is very short, or the first "section" is not really a section but just an starting line (e.g. declaration of a static variable) then I don't.
I realize this is quite arbitrary, so you are possibly right.

Drupal coding standard: I don't find anything here, https://www.drupal.org/docs/develop/standards/coding-standards
egrep -R --after-context=1 "function .*{" | egrep "php\- *$" shows that the initial blank line is very common in methods in test classes.
egrep -R --after-context=1 "function .*{" | egrep -v "^vendor/" | egrep -v "^tests/" | egrep --before-context=1 "php\- *$" reveals more cases in non-test classes.
Here is a statistic:

~/projects/d8/www/core $ egrep -R --after-context=1 "function .*{" | egrep -v "^vendor/" | egrep --before-context=1 "php\- *$" | wc -l
1229
~/projects/d8/www/core $ egrep -R --after-context=1 "function .*{" | egrep -v "^vendor/" | egrep --before-context=1 "php\- *[^ }]+$" | wc -l
11285

This means only ~11% of non-empty methods / functions start with a blank line.
I don't know what we should make of this :)

I personally don't see a big problem in some arbitrariness. Some functions look better with a starting blank line, some look better without it :)

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -125,12 +135,28 @@ public function setActiveTheme(ActiveTheme $active_theme) {
+      return $this->themeRender = $this->createThemeRender($theme_registry_runtime);

I'd set $this->themeRender to NULL so we don't have duplicated code.

I think this no longer applies in the latest patch.
And in the old patch, it would have been ok, because this "duplicate code" prevented an unnecessary operation (setting to NULL first, only to set it to something else later).

I've skimmed over the patch(es) but will have to do a more in-depth review later this weekend/next week. I'll try to get to this soon.

This would be great!
I should mention (perhaps again) that I still plan to refactor the ThemeRegistry prior to this. I haven't gotten to it in the last weeks, but it is still on my todo list.
See #2957440: Refactor ThemeRegistry and Theme\Registry

This should not stop you from reviewing the patches in this issue, just don't see it as the final version.

For the decorator layers in ThemeHookRenderer, I might have a better solution.
A clean separation of these layers is not really possible if we want full BC. So instead, we might have a base class and then subclasses for templates and theme functions.
Or we omit this step altogether, and keep it in ThemeRender class.
Also, the ThemeHookSuggestionFinder* classes might be overkill, we could keep this as private methods within ThemeRender class.

The motivation to split out ThemeHookSuggestionFinder was that it removes two dependencies from ThemeRender class, the $moduleHandler and $themeManager. Splitting out dependencies is always nice.

try to separate this into smaller sub-issues so it's easier to digest, review and ultimately commit. That being said, I know how intertwined it all is, so maybe that isn't likely to happen.

I have a branch with distinct commits locally. If we were developing on github, you could review it piecemeal.

I am going to respond to the other points later.

(sorry, this is long)

1.)

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.

Well..
In the past / currently, all the logic is in a monolithic method ThemeManager::render(), which is part of a larger class ThemeManager.
To have a different implementation of ThemeManager::render(), in the current version of D8, one would have to write a subclass of ThemeManager.

E.g. a very silly version of that would be this: https://pastebin.com/KLKASJvN
(Some assert() statements might not even pass, because some properties can be NULL at times)

Such a subclass of ThemeManager would still work after the patch, because none of the protected properties of ThemeManager is removed.

But maybe your real concern is this:
One can extend ThemeManager and access the protected properties.
But when extending the new class ThemeRender, one has no access to the private properties of the base class.
This is technically NOT a BC break, it does not break any hypothetically existing 3rd party code.
But perhaps you still miss the possibility to do access those properties.

The answer is: You don't need to extend ThemeRender.
You simply write a new class that implements the same interface.
This is the purpose of having smaller, "atomic" classes.

Let's face it: You would replace the complete render() method anyway.
All the properties you need to declare again in the new class. At least those that you need. But I see this as an advantage, because you can make them private, you don't need to look into a separate file to see the property declarations, and you can discard properties you don't need in this very class.

a method that has and can be, subclassed by replacing the service.

It is a weird statement, because you subclass a class and override a method, and you can replace a service with a subclass or with a new implementation. You do not subclass a method by replacing a service.
But I think I understand what you mean.

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

"encapsulate" is the word.
If your 3rd party component (which should not be a subclass) needs e.g. a ThemeEngine, it can have it injected in its own constructor. It does not need to access the properties of the ThemeRender class.
Less access is good. Components should not grab their dependencies from somewhere, they should passively receive them.

Now in the original ThemeManager, state was an issue. Some of the properties are lazily initialized, or they can change during the lifetime of the object. I will get back to that later.

2.)

This should really be using events, not some abstract closure that has to be registered each time.

Yeah, I knew this was not going to last :)
I wanted to have something that passes the tests.
I am not sure if symfony events (this is what you mean, right) are the right choice here, or something else. I will think about it.
Maybe I will get more ideas when I approach #2957440: Refactor ThemeRegistry and Theme\Registry.

What I like about the proposed solution is that the communication happens purely between these two objects, and does not go through some global event broadcasting system.

"events" or observer/listener/subscriber is not just a symfony thing, it is a pattern that one can apply locally.

3.)

+++ b/core/lib/Drupal/Core/Theme/ThemeHookRendererPreprocessDecorator.php
@@ -0,0 +1,109 @@
+class ThemeHookRendererPreprocessDecorator implements ThemeHookRendererInterface {

+++ b/core/lib/Drupal/Core/Theme/ThemeHookRendererVariablesDecorator.php
@@ -0,0 +1,47 @@
+class ThemeHookRendererVariablesDecorator implements ThemeHookRendererInterface {

I really don't understand the difference between these two. Also, I think a decorator pattern is completely unnecessary considering this API hasn't changed in nearly a decade (nor is it likely to change in the future). It'd be different if contrib were allowed to supply additional decorators, but I don't think that should happen given other issues that will likely this a moot point. This is overkill.

Yes, this is silly.

The original idea was somewhat reasonable (though still overkill, perhaps): One component (a decorator) would execute the preprocess callbacks, another would include the template or execute the theme function.

There would be one (compositional) instance of ThemeHookRenderer per theme hook (that is, string key identifying a theme registry entry). All the information from the registry entry would already be incorporated into the ThemeHookRenderer instance, so the only parameter to ThemeHookRendererInterface::render() would be $variables.

Unfortunately, in the legacy implementation, some information travels around in non-obvious ways, and are added to $variables at non-obvious moments in the process: The $variables['theme_hook_suggestion'] is set after all preprocessors have run, even though this value is generated earlier. Similar for $variables['theme_hook_suggestions'].

I think this explanation is still incomplete, it is a mess. But we need to maintain the order in which things are happening to guarantee full BC.

I will think about a way to clean this up, or perhaps merge it back into ThemeRender.

It'd be different if contrib were allowed to supply additional decorators, but I don't think that should happen given other issues that will likely this a moot point.

One idea here could be that different theme engines would provide different implementations.
There are already distinct implementations for template and function, and perhaps there could be a case for more?

Not sure. Perhaps for now it is better to avoid these extra classes, and do the split when we actually have a use case.

4.)

+++ b/core/lib/Drupal/Core/Theme/ThemeHookSuggestionFinder.php
@@ -0,0 +1,78 @@
+class ThemeHookSuggestionFinder implements ThemeHookSuggestionFinderInterface {

I kind of agree with you that separating this out into its own class seems a little like overkill. I think it should just live in ThemeHookRenderer.

Yeah, I think I will put this back into ThemeRender.

Just saying: The motivation to split things into smaller classes is not to make a flexible API for use by 3rd parties, but simply to have more manageable smaller components, which can reduce mental load (depending how its done) and allow more small-level unit tests.
E.g. here the ThemeHookSuggestionFinder could be easily replaced with a mock implementation which would return bogus.

It also reduces the temptation of inheritance.

5.)

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -125,12 +133,20 @@ public function setActiveTheme(ActiveTheme $active_theme) {
+    return NULL !== $this->themeRender
+      ? $this->themeRender
+      : $this->themeRender = $this->createThemeRender();

+++ b/core/lib/Drupal/Core/Theme/ThemeRender.php
@@ -0,0 +1,254 @@
+    if (FALSE === $theme_hook_renderer = $this->getThemeHookRenderer($hook, $chosen_hook)) {

Pretty sure this is a CS violation, I have never seen this kind of syntax structure in core/contrib. We don't code this way.

The first one could be simplified with ternary operator, if we wait for March 2019 :)
I still think this a very readable way to do things, so we should look if there is an actual code style policy saying we can't.

The second one:
egrep -R "if \((FALSE|NULL|false|null) (\!|=)== .* = "
One occurence in lib/Drupal/Core/Routing/UrlGenerator.php. git blame goes way back. I think it was copied from somewhere outside.
On the other hand, vendor/ has plenty of those. Makes me jealous not being allowed to do this in core.

I think the common way in core is long and nasty one-liners.

6.)

+    return new ThemeRender(
+      $this->root,
+      $this->getActiveTheme()->getEngine(),
+      $this->themeRegistry->getRuntime(),
+      new ThemeHookSuggestionFinder(
+        $this->moduleHandler,
+        $this));

There's no need to recreate this each time.

Nice catch!
This is "each time" we create a ThemeRender object, but not each time we render something, so it is not really relevant for performance.

Creating this only once would still be a good idea, perhaps in the constructor.
But since we want to ditch this class anyway, this point becomes obsolete.

7.)
I am pretty sure it is not a BC break. Adding a parent interface is only a BC break if this introduces new methods or changes the signature of existing methods.
This said, it may still be a bad idea.

8.)

+class ThemeRender implements ThemeRenderInterface {
This should probably be ThemeHookRenderer so it follows the same prefix of all the other classes.

It should not, because it is a different interface, with a different method signature and a different role in the application.
The distinct name is on purpose.

This said, "ThemeRender" is indeed a poorly chosen name.

That being said, given the number of classes this introduces, perhaps everything should just be namespaced.

Let's see how many classes are left in the next version.
Personally I like one namespace/subfolder per interface + implementations. But I don't think this is common for core.

Also, this should probably be a service so it's built with the container.

Currently this is not a service, which allows to discard the instance when the theme changes or the theme registry is rebuilt.
This allows the ThemeRender class to be stateless.

What we can do is add a separate class which manages those instances, and which can be registered as a service.

9.)
Yes, the remaining ThemeRender::render() is still relatively long.
Multiple variables travel through the entire function, so I have not found a good split yet.

But I think it would be fully acceptable to do this in a follow-up.

I think it already helps to keep all the other stuff from ThemeManager that is not directly related to ::render() out of this class.

10.)

So, this appears to be some sort of static cache, which I'm all for. If anything, I'd say that we should truly make the property static and include the theme name as part of the cache ID so it's cached for the lifetime of the request.

Making things like this static introduces global state, and makes it harder to unit-test. Not good.

The average request has only one theme, and only one instance of ThemeRender.
If we are truly concerned about performance in case of theme-switching, we could keep an array of ThemeRender objects around, one per theme, instead of throwing them away when the theme changes.
But I think theme-switching and registry rebuilds are not cases we need to optimize for.

We should probably also look into storing this in a backend cache as once it's been processed,

This runtime cache buffers behavior objects, one per theme hook.
These behavior objects contain dependencies (e.g. the theme engine), so serializing them is problematic.

We already have a persistent cache for theme registry entries. These are arrays, so it is natural for them to be written to storage.

I think it is a reasonable pattern to store definition/configuration arrays, and then create behavior objects from those arrays in a request.

Well, if we actually keep those classes that are instantiated per theme hook. Perhaps we ditch that part.

----------

This isn’t github

Sad!

Oh and FTR, ThemeManager is replaced/subclassed in contrib: https://cgit.drupalcode.org/plus/tree/src/Core/Theme/ThemeManager.php?h=...

It is great to have a data point like this!
I need a bit of time before I can say something meaningful about it.

For now I bet that
- the "plus" module as-is will still work even after this core patch.
- there is an upgrade path for the "plus" module, despite the private properties in ThemeRender.

But I have to study it first.

The only difference is that it doesn't have the name Hook in it, which is what all this is about: theme hooks. Namespacing it to something like Drupal/Core/Theme/Hooks and renaming the class to just Renderer makes more sense, logically.

And the reason for that is that one has the render($hook, $variables) as a signature, the other just render($variables).
The names are revserse, it is ThemeHookRenderer->render($variables) and ThemeRenderer->render($hook, $variables), the idea being that ThemeHookRenderer already contains the hook data so you no longer need to pass it as a parameter.

If we want to align the names, we should also align the signatures. Otherwise it is misleading.

All the code that lives in here has to be created each request, even though it would never actually change until another cache clear/theme registry rebuild.

I'm just saying that it makes little sense to just simply throw away all this work each request.

We should try both and measure which one is faster: Creating them from theme registry arrays, or unserializing the cached objects with DependencySerializationTrait. You may very well be right about this.

Aside of the other points (which I'll get to eventually), I notice one small but potentially breaking problem in my change.

In current version of ThemeManager::render(), we check in each call whether $this->moduleHandler->isLoaded() && !defined('MAINTENANCE_MODE').
In the modified version, I only check this when a new ThemeRender object is built.

This matters if something calls ModuleHandler->reload(), and during the reload something calls ThemeManager->render(), e.g. an error handler.

The maintenance mode is not a problem by itself, because it cannot * be unset once set, and from then forward the check always passes.

However depending how we deal with the ModuleHandler->reload(), we also need to take care of the maintenance mode.

I see three options, and only the first two are fully BC:

1. Check $this->moduleHandler->isLoaded() && !defined('MAINTENANCE_MODE') during each call to ->render().
2. Subscribe to some kind of module handler reload events and maintenance theme enabling events.
   (which do not even exist currently)
   
3. We don't care about this case, and assume that modules are always there once initially loaded.

I think the second option is out of scope for this patch, as we would first have to introduce these events (either as symfony events or as something one-off). So we have to go with the first or third option.

I wonder what exactly would go wrong if we don't throw this exception while modules are reloaded.

Not true. Yes, it can implement its own ThemeManager::render method (and would still work), but it wouldn't garner any of the benefits of being able to "match core" or gain any of the performance improvements. Thus, it would always be "locked" into implementing the old previously large "lumped-sum" method that was ThemeManager::render. Thus, increasing the technical debt of the contrib module to always having to manually backport any relevant changes. The reason this is a BC break is that it breaks the paradigm of "subclassable" as was done prior and instead forces the code to remain completely custom.

I already have some changes in mind based on the use case you pointed out.

However, the idea that you require protected properties to extend the functionality is wrong:

1. ThemeRender could be one big class with multiple small protected methods, but only private properties.
   Then your module could extend this class and override some of those methods, optionally calling the parent method.
   You would not need most of the properties. But if you need them, you can intercept them in the constructor and store your own copy in your own private variables. The benefit is that you don't have to look at multiple files to find read and write usage of a property. I do this all the time (if I use inheritance), and it really reduces pain.
   If you are worried about extra memory of copied property values: Mostly these are object pointers, which are cheap on memory.
   If you are worried about state change in the parent class: Make it immutable, then this problem goes away.
2. ThemeRender could be split in a number of smaller classes with interfaces and composition, each with their own private properties.
   You could replace some of those components with your own implementations, either as decorators, as subclasses, or as standalone.
   You never need access to any properties of core classes. Even if you extend them, you can intercept them in the constructor.

I am not sure if the current patch fully matches what I described above, but in principle this is how it can be done without protected properties.

Yes I am aware that protected properties are like an implicit standard in core, but this is not a state we should happily live with. I want at least a proof-of-concept to show that it can be done differently.