Config SchemaStorage uses InstallStorage

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?

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.

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.

Even better!

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