[PP-1] Refactor theme hooks/registry into plugin managers

While the above was a blocker for layouts work, this is just an awesome follow-up.
Excited to work on this, but untagging from the other initiative.

How would the interface for these plugins look like?
What is the behavior we expect from one theme hook plugin?

I really recommend following my work in https://www.drupal.org/project/plus though as this is what I'll be basing #2554199: [PP-3] Bootstrap 4 on.

Maybe I will do this some day. But I think the main ideas belong in the issue summary here :)

can be specified as a @Template annotation.

What will you annotate? Where do you put the annotation?
Into a template file? Or on a class? Would you have to create a class for every theme hook? Which interface would this class implement?

I'll be present at the next EU DC (if any!) and I'm also willing to be part of the conversation!

I think I will be on dev days Lisboa and on Drupal Europe this year.

I looked at https://cgit.drupalcode.org/plus/tree/src/Core/Theme/ThemeManager.php?h=....
Most of this is the same as the parent ::render() function. The same refactoring as proposed in #2954402: Refactor ThemeManager::render(), split into smaller immutable objects. could be applied here.
So it does not even conflict with what you are doing.
Probably what I see there is just a first step, and later it might all be different. I can only comment on what I see.

I still would like to see an example where you would put those annotations. Just to get an idea.
If I get a better idea how you envision the "theme hook as plugin" system, I might rethink some of my proposed refactoring in #2954402: Refactor ThemeManager::render(), split into smaller immutable objects..

I know you're passionate about this topic just as much as I am.

Perhaps not. I think my motivation works differently.
I just look at pieces of code and am unhappy what I see (most of the time), so I think about how it could be refactored without changing the behavior.
This doesn't mean I care so much about the respective subsystem, just that I looked at the code long enough that it triggered a reaction.

In my own projects, I usually do this kind of refactoring before I introduce any API changes, just to get a better idea what I am dealing with.

Concept looks interesting but overhead on class discovery/loading could kill performance that was polished in functional implementation

Re #17:

once I get a working POC

Before a POC, I would very much like to see some simple usage examples.
E.g. how would the following change?
- A module wants to register a theme hook (or whatever you want to call it).
- A module or theme wants to preprocess theme variables.
- A theme wants to override a template.

I suppose some of this would not change at all, but some of it would change with your annotations and plugins?
Of course, if I understand correctly, existing ("legacy") code would still work, but there would be new options how to do some of the things above?

-----

OP:

@Theme - Used to define a theme and handle the preprocessing/rendering.

Do you mean what is now an extension with type = "theme" should become a class/plugin instead?

@Template - Actual template definitions (formerly known as "theme hooks" provided by hook_theme())

Where do you put the @Template annotation?

And what do we gain by saying "template definition" instead of "theme hook"?
Imo, they are not the same.
A "theme hook" is a string key, which in the end will be mapped to a template, in some cases this mapping depends on the variables sent along with the hook name. But there is no fixed 1:1 mapping between theme hook names and template files.
So, I would strongly suggest to continue using separate names for these separate concepts.

"Theme hook" might remind us of something procedural, but it doesn't have to.
It is just a string key that will be mapped to a theme registry entry and a template.

An example in contrib (still a WIP):
https://cgit.drupalcode.org/plus/tree/src/ThemePluginManager.php?h=8.x-4.x

I looked at https://cgit.drupalcode.org/plus/tree/src/Plugin/Theme/ThemeInterface.ph..., which seems to be the interface for this "plugin type".
It is far too big for one class or interface. Imo this makes it a dead end.

----

Back to #17

Performance for procedural-based functions/hooks is not even in the same league as properly architected OO code.

These are two entirely different concepts/implementations and what is "performant" for one does not translate 100% of the time to the other.

Historically, procedural/hook-based functions are less performant (cpu + memory).

Any general statement about whether procedural code or OOP code is "faster" is usually bogus.
There are some statements we can make, see below.
The question is if and how they apply to your proposed changes. This is hard to say at this stage, where I don't see any code yet.

• A hardcoded procedural call like foo() is (a bit) faster than an object method call like $obj->foo().
• On the other hand, having to figure out dynamically which procedural function to call before we call it, makes the procedural call slower again.
• A dynamic function call like $foo() is slower than a static function call like foo(). I would suspect it is also slower than an object method call like $obj->foo(), but faster than a dynamic object method call like $obj->{$foo}().
• String operations, e.g. the $module . '_' . $hook concat, or worse, any strpos() and substr(), slow down procedural hook systems.
• Discovery of hook implementations or preprocess functions by passing every possible combination into function_exists() is slow. Slower than plugin discovery? Depends.
  However, this can be greatly improved by using tricks with get_defined_functions().
• Parsing a doc comment with annotations is significantly slower than some basic string operations on a function name.
• A dynamic class instantiation like new $class() is likely slower than a static class instantiation like new C().
• I suspect that unserializing objects is slower than unserializing arrays, if they are comparable in size.
  But I suspect a much bigger impact is from unnecessary junk that we serialize along with data that we actually care about.

I made these statements based on some common-sense considerations. I could be wrong.
But the essence is: Whether OOP or procedural code is faster, depends largely on what we do, and how we do it.

In our case, the biggest question is when do we perform these operations? On cache rebuild, or in every request?
If the answer is "on every request", then do we perform it once every time a theme hook is executed, or just the first time this theme hook is executed?

All of this is unanswerable at this stage.

In regards to runtime (rendering), the real "culprit" to performance here is memory. The current system actually tends to be a memory hog because every procedural-based function is put in the theme's *.theme file; which is loaded every single time the system renders something.

I imagine this file can get quite long, if you have a lot of preprocess functions.
But can it really be that long that you have to worry about memory?
I haven't seen relevant THEMENAME.theme files for D8 in client projects, but I have seen a number of template.php files in D7 client projects. They can be long, but usually it is nothing compared to the accumulated core code that is included in every request.
All of this will be in opcache.

By moving to an OO plugin based system, it only loads code that is actually needed for rendering whatever was actually requested. Not the entire theme itself.

Let's compare.

Procedural preprocess, as it is now:

• the entire THEMENAME.theme file is included at once.
• every theme registry entry has a list of function names to be executed for preprocess.
• for every such function, we call function_exists(), then execute it (dynamic call like $foo()) (*).
• The function_exists() is unnecessarily (*) repeated, if we call the same theme hook again in the same request.
• Theme preprocess functions need to get all their dependencies from global state, e.g. via \Drupal::service(..).
• Discovery currently has an inefficient implementation, last time I checked. (*)

(*)
The redundant function_exists() calls could be avoided, as proposed e.g. in #2954402: Refactor ThemeManager::render(), split into smaller immutable objects..
The dynamic calls could be avoided with code generation.. but it is not clear if this is a good idea.
Discovery of preprocess functions can be optimized with get_defined_functions(), like it is done in registryonsteroids. This would make it faster than any plugin discovery (I say without any proof, because we have no POC to compare with).

OOP preprocess, as I imagine you want to have it (please correct me if I'm wrong):

• Every preprocess is an object method.
  The class needs to be autoloaded.
  This reduces the code to be included on requests with only few preprocess functions, but increases autoload overhead on requests where many preprocess functions are executed.
• Every theme registry has either a list of serialized preprocess objects, or a list of preprocess class names, or a list of preprocessor plugin ids.
  
• For every such entry, we either need to unserialize the object (which may be part of the big unserialize of the entire registry entry), or we need to check class_exists() and then dynamically create a class instance from a class name, possibly passing the container into a static $class::create() method after a method_exists(), or we need to resolve the plugin id to create the preprocessor object.
• If we are smart, this happens only once for each theme hook.
  I think the plugin manager will take care of that.
• Preprocess objects (plugins?) get their dependencies injected, so they don't have to call Drupal::service(..).

The injected dependencies are actually a relevant selling point. Also having an interface is nice.
For performance: The discovery might be faster than current procedural preprocess discovery. But I suspect that an optimized procedural preprocess discovery will be faster than a plugin discovery ever can be.

And then, even if move to something OOP, I am not convinced that the plugin system is the best way to achieve this.

-----

All of this is based on what I imagine your proposal would look like.
I might totally change my mind if I see more details.

Its just "optimized" as much as it can be considering its current procedural/hook-based system.

I strongly doubt that.
Perhaps the execution of preprocess functions is as good as it can be without code generation. Except for the repeated function_exists() call.
The discovery, last time I looked, was far from what is technically possible.

For module hooks, ModuleHandler::invoke() is silly because we cache module names instead of function names, so we need a string concat before each call. And then we attempt to process the return value even for hooks where we don't care about it.

Hi,
I apologize for the negate energy. I don't know if I can avoid it, and still say what I need to say.

I was coming from #2954402-11: Refactor ThemeManager::render(), split into smaller immutable objects. and -16.
One could categorize this as "discouragement", but in the end, I think you should scrutinize me all day, if you think I have bad ideas, as long as you explain your reasons.

In your comments over there, you suggested that the "theme hooks as plugins" would either conflict with my proposed refactoring, or make it obsolete. This is why I went here, hoping for a clearer picture of what you are planning to do, so that I could decide if I like it, and if so, adjust my own patches.

So now I don't have a clear picture what you are planning, but also cannot proceed with my own proposed refactoring.
I think I will wait until after DC Nashville. I became curious in this stuff during the last weeks, but it is not really that urgent.

I hope my "discouragement" contains at least some useful feedback.

an attempt to hold onto/fix the procedural based theme system

As you say yourself, the old procedural system is not going away, at least not the public API part of it.
You can add alternatives, but you cannot remove the legacy parts.

So I imagine that refactoring this old logic will still be relevant, even after moving to plugins.

Btw, another refactoring site is the theme registry itself, see #2957440: Refactor ThemeRegistry and Theme\Registry.
I am not sure that my analysis there is accurate and complete, but I definitely think that these classes are not in the shape that they should be.

POCs are how I determine what is actually feasible/makes DX sense

Imaginary consumer code is less work than a working POC. The DX equivalent to a paper prototype. And it can give you your first test case.
But of course a POC can also bring some insight. Do what you need to do.

It just has to be "supported" (i.e. convert to a legacy based plugin) for BC/migration purposes... that's all. We don't have to "fix" it.

Well, I am not attempting to "fix" the system itself, just refactor its implementation. This is a big difference.

When we create the new system, we deprecate the old one and tell people to not use it. We don't have to "fix" the old sytem, it "works as designed", within the many, many limitations it has.

Again, I am not "fixing" or changing the way people can use the old system.

It just has to be "supported" (i.e. convert to a legacy based plugin) for BC/migration purposes... that's all. We don't have to "fix" it.

And this "legacy plugin" can be nice-looking or ugly.
Judging by the complexity of the existing/old system and its current implementation, it will be ugly, and we will likely introduce bugs or unintended behavior changes when doing this.

Attempting to wrap it with OO code, while this may work for your code, will only introduce confusion and more technical debt that we'll have to maintain until it can be removed in 9.x.

I don't get this.
OOP code is not a nice-looking costume, it is, if done right, the most maintainable way to implement something. Whether this "something" is from a past era does not matter.
I don't do this stuff to make it look nice superficially, but because this is, as I see it, the best tool for the job.

We'll be introducing new internal APIs that, ultimately, we should never really use nor advertise. I can see the benefit of this for personal/custom code when attempting to refactor or understand how something works, sure... but not for core.

If you mean the additional classes and interfaces that I proposed in the other issue:
Yes, these classes would only be used for this one purpose. They might still be usable as parts of your "legacy plugin", but still it would be a very limited application.
I think this does not matter at all. The worst that could happen is that we leave some classes and interfaces that are not used anymore. These files would not be included on an average request, and we'd only keep them in case some odd contrib or custom module ignored the "@internal" tag.

The idea that "internal" and one-off interfaces are to be avoided leads to the unhealthy situation we have now. Classes which do far too much on their own.
See #2956549: [policy, no patch]: Atomic behavior objects

Of course not every one-off internal interface is good. Perhaps in #2954402: Refactor ThemeManager::render(), split into smaller immutable objects. I do go one step too far. I have a branch locally, and made the patch based on the latest commit.

About any hook to plugin:

https://www.drupal.org/project/hook_manager

> But there is no fixed 1:1 mapping between theme hook names and template files.

This is something that's really confusing about the theming system. There is often a theme hook name and a template file with a matching name, which can make it seem like there's something automatic going on, but AFAICT there isn't.

This also makes it confusing to understand what you're going to get if you use #theme vs #type in render arrays.

E.g. #theme => table and #type => table give you slightly different results!

This is something that's really confusing about the theming system. There is often a theme hook name and a template file with a matching name, which can make it seem like there's something automatic going on, but AFAICT there isn't.

Exactly this ^^ !