Extension System, Part IV: Properly register all installed extensions/namespaces during container generation

Sure, that makes sense.

Hopefully #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList will make its way in soon and then I'll start tackling this.

Depending on the current request you can have different kind of active themes.

Correct, but this is becoming less and less of an “issue” due to the OO advancements we’re implementing.

Given that we cannot just make the namespaces available on boot-time.

Yes we can.

Code might do things conditionally whether a specific theme is loaded or not.

This issue isn’t about actually loading any classes. That is well out of scope. It’s about just ensuring that their namespaces are registered in the autoloader so, when they are needed, they can be found.

This point is also the reason why themes shouldn't have a .services.yml file

No, the reason themes shouldn’t have services is because then they’d just be modules (and because themes weren’t installable before).

I'm curios, what blocks us from expanding \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme to register the theme and all its parents namespaces?

Because, as the IS states, this is too late in the process and, quite frankly, overkill. We shouldn’t have to “boot up the entire theme system” just so namespaces can be registered.

The main reason this needs to be done in the kernel is because certain requests only bootstrap so far. If a theme provides a static #pre_render, #ajax, or #lazy_builder callback to a method in one of its classes, this lives in a render array which can become cached. Because of this caching, the entire theme system does not neccessarily need to be loaded just to execute a callback. Thus, the namespaces are never registered and the classes cannot be found.

Please put that into the issue summary so everyone can quickly see the problem.

Pretty sure it is implied, but I suppose it needs to be stated.

So we have to somehow autoload the theme(s) to which the cache belongs so that it can render to HTML, but we also want to remain performant by not bootstrapping resources we don't need for this to occur.

No. This issue isn't about bootstrapping/loading anything, let alone the theme system (that's way out of scope).

In this case the issue should rescope to: Register autoloading for extensions early in boot process.

That's what this issue already is: "Properly register installed extension namespaces in container"

Cleaned up IS to remove verbose project specific info.

Need to look at #2021959-33: Refactor module handling responsibilities out of DrupalKernel to see what can be salvaged from that issue/patch (similar goal to this issue).

This is a catch-22.

To get ExtensionHandler working requires installed extensions registered with the container, to get registered extensions requires ExtensionList/ExtensionHandler.

Both this issue and #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface must happen at the same time.

Turns out we can do this is steps after all.

Ok... an initial stab at this.

Doesn't include any new tests yet... but I think it should pass all existing tests (had to modify a couple existing ones slightly).

Guess the installed extensions instance really needs to be added in the constructor. Had some worries about the class loader not yet being initialized (properly), but it seems to be working ok.

Ok... let's try this.

Sigh...

Almost there (I think)...

Ok, I think this is finally it. At least it should fix the majority of all the "major" test failures. There may still be a few left.

Awesome! There's only a few left. Maybe someone else wants to take a look and see what I may have missed.

Had to do a lot of local debugging to sort through some of this mess. Just FYI, the kernel/extensions are extremely fragile... hence why this is a great first step. Ultimately I think this should just be a wrapper around ExtensionList instances, but we're not there yet.

Anyway, I'm hoping this fixes the remaining tests... 🤞

#49.1: This method already exists and is public. I really don't like this method and want to deprecate it and replace it with something better, but I think that may be out of scope for now. My goal is to simply get this working.

#49.2: Same as above, this already existed. I'm not entirely sure this is the most appropriate issue to tackle removing this.

---

On a side note: I've been doing even more local testing to try and figure out what's going on with the last remaining failures.

The best I can guess is that somehow normal procedural functions/constants (that aren't namespaced) can't be found.

In fact, I did a little searching and found #1912474: Unable to pre-populate db in settings.php.

That issue describes almost verbatim the same error messages I'm running into here now.

First I get:

Exception: Warning: Use of undefined constant USERNAME_MAX_LENGTH - assumed 'USERNAME_MAX_LENGTH' (this will throw an Error in a future version of PHP)
Drupal\Core\Installer\Form\SiteConfigureForm->buildForm()() (Line: 178)

Found #3015752: Properly deprecate USERNAME_MAX_LENGTH and USER_REGISTER_* constants, but then I run into:

Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'user_form_process_password_confirm' not found or invalid function name

That issue was closed as a dup of #2155701: Installer starts with fatal error/exception "table 'semaphore' not found" if settings.php contains $databases already, but I fail to see what exactly it did to rectify the warnings.

Could use some help here if someone is more familiar with \Drupal\Core\Installer\InstallerKernel and \Drupal\Core\Test\TestRunnerKernel.

---

Attaching patch of where I left off of solving a minor bug with cross profile modules and duplicate namespace registration.

It seems any pre-emptive extension setting must happen in initializeContainer.

I still cannot, for the life of me, figure out the errors in #50.

Any direction would be helpful.

---

edit: this includes a change/addition of InstallerKernel::isConfigEmpty since it really shouldn't be changing the scope of getConfigStorage to public when it's just used in one place.

Ok, I think I may actually have figured out the issues; at least I was able to get \Drupal\FunctionalTests\Installer\StandardInstallerTest::testInstaller passing locally anyway.

+++ b/core/lib/Drupal/Core/Extension/InstalledExtensions.php
@@ -0,0 +1,676 @@
+      $profiles = array_intersect_key($allProfiles, array_flip($this->getConfigExtensions()));

I think this was the main issue. The original code intersected $this->module_list, which is the currently set "install" list... not what is stored in config.

+++ b/core/lib/Drupal/Core/Extension/InstalledExtensions.php
@@ -0,0 +1,676 @@
+    // If installed extensions is not yet set, parse it from config.
+    if ($empty && ($extensions = $this->getConfigExtensions())) {
+      $this->add($extensions);
+    }

I also moved said "initial list" from config back to DrupalKernel, which I think had another small part in the failures.

I'll get started on the new tests if/when this passes the existing ones (with some minor modifications).

🤞

Awesome! This patch should now pass 😁

I really hate run-tests.sh...

w00t!!!! 🎉

Here are the initial tests. Still needs a bit more work so I'll reassign this issue to myself as I'll likely be working on them this week.

Ok, this should complete the necessary tests. Patch is officially "done", barring review(s).

Ugh, c&p fail...

Reading up the issue summary it seems to have an assumption that themes need to have complex logic, written in an OOP style.

I don't really see how describing a legitimate bug is a generalization/assumption for "all themes". In fact, the IS describes a very advanced use case where callbacks are cached in a render array. Advanced or not, it's still an issue.

Given that the overall goal is to move everything to OO, themes should be no exception.

Themes don't "need to have" complex logic, but when they do (especially when dealing with an external framework like Bootstrap)... the system should be able to handle it.

I wonder though why themes can't use a corresponding module instead and put the logic in there.

#474684: Allow themes to declare dependencies on modules

I'm still baffled by this way of thinking though. If this were the case, why have themes at all? Why couldn't we just put everything in a module?

The average [sub-]theme will likely not be as complex, but base themes like Drupal Bootstrap are.

Also, what type of "logic" we're referring to here? Complex or not, presentation logic should still live within a theme, not a module. This presentation logic can still be cached in a render array referencing a theme class callback.

Ultimately, I'd like to see #2869859: [PP-1] Refactor theme hooks/registry into plugin managers make its way in which would allow [base-]themes the ability to properly intercept the entire discovery (registry) and rendering process without the need of any "additional module" or antiquated theme special "hook/alter".

Just because things have been the way they have been for so long doesn't mean it has to stay that way.

Themes are this weird system as they are loaded dynamically. Putting them into the container might create the assumption they should be autoloadable at any given point in time, but as you know, that's not true. If they aren't the active theme, they cannot be loaded.

That's an extremely heavy burden to put on the kernel.

Does the kernel know which blocks should be added to a page?

No. This kind of logic should be left to whatever mechanism is in place that is responsible for determining this kind of logic.

We don't know when or where a theme may be used (at this step), but we do know that it is installed.

Installed extension namespaces should be registered with the autoloader, plain and simple.

It's up to other subsystems (i.e. elements, render api, render cache, theme registry, etc.) to ensure that if their logic depends on which "active" theme that they build necessary caches based on said "active" theme; which they already do and we have tests for.

I'm confused about this [InstalledExtensions]. Isn't that the same as the Extension list conceptually?

Yes/no.

I perhaps should have explained a little more in #24 and #26.

This code is literally just putting what was already in DrupalKernel into a separate service/class.

Currently ExtensionList has a dependency on ModuleHandler. There are also a lot of other services ExtensionList currently relies on.

I originally went down the path of "let's just use ExtensionList", but given the required dependencies and circular references it would create, I decided to create a separate encapsulating service/class was a needed first step in this process.

That is why I marked InstalledExtensions as @internal. Ultimately, I don't see this class sticking around. We'll probably remove it in #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface or if we don't clean up the ExtensionList during that issue, an issue shortly after that.

I didn't want to create a massive patch that no one could review. That's why I created InstalledExtensions as a way to allow us to make this change and introduce tests without breaking all of core (which you can see above, is very easy to do at this level).

After speaking with @alexpott in slack, I think that ultimately this entire process in the kernel will become a ServiceProvider decoration of the container and we can just let the various ExtensionList services handle it.

Until we decouple ExtensionList from ModuleHandler though, this step is needed.

Themes are this weird system as they are loaded dynamically. Putting them into the container might create the assumption they should be autoloadable at any given point in time, but as you know, that's not true. If they aren't the active theme, they cannot be loaded.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -113,11 +113,6 @@ public function listInfo() {
   public function addTheme(Extension $theme) {
-    // Register the namespaces of installed themes.
-    // @todo Implement proper theme registration
-    // https://www.drupal.org/project/drupal/issues/2941757
-    \Drupal::service('class_loader')->addPsr4('Drupal\\' . $theme->getName() . '\\', $this->root . '/' . $theme->getPath() . '/src');

This was already registering all installed themes; not just the "active" theme. However, this is too late in the process and requires the theme system to be initialized which may not happen during a slimmed down bootstrap (hence this issue).

Bump...

That issue is a byproduct of that module doing some weird namespacing hackery (that doesn't follow PSR-4 btw):
https://git.drupalcode.org/project/jsonapi_extras/blob/8.x-3.x/src/Jsona...

It doesn't really have anything to do with this issue.

Bump... again

While I understand the end goal/desire of #75, wouldn't it make more sense to create a follow-up to do that?

If this issue is any indication (#27-#63), this is a complicated area and doing this in steps would probably be best.