Move hook_theme implementations into yml

+1 for this issue. Currently the default values though can be anything in PHP and are mostly array or scalar values. Though we can put objects in there. Would it be possible to define say a default object in yml config? For example: new Attribute(['class' => ['happy']]) or will we have to explicitly exclude typed data?

+++ b/core/modules/help/help.theme.yml
@@ -0,0 +1,7 @@
+      description: => null

=> looks like a typo

But the fails are about the interface not implementing a method it seems

1. +++ b/core/lib/Drupal/Core/Theme/ThemeComponentDiscovery.php
   @@ -0,0 +1,71 @@
   +    $components = [];
   +    foreach ($this->moduleHandler->getModuleList() as $module) {
   +      $module_name = $module->getName();
   +      $path = $this->getPath($module_name);
   +      $file = $path . '/' . $module_name . '.theme.yml';
   +      $components[$module_name] = $this->componentParser->parse($file);
   

   Is there a reason we don't leverage \Drupal\Component\Discovery\YamlDiscovery here? It could simplify this a bit, I guess.

   An alternative could be also to use '%container.modules%' which provides the path you need. Even if not, you could use $module->getPath() here.

2. +++ b/core/modules/help/help.theme.yml
   @@ -0,0 +1,7 @@
   +      description: => null
   

   wait, is that valid syntax?

#8.1++

Is there a reason plugins aren't appropriate for replacing hook_theme?

This switches to using YamlDiscovery. Still using the moduleHandler since we need it for that now.
Also fixes the test fails.

Here's the thing: the changes to help are incorrect. Yes @joelpittet is right, that's invalid syntax.
Additionally, the YAML file is nested under a components: section. Why? That's not handled by the patch at all. And what else would possibly go in there?

Also, why is this only used for modules? Why not opt in everything: themes, base themes, theme engines.

Finally, are we sure we want to disable checking hook_theme() if a *.theme.yml file is present? If so, we need tests for that. If not, we need to fix it and also add tests.

To answer #11:
Elements (#type) are plugins. But that is because they have functionality associated with them, like #pre_render and #process and #value_callback methods. Additionally, they can be altered or swapped out.
Theme hooks (#theme) have none of that. Another module should be altering the #theme hook of another module (or theme!)

Turns out there was sufficient test coverage for the help.module changes.

Implement DiscoverableInterface directly
Check for a components key.

Sorry for being difficult, just not understanding the reasoning. There are other types of plugins which are simple yaml definitions with a simple provided class backing it. Breakpoints come to mind. Plugin managers solve the discovery aspect for you and altering plugin definitions is optional. Are the semantics really that far apart? Do we need another concept to define "thing like a plugin but not", wherever the differences lie. Libraries come to mind as another plugin-like snowflake implementation. Could the plugin API be extended to match the desired behaviour?

Breakpoints become objects that are directly interacted with.

Theme hooks are not, they are immediately consumed by the Registry class, and not exposed on runtime due to caching.

Permissions are also YAML files but not plugins, for a similar reason.

Okay so here's another argument for using plugin-based discovery: derivatives.
I'm currently working on a patch that has a foreach loop in hook_theme. That would map perfectly to derivatives.

My initial reaction was, why would you ever need plugins here, but then there are some good arguments for plugins:

• It would not add runtime costs, the theme registry deals with that already
• More consistency
• Given we have abstractions already, for example a BC which requires every variable to be defined, might be easier

May be missing the point in the issue summary when suggesting plugins:

difficult for themers to find the contents of these functions.

A YAML file seems a bit more approachable for the simple cases, the more elaborate cases may be useful as plugins but I don't think that is the intent behind this issue.

Can we/should we do both? Follow-up?

We can do YAML based discovery, without annotated classes. But with derivatives as an option for more complex needs.

Initial rough conversion to plugin discovery.

Had to remove the "components" top level key. Also changed it to *.theme_implementations.yml for now, because I'm not sure we want to squat *.theme.yml

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -166,6 +182,7 @@ public function __construct($root, CacheBackendInterface $cache, LockBackendInte
   +    $this->themeImplementationManager = $theme_implementation_manager;
   

   Given we don't need it most of the time, I'm wondering whether this should be a lazy service.

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -371,6 +389,46 @@ protected function build() {
   +
   +    // Invoke the hook_theme() implementation, preprocess what is returned, and
   +    // merge it into $cache.
   +    $function = $name . '_theme';
   +    if (function_exists($function)) {
   +      $result = $function($cache, $type, $theme, $path);
   +    }
   

   I would have expected to have this hook to be part of the plugin manager conceptually. Wait, its actually there.

The first big problem here is the passing of $existing to hook_theme(). Each implementation gets the *processed results* of all the implementations that ran before it.

However, the only places in core that use that are theme engines: nyan_cat_theme() and twig_theme().
Zero themes or modules use ANY of those provided pararameters, even in tests.
Furthermore, theme engines cannot provide YAML files.

The lack of test coverage makes me very nervous about refactoring/rewriting this.

Also, I am not actively working on this, unassigning.

@tim.plunkett you are right those params don't get used except theme engines and there is a bug in it that I've sorted that out a bit here: #2817103: Theme registry doesn't find templates in subfolders for self-defined theme hooks

-1 to a plugin based approach

We will need a refactoring of this at some point in time, but I think we should do that with the introduction of a new API - there is too much baggage we need to refactor anyway.

--

Simple proposal to get this in without many changes:

- Allow specifying the owner (module) in the the returned info structure of hook_theme(). (@internal, but useful for edge cases)

function system_theme() {
  return \Drupal::service('theme.manager.discovery')->getAll();
}

- Return all information from the YamlDiscovery (modules, themes) in this one theme hook, but set

$info[$hook]['owner_override'] = [$type, $name]; 

e.g.

// Block module with theme block
$info['block']['owner_override'] = ['module', 'block'];

// Theme with theme custom
$info['custom_widget']['owner_override'] = ['theme', 'custom'];

- Allow this owner override to replace the data that would be auto-populated by the theme registry depending on where the hook comes from

Advantages:

- Zero disruption for existing code
- Only API additions
- Minimal changes in the internals of the theme system
- hook_theme is still available for more complex cases

-----

Edit:

To clarify, those theme hooks collected via the new mechanism could also be plugins - that is okay. My -1 was more to the implementation, in the end the consumer does not care where the structured info array comes from - be it YAML or a plugin or both ...

I just would like to change the theme system itself as less as possible for it - because as Tim rightly said: We lack test coverage.

So having system_theme() just report the hooks for all modules + themes seems like a good idea to reduce the complexity of the change.

However we have to be careful of order of overwrites, though I am not sure how defining the same theme hook twice is supported right now.

I openned this issue a while ago Declare themes on YAML file as an alternative to hook_theme

Styles API is doing something in my understanding should be done by CORE.

That is exactly what is discussed here.

@hatuhay except that layouts are plugins, which contradicts #30

@tim.plunkett talking about the way they are declared, not how the declaration is handled.
Meanning the posibility to declare themes for specific components (like blocks, fields and regions) in modules or themes to extend other modules or themes.
And adding extra UI information like thimbnails or descriptions.

This is great! I think we could take it one step farther and make it a part of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers which could allow YAML plugins to be discovered.

I have adapted patch for last dev branch

Sorry, I think I misspoke in #39...

I was originally marking issues as "Needs work/reroll" because I was simply going through the backlog of issues.

Then when I started making the comment and mentioned the other issue, I forgot to change the status to a dup of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers. That issue is basically the same as this but allows us to put an entire plugin manager behind the discovery process.