Convert system_theme to OOP or remove it

The current issue seems to be that for some reason $this->moduleList->getPath('system') does not return the path to system, so when the preprocess hook process runs it doesn't have the path to the templates.

So then later on when they are diffed $diff = array_diff($parent_hook['preprocess functions'], $cache[$source_hook_name]['preprocess functions']); it fails on container since the templates have not been crawled.

At least this is what I think is happening.

Let me know if this requires a CR.

If we do not want to remove the system theme hook entirely then we will likely need to special case the install and load what is needed for the installer to work.

It might be more desirable to approach it this way cause then system_theme stays the same but it's OOP.

The issue is two fold.

1. The class is not loaded during install so the OOP hook will not be there. The procedural hook is still picked up in that situation though.
2. The RegistryTest does some funky stuff and Drupal is not fully there during that test.

What is in this pr bypasses those two issues, but we no longer have a system_theme hook implementation at all and we're loading the templates somewhat manually.

Let me test something

nicxvan → changed the visibility of the branch 3488176-convert-systemtheme-to to hidden.

Refactoring a bit

I've refactored this to be closer to the way it originally worked.

The one change is now the installer loads only the minimal needed to work.

Some comments.

Addressed your feedback, left a comment on one.

Thinking about your feedback on:

  if (!defined('MARK_READ')) {

vs

  if (!in_array(\DRUPAL_ROOT . '/core/includes/theme.inc', get_included_files())) {

This check is needed for just one test where preprocess functions are not loaded yet.

This whole stanza only gets hit during site install so there are no performance implications there.
However during tests every time drupal is installed we'll hit this and have to check.

It's a small thing, but I think checking the const is probably less intensive than the get_includes check. They both accomplish the same thing. Is it worth changing that back?

How about this? In theme.inc, add

// {Add comment documenting why this is here}.
const THEME_INC_INCLUDED = TRUE;

Then do if (!defined('THEME_INC_INCLUDED')) {.

Done

lgtm

Removed the Theme inc load check entirely. Still passing so leaving in RTBC.

There are a couple unresolved comments on the MR from @catch.

Tests are green, all comments resolved, back to RTBC.

catch → closed merge request !10235

Looks great now! Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

nicxvan → closed merge request !10229

Automatically closed - issue fixed for 2 weeks with no activity.