Problem

  • use Drupal\Core\Config\InstallStorage;
    
    /**
     * Defines the file storage controller for metadata files.
     */
    class SchemaStorage extends InstallStorage {
    

Details

  • The InstallStorage was explicitly designed to operate in the initial installer environment, in which the base system is not operable yet — it therefore performs plenty of filesystem scans (as opposed to relying on readily available application state information) and it also ignores the installation status of discovered extensions.

  • The InstallStorage was never intended to be used outside of the installer. In fact, as also clarified in #2155701: Installer starts with fatal error/exception "table 'semaphore' not found" if settings.php contains $databases already, the installer's minimal service container including InstallStorage is only used in the very first 2-3 installer screens until the base system has been set up and verified. As soon as System module and User module are installed, the installer uses a regular DrupalKernel with regular runtime services. Not even the installer uses Config\InstallStorage from that point anymore, because application state and extension information is readily available.

  • The use of InstallStorage by the SchemaStorage most likely causes a major slowdown in performance; at minimum in the installer (and consequently tests).

Files: 
CommentFileSizeAuthor
#11 2166703.11.patch23.74 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,216 pass(es). View
#11 9-11-interdiff.txt1.19 KBalexpott
#9 7-9-interdiff.txt8.14 KBalexpott
#9 2166703.9.patch23.56 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,276 pass(es). View
#7 2166703.7.patch17.02 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,100 pass(es). View
#7 5-7-interdiff.txt9.37 KBalexpott
#5 2166703.5.patch15.52 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,130 pass(es). View
#5 4-5-interdiff.txt408 bytesalexpott
#4 2166703.4.patch15.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 63,134 pass(es). View
#4 1-4-interdiff.txt6.16 KBalexpott
#1 2166703.1.patch6.22 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 63,044 pass(es), 13 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
Issue tags: +Configuration system
FileSize
6.22 KB
FAILED: [[SimpleTest]]: [MySQL] 63,044 pass(es), 13 fail(s), and 0 exception(s). View

The patch attached changes the SchemaStorage to extend from ExtensionInstallStorage instead which whilst still based on InstallStorage is limited to only enabled extensions.

This should result in quicker DrupalUnitTestBase tests and in general a smaller cache entry for schema definitions.

Status: Needs review » Needs work

The last submitted patch, 1: 2166703.1.patch, failed testing.

sun’s picture

The test failures are triggering two questions for me:

  1. When is SchemaStorage (or actually ExtensionInstallStorage::$folders) reset? → E.g., a ModuleHandler::install() must trigger a reset.
  2. None of the migrate_drupal tests are enabling/installing the module that they're migrating data for? → Their config schema does not exist. Is that a bug, or how is Migrate supposed to work?
alexpott’s picture

Status: Needs work » Needs review
FileSize
6.16 KB
15.55 KB
PASSED: [[SimpleTest]]: [MySQL] 63,134 pass(es). View
+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -82,6 +82,11 @@ public function installDefaultConfig($type, $name) {
+      if (is_dir($config_dir . '/schema')) {
+        // Refresh the schema cache if installing default configuration and the
+        // extension has a configuration schema directory.
+        $this->typedConfig->clearCachedDefinitions();
+      }

1. This clears the cache during the module install - schema's should never change during regular runtime. The ExtensionInstallStorage is only ever used in the ConfigInstaller and is not cached anywhere.
2. You're correct migrate_drupal tests are not enabling the module - I consider this a bug.

alexpott’s picture

FileSize
408 bytes
15.52 KB
PASSED: [[SimpleTest]]: [MySQL] 63,130 pass(es). View

Minor fix to EOF

Berdir’s picture

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateActionConfigsTest.php
@@ -16,6 +16,8 @@
 class MigrateActionConfigsTest extends MigrateDrupalTestBase {
 
...
  */
...
+  public static $modules = array('action');

We do have that stupid standard docblock for $modules...

I guess the system.module trick is for DUBT tests that don't enable system, or are there other reasons?

Confirmed that this does work and the time that DUBT tests spend for this goes down, they still have to parse the stuff in system.module, obviously.

alexpott’s picture

FileSize
9.37 KB
17.02 KB
PASSED: [[SimpleTest]]: [MySQL] 63,100 pass(es). View

The system module trick is because we write config before we truly have the system module enabled - see drupal_install_system() and yes also to ensure in DUBT that we have the minimum of config schema available.

There are many examples in core where this doc block is missing but here you go :)

sun’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -82,6 +82,11 @@ public function installDefaultConfig($type, $name) {
         if (is_dir($config_dir)) {
    +      if (is_dir($config_dir . '/schema')) {
    +        // Refresh the schema cache if installing default configuration and the
    +        // extension has a configuration schema directory.
    +        $this->typedConfig->clearCachedDefinitions();
    +      }
    

    I'm still concerned about two discrete things:

    1. The config.storage.schema service is not persisted, so every kernel/container rebuild will actually empty out the $folders property, since the whole class instance is replaced with a new one?

    ModuleHandler::install() + ::uninstall() are calling DrupalKernel::updateModules(), which rebuilds the container.

    → Manually clearing the cache seems to be superfluous?

    → In order to persist $folders across rebuilds, the property either has to be static, or the config.storage.schema service would have to be tagged with 'persist'?

    2. This may cover the module installation case, but the case of uninstalling a module is not covered?

    → Config schema definitions of a previously installed module are leaking into the next rebuild?

    3. Performance-wise, it would be a much better idea to specifically trigger a refresh of config schema information for a particular extension, instead of rescanning and rebuilding everything?

    → Shouldn't we replace this with explicit calls from ModuleHandler::install() + ::uninstall()?

  2. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaStorage.php
    @@ -56,4 +56,26 @@ public function rename($name, $new_name) {
    +  protected function getAllFolders() {
    +    if (empty($this->folders)) {
    +      parent::getAllFolders();
    

    Like parent::getAllFolders(), this condition should use !isset() rather than empty() for clarity and consistency, and to prevent it from being executed in unwanted situations.

  3. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaStorage.php
    @@ -56,4 +56,26 @@ public function rename($name, $new_name) {
    +      // Ensure that the system schema are available as the base types used by
    +      // all config schema are defined here.
    +      if (!isset($this->folders['system.schema'])) {
    +        $this->folders += $this->getComponentNames('module', array('system'));
    +      }
    

    I'm not sure I see the relation to DUTB, but if this specifically refers to the following lines in drupal_install_system():

      \Drupal::config('system.module')
        ->set('enabled.system', 0)
        ->save();
    
      // Update the module list to include it.
      \Drupal::moduleHandler()->setModuleList(array('system' => $system_path . '/system.module'));
    
      \Drupal::service('config.installer')->installDefaultConfig('module', 'system');
    

    ...then I wonder:

    1. Can we omit that enforced inclusion by simply swapping the order of setModuleList() vs. config() in drupal_install_system()?

    2. In case (1) does not work and we still have to keep this, then we should clarify this comment to clearly state why it is needed, including @see pointers + a @todo to resolve this circular dependency ASAP.

alexpott’s picture

FileSize
23.56 KB
PASSED: [[SimpleTest]]: [MySQL] 63,276 pass(es). View
8.14 KB
    1. This clears the cache in the database and static cache.
    2. Good point - fixed and tested in patch
    3. The cache is just one big blob and we have no clue which module provides what and there is no (automatic or standards based) namespacing
  1. Fixed in patch
  2. In DUBT the system module does not have to be enabled therefore the base schema types will not exist without this. So any writes to config by tests that do not enable system will be incorrect without this.
    1. This is a tangle of many knots - we need to read directly from the config.storage service to disentangle the knot between ConfigInstaller, ConfigFactory, ModuleHandler, and TypedConfigManager.

      ModuleHandler has to depend on ConfigInstaller
      ConfigInstaller has to depend on ConfigFactory and TypedConfigManager
      ConfigFactory has to depend on TypedConfigManager

      But doing this does not get round the fact that in order to write config with need the schema and the module list is held in config.

    2. Improved the comment and added a @todo to ExtensionInstallStorage to remove the procedural code.
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Overall, while the config schema code still looks very slow, fragile, and confusing to me, I think that this is certainly a step in the right direction.


Can we create a major follow-up issue to move the base configuration schema definitions from system.module into the Config\Schema component itself?

Let's simply add a /core/lib/Drupal/Core/Config/Schema/schema.yml file and force-include it manually, so we can get rid of the unnecessary dependency on system.module.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.19 KB
23.74 KB
PASSED: [[SimpleTest]]: [MySQL] 63,216 pass(es). View

Lets get the core data types issue fixed here :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Even better!

I secretly hoped that this would allow us to remove/revert some more workarounds, but ok...

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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