The current 'container.modules' parameter stores an array whose keys are module names and whose values are full namespace paths. In order to be fully useful as the de facto list of registered module namespaces, its keys should be the namespaces, not just the module names. And in order to be useful as the list of module paths, e.g. for use by the ExtensionHandler in #1331486: Move module_invoke_*() and friends to an Extensions class, it should not have the full namespace paths but just the module locations relative to the drupal root. I originally wrote a patch for this over in #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery but that is the wrong issue for this as they are focused specifically on a problem with registering plugin namespaces.
Anyway the change I made there also uncovered a bug in DrupalKernel.php where updateModules() adds the module path excluding [module_name].module as the uri property on the object it adds to the moduleData array, which is inconsistent and causes problems if we try to actually use it to register the namespace. That's why I'm categorizing this as a bug report.
The attached patch includes changes from dawehner too, so not sure what to do about that from a credit perspective :-/
Comment | File | Size | Author |
---|---|---|---|
#27 | 1849700.container.namespaces.27.patch | 10.88 KB | katbailey |
#20 | 1849700.container.namespaces.20.patch | 10.96 KB | effulgentsia |
#20 | interdiff.txt | 8.1 KB | effulgentsia |
#19 | 1849700.container.namespaces.19.patch | 9.53 KB | katbailey |
#13 | 1849700.container.namespaces.12.patch | 9.26 KB | katbailey |
Comments
Comment #1
sunTechnically, 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.
Comment #2
katbailey CreditAttribution: katbailey commentedThis takes #1 into account - see interdiff.
Comment #3
katbailey CreditAttribution: katbailey commentedOops, I had forgotten to change DrupalKernelInterface to have the $module_filenames param instead of $module_paths...
Comment #4
sunPSR-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?
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedSymfony'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.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedWould 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.
Comment #7
katbailey CreditAttribution: katbailey commentedYes, that makes sense, changed.
I also renamed the modulePaths property to moduleFilenames and updated the comment in registerBundles() to better reflect what's happening.
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? i.e. making the check and only setting it if it's not there or setting it regardless.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedPatch in #7 is for a different issue.
Comment #9
katbailey CreditAttribution: katbailey commentedDoh!
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedOnce 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.
Comment #11
sunSomehow 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"
Comment #12
effulgentsia CreditAttribution: effulgentsia commented#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.
Comment #13
katbailey CreditAttribution: katbailey commentedAttached patch fixes the typos.
The kernel's job is to manage the Dependency Injection Container and allow extensions to register services to it. That's pretty much all it does and all it knows how to do. We've had to override how it does certain parts of this because of the way Drupal's extension system works, but we haven't changed that it does this. And because it is concerned with extensions, it needs to know the locations of these extensions. That's really all there is to it - I'm not seeing where this is causing a problem.
Comment #14
katbailey CreditAttribution: katbailey commentedErm, probably no need to change that - the bot will still test it, right? And it was just typo fixes anyway...
Comment #15
katbailey CreditAttribution: katbailey commented#13 was a cross-post with #12 but I basically agree with what effulgentsia says.
Actually, this is the one thing about the kernel that I find really odd - and in fact from what I understand there is a push to change that and have the kernel + bundles system separate from the HttpKernel component, which makes more sense to me.
Comment #17
katbailey CreditAttribution: katbailey commented#13: 1849700.container.namespaces.12.patch queued for re-testing.
Comment #18
katbailey CreditAttribution: katbailey commentedBack to RTBC after a strange glitch...
Core committers: please be sure to include dawehner in the credits for this patch - its initial history is here #1836008-47: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery, comments 47 to 60.
Comment #19
katbailey CreditAttribution: katbailey commentedNeeded a reroll after #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled - interdiff not really possible. It'd be great if effulgentsia could double-check that it's still good to go and RTBC it again if it's green...
Comment #20
effulgentsia CreditAttribution: effulgentsia commented#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?
Comment #21
sun#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...
Comment #22
katbailey CreditAttribution: katbailey commentedActually, I'm ok with #20 as well - my original problem stemmed from the fact that we were storing the full namespace path for each module, i.e. starting with DRUPAL_ROOT and ending with /lib. And for the ExtensionHandler service, which will take this container parameter as an argument in its definition, we just want the module locations. But because there was discussion of a need for an actual list of namespaces over in #1836008: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery, I split it up into two distinct parameters.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedGiven 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.
Comment #24
katbailey CreditAttribution: katbailey commentedChanging the title to better reflect what the latest patch is doing.
Comment #25
katbailey CreditAttribution: katbailey commented#20: 1849700.container.namespaces.20.patch queued for re-testing.
Comment #27
katbailey CreditAttribution: katbailey commentedRebased... (I am hoping that last test failure was a glitch as it seems unrelated and passes locally for me)
Comment #28
katbailey CreditAttribution: katbailey commentedComment #29
katbailey CreditAttribution: katbailey commentedThis is blocking #1331486: Move module_invoke_*() and friends to an Extensions class.
Comment #30
catch@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.
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedI've created #1864292: Installation in non-English language fails which is probably related to this patch. Any help is appreciated.