Convert theme hooks (defined by hook_theme()) to be objects

Initial thoughts --

I like the direction this is going. A few minor observations, in no particular order:

• I think the support for theme functions (getFunction(), setFunction(), etc.) should be moved into a class that descends from ThemeHook, named something like ThemeFunctionHook. That way it's super easy to isolate, deprecate, and ultimately remove. Perhaps setFunction()'s $function parameter should also have the callable type hint so that we can pass a static class method and, if I'm not mistaken, implicitly validate the function exists. If it's not OK to have a type hint (I guess due to BC concerns), it should probably call function_exists() or otherwise silently validate the argument.
• I like the ArrayAccess BC layer. I think it should be moved into a trait so that this same technique can be applied to other arrays of doom that need to be modernized. Naming it oughta be fun (ArrayOfDoomAdapterTrait?)
• Should there be a ThemeHookInterface?
• setPath() says it requires setTemplate() to be called. It seems to me that setTemplate(), then, should accept two arguments -- the template and (optionally) the path, which could be automatically determined from the provider if not passed.
• Should setPreprocessFunctions() be documented as an array of callables, rather than strings? It'd be nice to "officially" support static class methods in that way. Same thing for addPreprocessFunction().
• I don't like the naming of getOverriddenPreprocessFunctionStatus(). Could it be renamed to something like ignoreStandardPreprocessing()? I dunno. This is more of a knee-jerk reaction to long method names than something I'm vehemently against.

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -380,28 +375,10 @@ protected function build() {
   -   * @param array $cache
   +   * @param \Drupal\Core\Theme\ThemeHook[] $cache
       *   The theme registry that will eventually be cached; It is an associative
   -   *   array keyed by theme hooks, whose values are associative arrays
   -   *   describing the hook:
   -   *   - 'type': The passed-in $type.
   -   *   - 'theme path': The passed-in $path.
   -   *   - 'function': The name of the function generating output for this theme
   -   *     hook. Either defined explicitly in hook_theme() or, if neither
   -   *     'function' nor 'template' is defined, then the default theme function
   -   *     name is used. The default theme function name is the theme hook
   -   *     prefixed by either 'theme_' for modules or '$name_' for everything
   -   *     else. If 'function' is defined, 'template' is not used.
   -   *   - 'template': The filename of the template generating output for this
   -   *     theme hook. The template is in the directory defined by the 'path' key
   -   *     of hook_theme() or defaults to "$path/templates".
   -   *   - 'variables': The variables for this theme hook as defined in
   -   *     hook_theme(). If there is more than one implementation and 'variables'
   -   *     is not specified in a later one, then the previous definition is kept.
   -   *   - 'render element': The renderable element for this theme hook as defined
   -   *     in hook_theme(). If there is more than one implementation and
   -   *     'render element' is not specified in a later one, then the previous
   -   *     definition is kept.
   +   *   array keyed by theme hooks, whose values are \Drupal\Core\Theme\ThemeHook
   +   *   value objects.
   

   I love how moving to value objects allow you to centralize documentation

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -599,34 +467,37 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +
   +    $cache[$hook]->markComplete();
   

   The alternative could be to have a builder object which returns a ThemeHook object when its marked as complete. This would allow to make the ThemeHook object to be a bit easier. I'm not sure its worth the tradeoff though

3. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1026 @@
   +  /**
   +   * An array of default values to be passed to the template.
   +   *
   +   * @var mixed[]|null
   +   */
   +  protected $variables;
   ...
   +  /**
   +   * Returns the default values to be passed to the template.
   +   *
   +   * @return mixed[]|null
   +   *   An associative array where the keys are names of variables, and the
   +   *   values are the default values if they are not given in the render array.
   +   *   If this returns NULL, self::getRenderElement() will be used instead.
   +   */
   +  public function getVariables() {
   +    return $this->variables;
   +  }
   

   I'm curious, how can the available variables be null? Could it not also be an empty array by default?

   I'm wondering whether having a RenderElementThemeHook and a normal ThemeHook would clarify things a bit

4. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1026 @@
   +    // If this object doesn't have a value, use the value from the other object.
   +    if (!$result->getProviderType()) {
   +      $result->setProviderType($other->getProviderType());
   +    }
   

   This behaviour is a bit weird ... the $other is ignored when a value in $resultis set.

   At least for me this is not an expected behaviour given how array_merge and \Drupal\Core\Cache\CacheableMetadata::merge works.

   Maybe just document that the existing theme hook "wins"?

5. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1026 @@
   +  public function addInclude($include) {
   +    $includes = $this->getIncludes();
   +    $includes[] = $include;
   +    $this->setIncludes($includes);
   +
   +    return $this;
   ...
   +  public function setPreprocessFunctions(array $preprocess_functions) {
   +    $this->preprocess_functions = array_values(array_unique($preprocess_functions));
   +
   

   Should we unique in both places?

6. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1026 @@
   +   * Process a theme hook.
   +   *
   +   * This is intended for usage by the theme registry.
   +   *
   ...
   +   */
   +  public function process($root, $theme, array $module_list, ThemeHook $existing_theme_hook = NULL) {
   +    $hook = $this->getName();
   ...
   +   */
   +  protected function handleIncludes($root, ThemeHook $existing_theme_hook = NULL) {
   +    if ($existing_theme_hook && $existing_theme_hook->hasIncludes()) {
   +      $this->setIncludes($existing_theme_hook->getIncludes());
   +    }
   

   It feels a bit weird to have these complex functions on this value object. I can't articulate why though to be honest.

7. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -189,26 +189,26 @@ public function render($hook, array $variables) {
   +      if ($info->getVariables()) {
   +        foreach (array_keys($info->getVariables()) as $name) {
   

   Should there maybe be a hasVariables method? Given that this is about renderElement vs. not renderElement maybe isRenderElement or if ($info instance RenderElementThemeInfo) or so could be also used here

8. +++ b/core/modules/locale/tests/modules/locale_test/locale_test.module
   @@ -154,7 +154,7 @@ function locale_test_theme() {
      $return['locale_test_tokenized'] = [
   -    'variable' => ['content' => ''],
   +    'variables' => ['content' => ''],
      ];
    
   

   Nice find!

I think the support for theme functions (getFunction(), setFunction(), etc.) should be moved into a class that descends from ThemeHook, named something like ThemeFunctionHook

That is a great idea!

One thing I would wonder: how big is the theme registry cache entry afterwards? Is it growing significantly?

I didn't read through the entire patch, but I found these minor things:

1. +++ b/core/includes/theme.inc
   @@ -149,20 +150,23 @@ function drupal_find_theme_functions($cache, $prefixes) {
   +      if (!$info->getBaseHook() && !empty($pattern) && isset($grouped_functions[$first_prefix])) {
   

   The !empty($pattern) check is no longer necessary here.

2. +++ b/core/includes/theme.inc
   @@ -149,20 +150,23 @@ function drupal_find_theme_functions($cache, $prefixes) {
   +            if ($info->getVariables()) {
   +              $implementations[$new_hook]->setVariables($info->getVariables());
   +            }
   +            else {
   +              $implementations[$new_hook]->setRenderElement($info->getRenderElement());
   +            }
   

   Why isn't this done in ThemeHook::createFromExisting()?

3. +++ b/core/includes/theme.inc
   @@ -256,8 +257,8 @@ function drupal_find_theme_templates($cache, $extension, $path) {
   +    $pattern = $info->getPattern() ?: $hook . '__';
   +    if (!$info->getBaseHook() && !empty($pattern)) {
   

   !empty($pattern) is not necessary because the previous line guarantees a value.

4. +++ b/core/includes/theme.inc
   @@ -270,13 +271,17 @@ function drupal_find_theme_templates($cache, $extension, $path) {
   +          if ($info->getVariables()) {
   +            $implementations[$hook_name]->setVariables($info->getVariables());
   +          }
   +          else {
   +            $implementations[$hook_name]->setRenderElement($info->getRenderElement());
   +          }
   

   Same question as before -- seems like something that createFromExisting() should do.

5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -380,28 +375,10 @@ protected function build() {
   +   *   array keyed by theme hooks, whose values are \Drupal\Core\Theme\ThemeHook
   

   A little early for a nitpick like this, but 'array' should be capitalized.

@tim.plunkett, is there a way to distill this down to a review-only patch? Something that just show the essentials of the change w/o all the conversions (maybe just one)? Or is this really a 100k patch to be reviewed?

I looked at this, and I think a test-only patch that has the essential changes and then relies on the BC array access for everything else would be a good idea when you feel this is stable. Maybe that is something that could also be a review-only patch?

This is going to take some time to review; I am more of a casual bystander here. I am not sure if I can properly review this, as I don't fully understand this. However I spent some time look at this, and from an overall product standpoint, I think this is addresses some of the current baggage we have in core in a good way. I also want to give it a look applied, which I wasn't able to do.

One thought, though, that permeates the patch...

+++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
@@ -0,0 +1,1067 @@
+  /**
+   * Sets the function name to invoke for this implementation.
+   *
+   * If this is set, self::getTemplate() will be ignored.
+   *
+   * @param string $function
+   *   The function name to invoke for the theme implementation.
+   *
+   * @return $this
+   *
+   * @deprecated in Drupal 8.0.x, will be removed in Drupal 9.0.x.
+   */
+  public function setFunction($function) {
+    $this->function = $function;
+
+    return $this;
+  }
+

Do we buy anything if we make $function a callable? I am pondering if support now for allowing for the class/object+method will put in a better place for future refactoring (though I suspect that doing more of this type of stuff with annotations may be the road we go down).

I really like the direction here.

I feel like, if we're going to go the route of turning these into objects, we may as well go all the way and have the full ability of the plugin system as well.

This is essentially the same thing as @RenderElement, but for #theme instead of #type.

I personally would prefer something simple like @Theme over @ThemeHook since "hook" is really just a way to indicate the procedural implementations in Drupal prior to 8.x.

This would allow us to add a preprocess method and tie the definition of a theme hook with the necessary code that preprocesses it.

Thus, ultimately allow subclassing to also give us a better ability to determine when something should be inherited or overridden.

I'm currently implementing a few custom/theme specific plugins in Bootstrap that do very similar things: @BootstrapPreprocess.

It's amazingly liberating to have that much control over when, where and how something is preprocessed in the theme system.

---

I'm adding the related "yml" issue because it's essentially trying to do the same thing here: get rid of arrays. However, given that theme hooks inherently come with PHP code (usually to preprocess variables), I doubt it would make much sense to continue down that direction. Plugins are a better approach.

a) If these were plugins, these ThemeHook objects would be the equivalent of a plugin definition, not a plugin instance.

Correct. This would mostly be the @Theme annotation class. Any actual logic for discovery/conversion of legacy theme hook arrays should be done in the plugin manager.

b) Render elements were barely worth making plugins IMO, and only since they have the need for inheritance (MachineName is a Textfield is a FormElement)

There are multiple layers/opportunities to alter the final output of a theme hook, this is the exact the reason they should be plugins. Theme hook suggestions also increase the importance of why this necessary since they add even more variety. That's definitely more than "barely" worth it.

c) ... Preprocess functions could not use such a thing.

This isn't true.

We're doing it in Bootstrap just fine. Granted, there are some advanced work arounds because core's invocation of preprocess functions are still manual (i.e. not using CUFA), but the POC is stable and one of the reasons why Bootstrap already has 22,188 reported installs for 8.x.

This would obviously still need to keep an internal array of preprocess callbacks. One that could easily add template_preprocess_* legacy procedural functions if they existed.

Allowing preprocessing of a theme hook inside a class is a massive DX improvement.

We'd have the ability to override existing implementations and choose wither or not to call parent::preprocess().

Are there a couple current "limitations" around this topic, sure?

However, it's mainly because the internals of the theme system were never really refactored to be OO and they are seriously overdue.

That doesn't mean this cannot easily be solved if we finally architected this properly.

The two primary pain points are:

1. Adding theme's namespaces to plugin discovery (perhaps even as a dedicated ThemePluginManager, so modules can opt-in to this)
2. twig_theme()'s use of $existing as you mentioned in the other issue. Technically, this should just be moved to the alter phase of the discover because it's not actually providing new theme hook info. For legacy hook_theme() implementations, we could easily pass the current list of definitions since they would/should be last in the plugin discovery process.

I tried to make them plugins (see the issue you linked), it didn't work, I'd like to not try that again.

I completely understand and I'm not trying to ignore previous efforts.

I am, however, extremely familiar with 8.x's current procedural implementation of the theme system + adding OO on top of it.

A lot of the code I've put in Bootstrap could easily be abstracted into core and what I had originally intended https://www.drupal.org/project/basetheme to be.

I don't believe we should add something that, if eventually replaced, will also have to be supported for BC reasons.

If this issue aims to make this OO, we may as well go all the way.

I'm willing to help make this a reality because, as someone who uses this on a daily basis, it's definitely needed in core moving forward.

---

edit: I'm just asking to keep an open mind and let me help solve these issues with you. I know it's going to be hard, there's no doubt. I'd just like the chance to do this right.

Not talking about annotation classes, but object-based plugin definitions.

I know exactly what you're talking about.

See \Drupal\Core\Layout\LayoutDefinition for an example.

I maintain https://www.drupal.org/project/bootstrap_layouts too, remember...

Can you please stop being dismissive? I'm trying to help here and actually give this issue valid feedback from real world scenarios/development/contrib.

Your responses seem to be implying that I don't fully grasp this issue or the complexities of the theme system.

---

Theme hooks aren't layouts. Many layouts use the same code (PHP class) and it indeed makes sense to use LayoutDefinition/YAML in that instance.

Theme hook are, for the most part, completely unique and either implement or inherit a preprocessor function of some kind.

What you're proposing continues the fragmentation of code in the theme system. It keeps the theme hook definitions separate from its preprocessing. It also keeps them as procedural functions which is, inherently, a PITA to override and requires a mountain of boilerplate code in both core and contrib to provide proper inherence/overrides.

This is primarily about DX/TX. Especially in the contrib/custom realm.

I would much rather we have:

use \Drupal\Core\Theme\ThemePluginBase;

namespace Drupal\system\Plugins\Theme;

/**
 * @Theme(
 *   "id" = "input"
 *   "render element" = "element",
 * )
 */
class Input extends ThemePluginBase {

  /**
   * {@inheritdoc}
   */
  public function preprocess() {
    $element = $this->element;
    // ...
  }

}

and

use \Drupal\Core\Theme\ThemePluginBase;

namespace Drupal\system\Plugins\Theme;

/**
 * @Theme(
 *   "id" = "table"
 *   "variables" = {
 *     "header" = NULL,
 *     "rows" = NULL,
 *     "footer" = NULL,
 *     "attributes" = {},
 *     "caption" => NULL,
 *     "colgroups" => {},
 *     "sticky" => FALSE,
 *     "responsive" => TRUE,
 *     "empty" => '',
 *   },
 * )
 */
class Table extends ThemePluginBase {

  /**
   * {@inheritdoc}
   */
  public function preprocess() {
    $header = $this->header;
    // ...
  }

}

Than "object-based plugin definitions" from YAML files.

This doesn't need or warrant a middle-class.

What you're proposing continues the fragmentation of code in the theme system. It keeps the theme hook definitions separate from its preprocessing. It also keeps them as procedural functions which is, inherently, a PITA to override and requires a mountain of boilerplate code in both core and contrib to provide proper inherence/overrides.

I think what @tim.plunkett tries to solve ia one specific problem of the theme system aka. the debuggablity of the theme registry, while @markcarver tries to improve the actual runtime operations. While they are connected with each other, they don't have to be resolved as part of this issue. Once you have the objects which contain the theme registry information (ThemeHookInfo()) you can afterwards still convert things to actual plugins. Doing things in two steps makes it possible, at least for me, to reason about it.

As a sidenode: On D8 projects I've used inheritance for preprocess functions using classes quite a bit, even well, I hope we move away from preprocess on the longrun, and keep things on the template level. There is also a lot of potential overlap with pre_render functions of elements.

@markcarver Very sorry you took it this way, I'm just really spread thin right now, and trying to chime in when I can.

Same here. I appreciate the clarification. I didn't mean to take it that way, but it was kind of hard not to.

Let's continue this discussion

Yes. Let's.

I think what @tim.plunkett tries to solve ia one specific problem of the theme system aka. the debuggablity of the theme registry, while @markcarver tries to improve the actual runtime operations.

Yes and no. The actual runtime implementation/preprocess method can technically be done in subsequent follow-up issues.

I'm fine with that aspect since this will have to support BC with existing hooks anyway.

I just want to make sure that others realize the true potential we have with plugins here.

By coupling the theme hook definition inside the plugin, we have more cohesion and ability to notice or update a change to the theme hook.

I cannot tell you how many times I've run into bugs because I "forgot" to also update the default values implemented in said hook_theme().

Moving these to YAML files only keeps this level separation.

As far as debuggability goes, we'll have the same with plugins, if not more so. Annotated plugins are all classes. Classes that can be stepped through rather than a single YAML file that's only parsed once.

I hope we move away from preprocess on the longrun, and keep things on the template level.

Yes. That was the ultimate goal, but the reality this is PHP. That isn't changing anytime soon. There are way too many real world complex "conditionals" that have to be done in the preprocess layer that simply don't work anymore in Twig. This is mainly because Twig isn't PHP and now requires building extensions... or the easier existing solution: preprocessing variables.

A simple example is: adding icons or classes based on contextual information that doesn't make its way directly to the template (i.e. $element['#context']).

There is also a lot of potential overlap with pre_render functions of elements.

Possibly, but perhaps only the "callback/inheritance" bits. Pre-renders are usually only ever used on #type elements and ultimately still make their way to whatever implements #theme.

Given there is more potential we want the new object to be marked as internal so we can continue to refactor it.

That looks very promissing, thanx for moving this forward!

Would great to get Mark's idea with inheritance for flowing issues
- #2307103: Follow-up: remove explicit comment field theme hook suggestion implementation
- #2808481: Introduce generic entity template with standard preprocess and theme suggestions

btw Related may need updates

I'm OK with ThemeHook being marked as @internal. As @tim.plunkett said, this would allow us to do anything to it in the future.

I just didn't want themes to suddenly have to start providing .yml files which keeps the separation of definition and, more than likely, the PHP code necessary to preprocess the theme hook definition.

This will allow us to create a proper plugin system for theme hooks in the future and likely migrate this OO code into that.

I really like the changes. Its a bit the logic step what we did when we worked on this class YEARS ago.

I just didn't want themes to suddenly have to start providing .yml files which keeps the separation of definition and, more than likely, the PHP code necessary to preprocess the theme hook definition.

I hop

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -343,6 +343,7 @@ protected function build() {
        foreach (array_reverse($this->theme->getBaseThemes()) as $base) {
   ...
   +      /** @var \Drupal\Core\Theme\ActiveTheme $base */
   

   Do you understand why phpstorm doesn't know the type based upon the typehint?

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -366,12 +367,6 @@ protected function build() {
        // @todo Implement more reduction of the theme registry entry.
   

   I'm glad that you didn't just removed the todo :)

3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -599,34 +485,37 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
      protected function completeSuggestion($hook, array &$cache) {
   ...
   -    while ((!isset($cache[$previous_hook]) || isset($cache[$previous_hook]['incomplete preprocess functions']))
   +    while ((!isset($cache[$previous_hook]) || $cache[$previous_hook]->isIncomplete())
   ...
   -      $this->mergePreprocessFunctions($hook, $previous_hook, $incomplete_previous_hook, $cache);
   +      $this->mergePreprocessFunctions($hook, $previous_hook, $cache, $incomplete_previous_hook);
   ...
   +      $cache[$hook] = $cache[$hook]->merge($cache[$base_hook]);
   

   This is weird for me at least. We have ::merge() and $this->mergePreprocess() now, even we had two methods before. Either their is some weird change in behaviour, or we could reuse some code?

4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -599,34 +485,37 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +        $incomplete_previous_hook = clone $cache[$previous_hook];
   

   I'd be nice to have documentation why this clone is needed.

5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -599,34 +485,37 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +    $base_hook = $cache[$hook]->getBaseHook();
   +    if ($base_hook && isset($cache[$base_hook]) && !$cache[$base_hook]->isIncomplete()) {
   

   Can/Should we inline this line using

   if (($base_hook = ...)
    ... 

   ?

6. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -738,18 +634,8 @@ protected function postProcessExtension(array &$cache, ActiveTheme $theme) {
   -      if (isset($info['incomplete preprocess functions'])) {
   +      if ($info->isIncomplete()) {
   

   It is a bit weird that the information about preprocess got lost. I bet you did that on purpose, I'd just like to understand it a bit more ...

7. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1070 @@
   +    $instance->handleDefaultValues($other);
   ...
   +    $this->handleDefaultValues($existing_theme_hook);
   ...
   +  protected function handleDefaultValues(ThemeHook $existing_theme_hook = NULL) {
   

   It is a bit weird that the parameter is optional to be honest. It is internal only, so I think different rules apply, but at least for a public api I'd have tried to avoid the optional parameter

8. +++ b/core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php
   @@ -141,7 +141,16 @@ public function testSuggestionPreprocessFunctions() {
    
   +    // Test an object-based theme hook.
   +    $info = $registry_theme->get()['theme_test_object_based'];
   +    $this->assertSame(['template_preprocess'], $info['preprocess functions']);
   +    $this->assertSame('elements', $info['render element']);
   +
   

   Some assertInstanceOf could be nice here.

9. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -191,6 +192,20 @@ public function testGetRegistryForModule() {
    
   +    // @todo The BC layer is in \Drupal\Core\Theme\Registry::processExtension()
   +    //   which this test bypasses via reflection. These legacy conversion calls
   +    //   are necessary until the test data and expectations are converted.
   +    foreach ($expected as $name => $hook) {
   +      if (is_array($hook)) {
   +        $expected[$name] = ThemeHook::createFromLegacy($name, $hook);
   +      }
   +    }
   +    foreach ($hooks as $name => $hook) {
   +      if (is_array($hook)) {
   +        $hooks[$name] = ThemeHook::createFromLegacy($name, $hook);
   +      }
   +    }
   

   Super nice idea for the test coverage.

1) array_reverse eats the typehint :(

No worries, I was really just curious.

Renamed, hopefully it becomes clearer

Oh totally! Thank you for that.

4) Neither of the clones are needed, now that I reread the code. They were defensive against some relics of the code that only existed to support += of arrays.

Oh nice!

5) I thought you were against assignment in complex conditionals? Leaving as is, it looks more confusing combined.

Mh yeah I think I don't have a strong opinion about it, if I'm honest. Yeah keep it as it is.

One tiny nit:

+++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
@@ -0,0 +1,1070 @@
+ * @todo Consider removing \ArrayAccess in Drupal 9.0.x.

Since this is now marked as @internal I think this @todo should be changed to mention refactoring into a proper plugin manager instead with this issue's link as a reference.

Wfm, thanks! :D

+++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
@@ -0,0 +1,1072 @@
+    $name = str_replace(' ', '_', $name);
+    if (property_exists($this, $name)) {
+      $this->{$name} = $value;
+    }

Let's not use the snake case properties. @larowlan wrote some code to translate array values to object properties.

use Symfony\Component\DependencyInjection\Container;

/**
 * Defines a class for reading object properties from an instance.
 */
class ObjectReader {

  /**
   * Given an object determine the metatdata.
   *
   * @param \SomeInterface $object
   *   The object to fetch metadata for.
   *
   * @return array
   *   Array of field properties keyed by field name.
   */
  public static function getObjectDetails(SomeInterface $object) {
    $class = new \ReflectionClass($object);
    $fields = [];
    // Get all properties.
    foreach ($class->getProperties(\ReflectionProperty::IS_PROTECTED) as $property) {
      $property_name = ucfirst(Container::camelize($property->getName()));
      if (empty($fields[$property_name]['getter'])) {
        $candidate_getter_prefixes = ['get', 'is', 'has'];
        foreach ($candidate_getter_prefixes as $prefix) {
          $method_name = $prefix . $property_name;
          if ($class->hasMethod($method_name) && $method = $class->getMethod($method_name)) {
            if ($method->isPublic() && empty($method->getParameters())) {
              $fields[$property_name]['getter'] = $method_name;
              break;
             }
          }
        }
        if (empty($fields[$property_name]['getter'])) {
          throw new \Exception(sprintf('No getter found for %s::%s', get_class($object), $property_name));
        }
      }
      if (empty($fields[$property_name]['setter'])) {
        $candidate_setter_prefixes = ['set', 'is'];
        foreach ($candidate_setter_prefixes as $prefix) {
          $method_name = $prefix . $property_name;
          if ($class->hasMethod($method_name) && $method = $class->getMethod($method_name)) {
            if ($method->isPublic() && !empty($method->getParameters())) {
              $fields[$property_name]['setter'] = $method_name;
              break;
            }
          }
        }
        if (empty($fields[$property_name]['setter'])) {
          throw new \Exception(sprintf('No setter found for %s::%s', get_class($object), $property_name));
        }
      }
      return $fields;
    }

}
function translate_values_to_object(\SomeInterface $object, array $input = []) {
  $meta = ObjectReader::getObjectDetails($object);
  foreach ($meta as $field_name => $details) {
    if (isset($input[$field_name])) {
      if ($input[$field_name] === '') {
        $input[$field_name] = NULL;
      }
      // We know the method exists, as the object-reader throws an exception
      // if it does not.
      $object->{$details['setter']}($input[$field_name]);
    }
  }
  return $object;
}

This is a LOT of code in order to simply deal with CS. I think the alternative is to simply hardcode a mapping between internal and external variable.

This can be simplified. I just shared it as a POC. I think mapping is a good option.

For BC it seems to be overkill.

I know but snake_case properties bring back the D7 memories. :P

Can't we iterate on this after its in?

Sorry to change the status from RTBC but IMO why introduce the obsolete layer if we are going to replace it anyway. Anyway if you still think it's fine as is then feel free to RTBC it.

We have snake cases all over the place ...

Yeah like config entities for example.

why introduce the obsolete layer if we are going to replace it anyway

Baby steps. The internals of the theme system API are extremely complex.

This is a temporary "solution" to unblock Layout API (@tim.plunkett, issue/reference?), that is why it's marked as @internal.

The internal theme system is built using arrays that can be modified in any number of arbitrary ways at any given point in the process.

This issue at least gets us to a place where we can start start to OO and better test theme hook definitions.

We can iterate on it further or, the likely case, add on on top of/replace entirely with something better (e.g. plugins) as the follow-up issue suggests.

This looks really nice! Would love to have a current theme system maintainer give this a look.

Heh, yeah, that was a while ago #2350981: Remove Mark Carver from MAINTAINERS.txt ;)

I wasn't aware you were referring to me, I thought you were referring to @effulgentsia as you had mentioned in IRC.

No worries :D

Spent some time reviewing the patch and it looks great! Thanks for doing this @tim.plunkett!

A couple notes:

1. Not totally sold on the keyword "provider" but I don't have a better suggestion, any insight into why you chose that, is there some precedence in another system?
2. I've no beef with the snake case properties, they are all protected so someone can fix that when a coding standard is decided.

3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -599,63 +485,71 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +      if (isset($cache[$previous_hook]) && !$incomplete_previous_hook && $cache[$previous_hook]->isIncomplete()) {
            $incomplete_previous_hook = $cache[$previous_hook];
   -        unset($incomplete_previous_hook['incomplete preprocess functions']);
   

   Doesn't this unset() need to be replaced by $cache[$previous_hook]->markComplete()? or is it dealt with in another way

Leaving as RTBC because the last thing I mentioned is an oversight and the first is minor bikeshed gripe.

I started reviewing this a few days ago and @lauriii and I sat down 1-2 days ago to discuss and review it together :)

1. First of all, I think we need to update the documentation of hook_theme() to tell people to use the new thing.
2. I think we need a change record, tagging.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1072 @@
   + * @internal
   ...
   +class ThemeHook implements \ArrayAccess {
   

   @lauriii and I discussed and we agreed that it would be good to make this final for now so that people don't extend it.

   Because we want people to use this new API in their hook_theme() implementations it might not make sense to mark it as @internal. We'll need to support the API and the public methods going forward otherwise we break BC.

   So tl;dr - our suggestion is to make this final instead of internal.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1072 @@
   + *   marked internal until finalized. See https://www.drupal.org/node/2869859
   

   Can we update this follow-up issue to talk about adding test coverage for the legacy array handling? I think we're well covered for tests at this point because there are so many legacy arrays now but that won't always be the case.

@lauriii and I discussed and we agreed that it would be good to make this final for now so that people don't extend it.

In general we should leverage final for objects which don't contain too much logic. If you would mock this class, you are lost with final classes.

3) Technically there is nothing wrong with extending this class. There is no interface for a reason, so they would HAVE to extend it. If someone would choose to override the existing method implementations, that is their prerogative.

For me its about the following: If there are usecases they will come up and we can remove the final again.

Cool, we are finally increasing our knowledge about minimising our API surface.

Thank you tim!

@laurii and I discussed this quite a bit and in great length at DC and I wish it made it's way here.

Marking as both final and @internal is probably best thing for this class.

Converting theme hook info array into objects is just the first baby step; one that will undoubtedly have additional follow-ups.

This implies that this may (or may not) be the end "product" that is created/consumed by the front facing public API.

Thus, we should not be publicly announcing that people (FE) should be using it.

It may be replaced with something better, or be used as a low level object that is created by a higher front facing API: plugins (which is what we'd really want to document).

Instead, and because this class has BC, we should keep the public facing API/documentation (for hook_theme) the way it is and not encourage people/contrib to use it.

Whether or not core converts them into arrays of ThemeHook objects is beside the point.

This issue is entirely for internal purposes and can of course be used by core.

---

Argument: Doing this wouldn't help improve our FX.

Counterpoint: This issue was never about FX, it was about turning arbitrary arrays into objects for better testing/handling/pin-pointing point of failures.

Second counterpoint: the reality is that anyone that is going to be using this in the first place is likely going to be backend developers or unicorns such as myself who actually understand PHP and our theme system in the first place. Regular "front end" developers could care less about this issue.

Third counterpoint: introducing new/changing existing front end facing APIs can have much larger consequences. If we start telling people who use X to start using Y, only to end up replacing it with Z... we're going to have a major cluster-*#&! on our hands. I think it would be wise if we didn't box ourselves in, we've been burned by that in the past.

1. Do we have any idea of the cost of this in terms of performance? Like does the registry take significant longer to build? Unserialize etc?

2. +++ b/core/lib/Drupal/Core/Theme/ThemeHook.php
   @@ -0,0 +1,1072 @@
   +/**
   + * Provides a value object for a theme hook.
   + *
   + * @todo This class exists as an iterative improvement to the Theme API but is
   + *   marked final until it is complete. See https://www.drupal.org/node/2869859
   + *   for next steps. See https://www.drupal.org/node/2873117 for finalizing the
   + *   removal of \ArrayAccess.
   + */
   +final class ThemeHook implements \ArrayAccess {
   

   Really nice to see the @todo here. It was my first though when I saw the \ArrayAccess.

   Another good reason to mark this final and @internal is that I think the name is as about the BC rather than what it is it. Maybe ThemeDefinition or ThemeImplementation - the docs for hook_theme() say

    * Register a module or theme's theme implementations.
    *
    * The implementations declared by this hook specify how a particular render
    * array is to be rendered as HTML.
   

   Also I think the class docs here need improving to say more about what it does and point to other relevant parts of the document on theming.

   Another thought is that we're creating another mutable value object - should we really be doing that? Mutability can lead to all sorts of odd bugs and new behaviour. At the moment a hook_theme can't change its path. With the new theme object a hook_theme implementation could call setPath after constructing the object but that doesn't make sense.

Btw did anyone measure how that change affects memory usage?

Can the next step be #2316941: Use the builder pattern to make it easier to create render arrays after this patch?

@jibran, that issue isn't really related to this issue or subsequent follow-up issues, e.g. using the Plugin API to create initial theme [hook] definitions instead of arbitrary arrays.

That issue primarily focuses on attempting to "OO render arrays". Which, IMO, is the wrong approach, but meh.

Tagging for #63.1 + #64 to get a performance test.

Interesting point @alexpott about mutability of this value object. It's kind of tricky to create this in BC kind of way and make bits less mutable, but maybe we can trim that down to bare minimum? The arrays were also mutable before, by definition, no?

@markcarver, Trying to understand your comment #64's next steps for the work still needing done: is it only related to the final + @internal, or did I miss something more important? Seemed all related to that but I could be missing a bigger message.

@joelpittet, yes, current patch needs final + @internal so we can iterate on this in future issues (see #18 - #25). @tim.plunkett and I had planned to discuss this in length at DC, but unfortunately that didn't happen. We'll be discussing this next week in a little bit more depth. If only to maybe translate all that has been said so far into a better IS for #2869859: [PP-1] Refactor theme hooks/registry into plugin managers and what all that may entail. I suspect we may need future meetings about all this, but there's no reason we cannot leverage an already powerful API to help with a lot of the legwork.

Just arriving on this issue.

hook_theme() should now return an array of \Drupal\Core\Theme\ThemeHook objects
ThemeHook implements ArrayAccess for BC purposes.

This sounds like a BC break.
Looking at the latest patch, the issue summary should be saying "Theme hooks may still return arrays for BC, which will then be transformed into ThemeHook objects automatically."

+  /**
+   * Sets the name of the renderable element.
+   *
+   * Used for render element items only. This name is used as the name of the
+   * variable that holds the renderable element or tree in preprocess functions.
+   *
+   * @param string $render_element
+   *   The name of the renderable element to pass to the theme function.
+   *
+   * @return $this
+   */
+  public function setRenderElement($render_element) {
+    $this->render_element = $render_element;
+
+    return $this;
+  }

Can we call this ->setRenderElementName(), please? My first thought looking at this was that it would accept an array, the "real" render element. It wouldn't make sense here, but as a first-time reader you don't know that.

+  /**
+   * Process a theme hook.
+   *
+   * This is intended for usage by the theme registry.
+   *
+   * @param string $root
+   *   The app root.
+   * @param string $theme
+   *   The theme currently being processed.
+   * @param array $module_list
+   *   An array of module names.
+   * @param \Drupal\Core\Theme\ThemeHook|null $existing_theme_hook
+   *   An existing theme hook, if it exists.
+   *
+   * @return static
+   */
+  public function process($root, $theme, array $module_list, ThemeHook $existing_theme_hook = NULL) {

I would suggest to put this logic into a separate class.
The ThemeHook class is big enough for its main objective, which is, to contain the data of a theme hook definition.
It doesn't need to be able to process itself.

EDIT: I realize this was already discussed above between @dawehner and @tim.plunkett in #6 and #7.
I still think it should be elsewhere, because all of this can be implemented using the public API of ThemeHook.

But I am going to post another proposal (new comment) which makes this point obsolete.

+  /**
+   * Handles merging in any default values from an existing theme hook.
+   *
+   * @param \Drupal\Core\Theme\ThemeHook $existing_theme_hook
+   *   An existing theme hook.
+   */
+  protected function handleDefaultValues(ThemeHook $existing_theme_hook) {

So why not call it mergeDefaultValues() then?

+      if ($type == 'module') {

Let's use === instead of == where possible.
In this case it is safe to replace this, because we expect $type to be a string.

 /**
  * Implements hook_theme().
@@ -70,6 +71,8 @@ function theme_test_theme($existing, $type, $theme, $path) {
     'render element' => 'content',
     'base hook' => 'container',
   ];
+  $items[] = ThemeHook::create('theme_test_object_based')
+    ->setRenderElement('elements');
   return $items;
 }

I think the API would be more useful if it helped you to know which default keys need to be filled in.
E.g. in this case how do you know that you need to call ->setRenderElement()?
So, perhaps have "combined" constructors like ::createForRenderElement()?

More in the next comment.

Looking at functions like ->markComplete(), ->process() etc.
I think the idea here is that hook_theme() returns incomplete ThemeHook objects, and the central registry code completes them.

However, there is no technical guarantee that a ThemeHook object is really complete, even if ->markComplete() was called.

I would like to propose something else:

Have distinct classes for theme hook definitions returned from hook_theme(), and those saved in the theme registry.

Then ->process() would not modify data on the same object, but instead create a new object. And it would be natural that ->process() would live outside either of those objects.

Have distinct classes for theme hook definitions returned from hook_theme(), and those saved in the theme registry.

Where those saved in the theme registry are also those that are used in ThemeManager::render().

This split allows a more immutable design:
- Objects saved in the registry and sent to ThemeManager::render() can be completely immutable.
- Objects in hook_theme() could also be immutable, with wither methods (e.g. ->withTemplate()) that create clones (*). If not too many of those methods are called, the performance hit will be acceptable.
- Processor methods, e.g. to discover preprocess hooks, can do their calculations and then create a new object (the one which will be saved and sent to theme registry).

This makes sure that objects are not modified in the wrong place.

It also allows to hide some parts of the API to hook_theme(). E.g. it blocks hook_theme() implementations from adding preprocess functions.
(or is this explicitly allowed?)

And it makes it clear in the code where we have a complete or an incomplete theme hook object, because they have different types, instead of being the same object at different times with different states (which is a reoccurring problem in Drupal).

None of this is a BC break or API expansion.

But the test shows that hook_theme() will be allowed to return objects. So it is an API extension. Perhaps documented as @internal, but is this enough?

If we want piecemeal change instead, we could remove this again in hook_theme(), only accept arrays, and transform those into objects internally.
We could implement one side of my proposed split, and leave the other for a follow-up.
So, make class HookTheme suitable for being stored in the registry and being used in ThemeManager::render(), but not (yet) as return value for hook_theme(). This would make possible follow-ups less disruptive.

Or, even we want to split it up further, we could implement something where the theme registry continues to store arrays, which are converted to objects before using them in ThemeManager::render().

I think forcibly preventing use of ThemeHook would be a step too far. Also, how would we test what we have?

We would simply assume that all definitions returned from hook_theme() are arrays, just as we did in the past.
Or maybe add a sanity check, instead of just assuming.

Then turn every such item into an object: $theme_hook = ThemeHook::createFromLegacy($hook, $info);.
Note that I am using a different variable to hold the object.

Btw, currently the patch does not check for all cases. If we already check the type, maybe we should do a complete sanity check?

Current patch:

      $result = $function($cache, $type, $theme, $path);
      foreach ($result as $hook => $info) {
        // If there was no string $hook provided retrieve it from the ThemeHook.
        if ($info instanceof ThemeHook && is_int($hook)) {
          $hook = $info->getName();
        }

        // @todo Remove support for legacy array-based theme hooks in
        //   https://www.drupal.org/node/2873117.
        if (is_array($info)) {
          $info = ThemeHook::createFromLegacy($hook, $info);
        }

        [...]

        // Fill in the name, type, and path of the module, theme, or engine that
        // implements this theme function.
        $info->setProvider($name);

what if $info is neither an object nor an array?

      $result = $function($cache, $type, $theme, $path);
      foreach ($result as $hook => $info) {
        // If there was no string $hook provided retrieve it from the ThemeHook.
        if ($info instanceof ThemeHook) {
          if (is_int($hook)) {
            // @todo Shouldn't we always use $info->getName()?
            $hook = $info->getName();
          }
        }
        // @todo Remove support for legacy array-based theme hooks in
        //   https://www.drupal.org/node/2873117.
        elseif (is_array($info)) {
          $info = ThemeHook::createFromLegacy($hook, $info);
        }
        else {
          // @todo Log misbehaving hook_theme() implementation.
          continue;
        }

I think forcibly preventing use of ThemeHook would be a step too far. Also, how would we test what we have?

Because they are all converted, all tests of theming functionality would use those objects.

I also why we want to expose all the details of the former theme registry entry array as part of a public API?
Why not expose behavior instead?

E.g. ThemeManager::render(), we would then call $theme_hook->render($variables); (after applying all the suggestions logic). This would do everything internally: preprocess, and include the template or execute the theme function.

Or we could expose smaller chunks of behavior:
- $theme_hook->preprocess() would execute all preprocess functions.
- $theme_hook->generateOutput($variables) would call the theme function or include the template.

We could even split this last part into a separate class/interface, with distinct implementations for theme function and template engines.

All the public getters could go away if we do this.

Or maybe we do both things? Use the class proposed here for storage (although I am not so excited about object serialization), then build those other objects I am talking about based on this information?
If we do this, I would prefer if the class discussed here would be called ThemeRegistryEntry, so that the name ThemeHook is free for new things.

I think those behavior objects would bring us more structural improvement than objectifying the raw theme registry entries with getters and setters.

I created an issue for my proposed changes: #2954402: Refactor ThemeManager::render(), split into smaller immutable objects.
Work in progress.

Was going to set as RTBC, but its still tagged as needing a performance review.

Personally, I think any performance impact (which is negligible IMO due to caching) should not really be a concern for this issue.

Considering that this is just an internal/temporary solution and will be replaced with #2869859: [PP-1] Refactor theme hooks/registry into plugin managers, which has all the optimizations that come with plugins/managers.

Re #69:

Because of that issue (and other possible changes), this should remain as internal as possible. Therefore IMO no docs should be updated yet. It should be completely transparent.

I also think we should remove the change record. This isn't something that we really want to advertise as it's likely to go away in the future.

Something I missed to say in my previous comments.
The theme hook $info arrays are passed around to preprocess functions, which expect to see an array.

The ArrayAccess will be good enough for most of those implementations. But some array operations are not supported by ArrayAccess:
- If someone (like me) puts a native "array" type hint in front of the $info parameter of a preprocess function, it will break.
- If someone uses array addition operator (+, +=) on $info (which is a bad idea in preprocess functions).

So.. most preprocess functions will be ok.
Technically it is still a BC break for preprocess functions. It might be a small enough BC break to be acceptable when upgrading from 7.5.x to 7.6.x. But let's not pretend that it does not change the API.

@tim.plunkett, I'd like to set up another meeting to discuss this and other issues surrounding the theme system. Ping me in slack when you have a minute.

I'm wondering if we shouldn't postpone this on #2972143: Create \Drupal\Component\Utility\ArrayObject which this issue could greatly benefit from by extending ThemeHook from ArrayObject.

#95 I don't think we should block this issue on that, even though this could benefit from that.

Would be great if someone who know what is missing could update the remaining steps to the issue summary.

#94 patch needs reroll against 8.7.x

Fresh reroll according to current 8.7.x

Patch rerolled

Rerolled patch for 9.1.x, please review.

Please ignore patch #109, here is the rerolled patch for 9.1.x, please review.

Adding the test case fixing.

Thanks

Fixed test case and phpcs coding standard violation of core/lib/Drupal/Core/Theme/Registry.php

Last patch failed to apply , re-rolling the patch kindly review.

Re-rolling the patch for the latest branch

Rerolling based on #119

Rerolled #123 against 10.1.x

I have fixed CS error. Please review it

Reroll the patch #130 with Drupal 10.1.x .Please review it.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Starting with #129 the patch after re-roll became x2 smaller

Trying to fix to custom command failed issue of #132

Removing credit for #134, 131, 130, 129, 123, 122 as it is expected to check a patch before uploading. You can check for build errors make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

Hiding those patches as well.

This was previously tagged for issue summary update which still needs to happen.

Also as @andypost pointed out the patch got cut in half. How come?

Tried to rebase the MR. Needs performance testing still.

Needs performance testing still.

Just a quick note not for performance but memory/storage.

I was suspecting that these objects significantly increase the size of the registry when serialized in cache, due to all the unused properties.
But actually, uninitialized properties take zero space when serialized: https://3v4l.org/eoLeo

Therefore the increase in cache space is much smaller than I previously expected, at least for plain core.
For me, the string length in the `cache_default` table grows from ~83.000 to ~100.000 for theme_registry:olivero.