Problem
-
The
system.module
configuration contains the list of installed modules, but System module is a module that needs to be installed on its own.Due to this circular dependency, the Drupal installer has to manually install System module in a very custom + fragile way.
-
Installation profiles are supposed to ship with default configuration, but they cannot ship with e.g. a
system.theme
file to configure the default theme + admin theme.That is, because the
system.module
+system.theme
configuration also contain the list of enabled modules and themes, and thus those files are force-overwritten by the Drupal installer.
Proposed solution
-
Merge and consolidate
system.module
+system.theme
+system.theme.disabled
into a newcore.extension
configuration file.This file is owned and maintained by
Drupal\Core\Extension
, hencecore.extension.yml
.The default configuration file + config schema file is located in:
/core/config/core.extension.yml /core/config/schema/core.extension.schema.yml
-
Change the Drupal installer to install System module... like any other module! :-)
-
Make
Config\ConfigInstaller
+Config\InstallStorage
+Config\ExtensionInstallStorage
+Config\Schema\SchemaStorage
aware that the core base system itself has default configuration to be installed.This only affects installation of default configuration.
The
core.extension
file has to be maintained in very special ways either way (especially when configuration is synchronized and imported); cf.#2187339: [META-0] CMI path to beta
#1808248: Add a separate module install/uninstall step to the config import process -
To limit the necessary changes throughout the system, make
drupal_get_path()
aware of a new file lookup "type", introducing'core'
as a new type/provider:drupal_get_path('core', 'core'); // yields 'core'
The file layout of Drupal core does follow the common pattern, as also implemented by modules:
core/config/schema core/config core/lib core/core.services.yml core/core.libraries.yml
Making "core" available as an actual provider location will open the door for simplifying a lot of code throughout core, which has to special-case the base system declarations currently.
In addition, developers familiar with Symfony's concept of Bundles will notice that Symfony treats the framework itself as a bundle, too. — Exactly for the same reason.
Note: This only affects
drupal_get_path()
. Core is not added as an Extension to the module list, since that is not necessary for this patch, but it could be discussed and investigated in a separate follow-up issue.
Comment | File | Size | Author |
---|---|---|---|
#21 | extension.core_.21.patch | 38.38 KB | sun |
#12 | interdiff.txt | 4.13 KB | sun |
#12 | extension.core_.12.patch | 38.38 KB | sun |
#10 | interdiff.txt | 20.84 KB | sun |
#10 | extension.core_.10.patch | 34.87 KB | sun |
Comments
Comment #1
sunHuh. That was much easier than I thought.
Comment #3
sunFixed invalid PHP syntax in ThemeHandlerTest.
Comment #4
dawehnerThis is really cool. I wonder whether we should really hardcode core like that or think about making "core" another extension type.
Yo dawg, I put drupal into drupal
I really like how this allows us to remove one config file.
Wouldn't it also work to just enable 'core' by default inside that config file?
This feels a little bit odd
Are there other places where we use $theme_config? Otherwise we should maybe rename that variable.
Comment #5
sunAdding a proper issue summary.
Comment #7
sunComment #9
catchThis would require an upgrade path, so promoting to beta blocker.
Comment #10
sunComment #12
sunThis should come back green.
Comment #13
jibranGreat work @sun. Keep up the good work I came here to review but I find nothing to complain about.
Comment #14
sunAs promised, latest patch is green. :-)
Actually, I think that this change is pretty simple and straightforward, so hopefully we can move forward as soon as possible.
Comment #15
jibranI don't get what the patch is doing so I am leaving it for others to review and RTBC.
Comment #16
dawehnerFrom my limited perspective it certainly makes sense to move that setting out of system module into core. The enabled extensions are the core of drupal.
Quick sidenote: It is odd that we track both enabled and disabled themes.
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedThough I don't know the code deeply I would speculate that keeping track of disabled themes makes the Appearance page's display logic simpler.
Comment #18
sunUnlike modules, themes still have a tri-state: enabled, disabled, uninstalled.
There no plans to change that for D8, because themes are operating at the end of all dependency chains, they do not have a database schema, and they also do not store data on their own, so the reasons that resulted in removing support for disabled modules do not apply.
The list of disabled themes is included in the move and merge into
core.extension
, because the configuration is owned and maintained byCore\Extension\ThemeHandler
— i.e., it is primarily a question of ownership and maintenance. But yes, it also allows to simplify the code inThemeHandler::enable()
and::disable()
.Once #1067408: Themes do not have an installation status is resolved (which is blocked by this patch), a Drupal production site is not expected to have any disabled themes, only enabled/installed + uninstalled. Just to preemptively address potential performance concerns of having that list of disabled themes in
core.extension
. ;-)Comment #19
webchickThis seems like it's alex-ish.
Comment #21
sunDiff context conflict.
Comment #22
sunAny chance to move forward here?
That would allow me to continue with #1067408: Themes do not have an installation status (we're very close!)
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedFWIW, RTBC+1.
Comment #24
alexpottCommitted 4511dc2 and pushed to 8.x. Thanks!
I've debated whether or not a change notice is necessary. I think better than change notices will be a set of documentation on how the 8.x extension system works.
Comment #26
jessebeach CreditAttribution: jessebeach commentedWe haven't changed the behavior of the system in such a way that would require devs to alter their modules' behavior. It seems pretty transparent and as you mention, more a topic for documentation.
Also, I want to raise a cheer to sun on this! It's a commendable patch that cleans up a sub system really nicely. Thank you!
sun++
Comment #27
sunYeah, I agree, we need to look into a revised + potentially combined change notice and/or documentation page for the new extension system at some point.
I think that's going to make most sense after some more tasks of the parent #2186491: [meta] D8 Extension System: Discovery/Listing/Info have been resolved + when some primary aspects of #2228093: Modernize theme initialization are done. Because at that point,
module.inc
will pretty much cease to exist, andsystem_list()
and friends will be gone — and that's going to be real point in time at which developers will need a summary of "WTF happened to the functions I used to use?" ;-)Lastly, FYI, the patch in #2184387: Remove SchemaStorage is green and nicely builds on this now — that issue is more or less where the idea for core to have config was born :-)
Comment #28
jessebeach CreditAttribution: jessebeach commentedWe will want a change notice to explain default them and admin theme specification in profile configuration. I'm planning to do that in a change notice for #1067408: Themes do not have an installation status. Does that seem like the right place?
Comment #29
sunYeah, that change is technically part of #1067408: Themes do not have an installation status — even though one could argue that it's not actually something special in any way; an install profile overrides
system.theme.yml
just like it overrides any other config file + that general "How to do an installation profile in D8" should (hopefully) be documented somewhere already ;-)Also, sorry, assigning back to me, as I'm tracking my weekly progress. ;)