DrupalKernel's container.modules param should contain module filenames, not full namespace paths

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -256,7 +270,7 @@ protected function moduleData($module) {
   public function updateModules(array $module_list, array $module_paths = array()) {
...
     foreach ($module_paths as $module => $path) {
-      $this->moduleData[$module] = (object) array('uri' => $path);
+      $this->moduleData[$module] = (object) array('uri' => $path . "/{$module}.module");

Technically, all of our callers only have a module's full filename/uri and not the directory name. The entire moduleData[]->uri construct stems from the fact that the data is essentially derived from a filesystem scan for extensions, which delivers filenames, not paths, and that's how we "register" extensions literally everywhere else.

So I think we should hunt down the callers that are passing a bogus directory/path instead of a filename, and adjust those to pass the filename(s) instead.

Ideally, we'd additionally clarify that by renaming the $module_paths argument to $module_filenames or $module_files.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -318,13 +332,10 @@ protected function initializeContainer() {
-      $namespaces = $this->classLoader->getNamespaces();
-      foreach ($this->container->getParameter('container.modules') as $module => $path) {
-        $namespace = 'Drupal\\' . $module;
-        if (!isset($namespaces[$namespace])) {
-          $this->classLoader->registerNamespace($namespace, $path);
-        }
+      foreach ($this->container->getParameter('container.namespaces') as $namespace => $path) {
+        $this->classLoader->registerNamespace($namespace, $path);
       }

PSR-0 class loaders actually support multiple filesystem locations for a single namespace, so I think the old code needs to be kept in order to not double-register a module's path with its namespace?

in order to not double-register a module's path with its namespace

Symfony's class loader doesn't double register. registerNamespace() entirely clobbers whatever else was there previously for that namespace. You can pass an array as the second argument if you want multiple locations for that namespace, but even if you do, that array replaces rather than merges with what was there previously.

So the question of whether or not we want the isset() is more about whether we want to avoid clobbering what's already there, or if we do in fact want to replace what was there previously with what the kernel believes should be there.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -179,27 +191,28 @@ public function registerBundles() {
-        $this->modulePaths[$module] = $namespaces[$namespace];
+        $full_path = dirname(DRUPAL_ROOT . '/' . $path) . '/lib';
+        $this->classLoader->registerNamespace($namespace, $full_path);
+        $this->moduleNamespaces[$namespace] = $full_path;
...
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -355,6 +366,7 @@ protected function buildContainer() {
+    $container->setParameter('container.namespaces', $this->moduleNamespaces);

Would it be possible to not include DRUPAL_ROOT in the parameter? Having it there prohibits the possibility of precompiled containers that are independent of server environment.

Patch in #7 is for a different issue.

What the kernel thinks should be there will definitely be correct, so I guess it's now a question of what is more performant, right?

Once we remove calls to system_register(), the isset() check would not return true within anything that's performance critical, so it should make no difference. But, depending on which class loader we end up using (e.g., #1818628: Use Composer's optimized ClassLoader for Core/Component classes), there might be some micro-optimizations specific to that loader that we can employ. No point in micro-optimizing now to the wrong loader.

It's more important at this point that we express the logic that we want, and if we're designing the kernel such that it must trump anything that might have been registered outside of it (and that seems a reasonable design choice to me), then this patch looks right, so RTBC.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -179,27 +191,29 @@ public function registerBundles() {
-      // When installing new modules, the modules in the list passed to
-      // updateModules() do not yet have their namespace registered.
...
+      // Enurse all module namespaces are registered and store the list of
+      // namespaces in $this->moduleNamespaces for retrieval when setting the
+      // container.namespaces parameter.
+      if ($this->moduleData($module)) {
...
+        $namepace_path = dirname($filename) . '/lib';
+        $this->classLoader->registerNamespace($namespace, DRUPAL_ROOT . '/' . $namepace_path);
+        $this->moduleNamespaces[$namespace] = $namepace_path;
       }

Somehow I've the impression that we're overloading the kernel with functionality that it shouldn't contain...

Technically, the original comment already hints at the fact that the code shouldn't have been added to the kernel, but the necessary logic and update should have been performed by module_enable() instead, which is responsible for properly registering a new module/extension.

I still don't really know and understand what exact job "the kernel" tries to perform and what it actually buys us. So far, I haven't seen a single reason or use-case for which the kernel would actually do something useful instead of causing havoc and harm down the line. We had to override and disable it's initialization because it entirely broke our error handling, we had to replace how it handles the container, we have to replace how it handles bundles, we have to implant non-existing logic for configurable extensions and varying namespaces, and we even have to replace how the kernel's container is dumped. So, erm... What, exact, functionality is actually in there that is actually useful?

I can totally get behind the new Request and Response objects, and all the other services we have. But so far, the kernel is a total red herring to me.

The fact that we're overloading and turning the kernel into half of an extensions service with fixes/changes like this is the exact reason for why I started to perceive and call it a God object in various issues. Obviously, that's definitely an architecture we better avoid.

If someone is able to clarify that (dare I say I doubt that?), that would be highly appreciated. Even worth a documentation page in the spotlight, if you'd ask me (and if the explanation makes sense ;)).

Regardless of the above, two typos here:

1) "Enurse" vs. "Ensure"

2) "namepace_path" vs. "namespace_path"

#11 raises good questions about what the purpose of DrupalKernel is, but I think they're all outside the scope of this issue. This issue is just taking an existing variable and splitting it into two for reasons stated in the issue summary.

As to the purpose of DrupalKernel, all I can make sense about it at this point is that it manages bootstrapping the DIC. By virtue of extending Symfony's Kernel, it also implements handle() and terminate(), though it just forwards that to HttpKernel, a separate object. So, really, I still consider it just a DIC bootstrapper.

However, in order to boot the DIC (and reboot it as modules are enabled/disabled), we need to interact with the module list and the class loader. Whether we're currently doing that in the most sane way is up for discussion, but let's have that discussion in another issue.

#19 looks good. But, I thought a bit more about this issue, and am wondering why we want container.namespaces as a separate container parameter, rather than letting the kernel privately derive it. Here's what I mean. Is this ok?

#20 looks technically good, but I suspect that @katbailey had some Extensions service implications in mind?

Also, thanks for the clarifications - and to give an update on #11:

Meanwhile, I'm wondering whether "the extensions service" isn't or shouldn't be identical to "the kernel"; i.e., one and the same thing.

One could potentially label that as ConfigurableKernel or similar and push it back upstream to Symfony...

Given sun's and katbailey's okay, I think this is RTBC. Once this is in, we can continue discussing in #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery how AnnotatedClassDiscovery should get the namespaces, whether by container parameter, by adding a public kernel method, or something else.

@sun's #11, this comes back to #1599108: Allow modules to register services and subscriber services (events) and whether that was a good idea at all in the first place.

I'm not sure it was, but it's getting a bit late to roll back, so we should at least try to make the dependency of everything everywhere on the module system technically cleaner even if it conceptually still feels wrong, and this patch looks great for that.

So committed/pushed to 8.x.

I've created #1864292: Installation in non-English language fails which is probably related to this patch. Any help is appreciated.