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

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?

The entire ThemeHook.php was dropped from the patches. Working on this.

Switched to an MR.
This will probably break things, leaving assigned for now.

Fixed a bunch of credit.

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.

It's getting bigger just because of namespaced class but as I see it has \0 code so sqlite truncating it

/var/www/html/web $ drush sqlq "select * from cache_default where cid like 'theme_registry%';"
theme_registry:build:modules|a:147:{s:26:"big_pipe_interface_preview";O:27:"Drupal\Core\Theme\ThemeHook":7:{s:7:"|-1|1690406986.891|1||0
theme_registry:claro|a:177:{s:26:"big_pipe_interface_preview";O:27:"Drupal\Core\Theme\ThemeHook":7:{s:7:"|-1|1690406986.915|1||0
theme_registry:olivero|a:181:{s:26:"big_pipe_interface_preview";O:27:"Drupal\Core\Theme\ThemeHook":7:{s:7:"|-1|1690406989.703|1||0

my numbers

/var/www/html/web $ drush sql:query "select cid, length(cast(data as blob)) from cache_default where cid like 'theme_registry%';"
theme_registry:build:modules|74958
theme_registry:claro|99383
theme_registry:olivero|99111

/var/www/html/web $ drush sql:query 'delete from cache_default;'

... removing patch

/var/www/html/web $ drush sql:query "select cid, length(cast(data as blob)) from cache_default where cid like 'theme_registry%';"
theme_registry:build:modules|61296
theme_registry:claro|82337
theme_registry:olivero|82029

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased

Seems rebase had some errors.

This issue seems to have stalled out. Reading through and I get a feeling that it might be close to ready, but is in dire need of a manual rebase as #146 indicate. On the performance profiling, should not our Gander setup now be able to tell us if we have regressions based on changed to the registry size?

Just like alexpott in #63 I am not too keen on the name ThemeHook.

What about ThemeableTemplate?

Good point but it's really about implementation so TemplateImplementation or TemplateDefinition

Rebased and fixed CS/phpstan but lots of tests still fail

Is this issue only about replacing what hook theme returns?
Or is this about how to define preprocess hooks and hooks implemented by themes?

If this is the latter has anyone looked at 📌 OOP hooks using event dispatcher Needs review , a somewhat consistent way to implement these would be great, there could also be some confusion calling this ThemeHook since hook_theme will likely be placed in src/Hook/ThemeHook.php but return a different ThemeHook class.

Though I'm not sure what you would call it, maybe it's ok to overload it a bit.

There are some related thoughts on preprocess hooks here too 📌 [PP-1] Explore converting Install hooks to OOP Active in Followup 2. If this isn't the place let me know.

Finally can someone add some before / after examples to the IS, it would make it easier to review the MR.

To answer my own question it is about the return of hook theme.

I prefer TemplateDefinition over TemplateImplementation