Problem

  1. 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.

  2. 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

  1. Merge and consolidate system.module + system.theme + system.theme.disabled into a new core.extension configuration file.

    This file is owned and maintained by Drupal\Core\Extension, hence core.extension.yml.

    The default configuration file + config schema file is located in:

    /core/config/core.extension.yml
    /core/config/schema/core.extension.schema.yml
    
  2. Change the Drupal installer to install System module... like any other module! :-)

  3. 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

  4. 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.

Files: 
CommentFileSizeAuthor
#21 extension.core_.21.patch38.38 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,938 pass(es). View
#12 interdiff.txt4.13 KBsun
#12 extension.core_.12.patch38.38 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch extension.core_.12.patch. Unable to apply patch. See the log in the details link for more information. View
#10 interdiff.txt20.84 KBsun
#10 extension.core_.10.patch34.87 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,650 pass(es), 1 fail(s), and 82 exception(s). View
#7 interdiff.txt1.06 KBsun
#7 extension.core_.7.patch30.43 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,901 pass(es), 36 fail(s), and 95 exception(s). View
#3 extension.core_.3.patch29.63 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,869 pass(es), 39 fail(s), and 54 exception(s). View
#1 extension.core_.1.patch29.63 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php. View

Comments

sun’s picture

Status: Active » Needs review
FileSize
29.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php. View

Huh. That was much easier than I thought.

  1. Added core.extension config + schema.
  2. Converted all uses of system.module + system.theme + system.theme.disabled.
  3. Enable drupal_get_path('core', 'core') to make config schema + installer work.
  4. Account for an empty module list in DrupalKernel::initializeContainer().
sun’s picture

Status: Needs work » Needs review
FileSize
29.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,869 pass(es), 39 fail(s), and 54 exception(s). View

Fixed invalid PHP syntax in ThemeHandlerTest.

dawehner’s picture

This is really cool. I wonder whether we should really hardcode core like that or think about making "core" another extension type.

  1. +++ b/core/includes/bootstrap.inc
    @@ -575,6 +575,10 @@ function drupal_get_filename($type, $name, $filename = NULL) {
     
    +  if ($type === 'core') {
    +    return 'core/core.info.yml';
    +  }
    

    Yo dawg, I put drupal into drupal

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -94,8 +94,11 @@ public function installDefaultConfig($type, $name) {
    -      $enabled_extensions = array_keys((array) $this->configFactory->get('system.module')->get('enabled'));
    -      $enabled_extensions += array_keys((array) $this->configFactory->get('system.theme')->get('enabled'));
    ...
    +        $enabled_extensions += array_keys((array) $this->configFactory->get('core.extension')->get('module'));
    +        $enabled_extensions += array_keys((array) $this->configFactory->get('core.extension')->get('theme'));
    

    I really like how this allows us to remove one config file.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -94,8 +94,11 @@ public function installDefaultConfig($type, $name) {
    +      if ($type !== 'core') {
    

    Wouldn't it also work to just enable 'core' by default inside that config file?

  4. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaStorage.php
    @@ -80,7 +80,8 @@ protected function getAllFolders() {
       protected function getBaseDataTypeSchema() {
    ...
    +      'core.extension.schema' => 'core/config/schema',
    

    This feels a little bit odd

  5. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -127,8 +127,7 @@ public function __construct(ConfigFactoryInterface $config_factory, ModuleHandle
    +    $theme_config = $this->configFactory->get('core.extension');
    
    @@ -139,8 +138,10 @@ public function enable(array $theme_list) {
    +      $theme_config
    

    Are there other places where we use $theme_config? Otherwise we should maybe rename that variable.

sun’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
30.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,901 pass(es), 36 fail(s), and 95 exception(s). View
1.06 KB
  1. Removed stale system.module default config file.
  2. Fixed Stark theme must be enabled (fake-installed) by default until #1067408: Themes do not have an installation status
  3. Fixed Config FileStorageTest.
catch’s picture

Priority: Major » Critical
Issue tags: +beta blocker

This would require an upgrade path, so promoting to beta blocker.

sun’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
34.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,650 pass(es), 1 fail(s), and 82 exception(s). View
20.84 KB
  1. Removed obsolete getAllFolders() override in SchemaStorage.
  2. Added docs for $type 'core' to drupal_get_filename() + drupal_get_path().
  3. Clarified condition in ConfigInstaller::installDefaultConfig().
  4. Renamed local variables to $extension_config (and $extensions).
  5. Fixed core default configuration is not installed; only possible after rebooting installer kernel.
  6. Skip entire integration configuration processing code for $type 'core' in ConfigInstaller.
  7. Use injected config factory in ThemeHandler::disable().
sun’s picture

Status: Needs work » Needs review
FileSize
38.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch extension.core_.12.patch. Unable to apply patch. See the log in the details link for more information. View
4.13 KB
  1. Make system_list() also use ternary pattern.
  2. Updated update_fix_compatibility().
  3. Fixed DefaultConfigTest.

This should come back green.

jibran’s picture

Great work @sun. Keep up the good work I came here to review but I find nothing to complain about.

sun’s picture

Issue summary: View changes
Issue tags: +Configuration system

As 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.

jibran’s picture

I don't get what the patch is doing so I am leaving it for others to review and RTBC.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

From 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.

cosmicdreams’s picture

Though I don't know the code deeply I would speculate that keeping track of disabled themes makes the Appearance page's display logic simpler.

sun’s picture

Unlike 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 by Core\Extension\ThemeHandler — i.e., it is primarily a question of ownership and maintenance. But yes, it also allows to simplify the code in ThemeHandler::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. ;-)

webchick’s picture

Assigned: sun » alexpott

This seems like it's alex-ish.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: extension.core_.12.patch, failed testing.

sun’s picture

Assigned: alexpott » sun
Status: Needs work » Reviewed & tested by the community
FileSize
38.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,938 pass(es). View

Diff context conflict.

sun’s picture

Any chance to move forward here?

That would allow me to continue with #1067408: Themes do not have an installation status (we're very close!)

effulgentsia’s picture

FWIW, RTBC+1.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4511dc2 and pushed to 8.x. Thanks!

diff --git a/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
index a96aca9..a50a2d4 100644
--- a/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -143,7 +143,7 @@ protected function setUp() {
     $this->kernel = new DrupalKernel('unit_testing', drupal_classloader(), FALSE);
     $this->kernel->boot();
 
-    // Create a minimal system.module configuration object so that the list of
+    // Create a minimal core.extension configuration object so that the list of
     // enabled modules can be maintained allowing
     // \Drupal\Core\Config\ConfigInstaller::installDefaultConfig() to work.
     // Write directly to active storage to avoid early instantiation of

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.

  • Commit 4511dc2 on 8.x by alexpott:
    Issue #2228921 by sun: Consolidate system.module + system.theme + system...
jessebeach’s picture

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.

We 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++

sun’s picture

Yeah, 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, and system_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 :-)

jessebeach’s picture

Assigned: sun » Unassigned

We 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?

sun’s picture

Assigned: Unassigned » sun

Yeah, 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. ;)

Status: Fixed » Closed (fixed)

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