It is possible to validate configuration using schema prior to importing. This will make the configuration import more robust by doing a pre flight check that what is being provided is sane.

Files: 
CommentFileSizeAuthor
#6 2414951.6.patch11.66 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,864 pass(es). View
#1 2414951.1.patch6.97 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,856 pass(es), 0 fail(s), and 1 exception(s). View

Comments

alexpott’s picture

Parent issue: » #1842956: [Meta] Implement event listeners to validate imports
Related issues: +#2414953: Element uses \Drupal::service() as a service locator and prevents injecting a custom typed config manager in config
FileSize
6.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,856 pass(es), 0 fail(s), and 1 exception(s). View

Let's see what breaks. I've created #2414953: Element uses \Drupal::service() as a service locator and prevents injecting a custom typed config manager in config to handle something we need to do even if we decide not to do this.

alexpott’s picture

Status: Active » Needs review
alexpott’s picture

Also to make this performant we really need to get #2411689: Use a MemoryBackend in StorageComparer so that configuration import validators don't have to reread data from disk or the db done. Since without that we'll be reading all the configuration many many times.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I think this is a great idea.

  1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
    @@ -295,4 +296,21 @@ public function hasConfigSchema($name) {
    +  protected function alterDefinitions(&$definitions) {
    +    $existing = array_keys($definitions);
    +    parent::alterDefinitions($definitions); // TODO: Change the autogenerated stub
    +    if ($existing !== array_keys($definitions)) {
    +      // Throw an exception.
    +    }
    +  }
    
    +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -224,9 +224,7 @@ protected function findDefinitions() {
    -    if ($this->alterHook) {
    -      $this->moduleHandler->alter($this->alterHook, $definitions);
    -    }
    +    $this->alterDefinitions($definitions);
    
    @@ -242,4 +240,16 @@ protected function findDefinitions() {
    +  /**
    +   * Invokes the alter hook if set.
    +   *
    +   * @param $definitions
    +   *   The array of definitions found during discovery.
    +   */
    +  protected function alterDefinitions(&$definitions) {
    +    if ($this->alterHook) {
    +      $this->moduleHandler->alter($this->alterHook, $definitions);
    +    }
    +  }
    +
    

    This is great, but would be a different issue, no?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -35,6 +48,26 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +          if ($errors === FALSE) {
    +            $event->getConfigImporter()
    +                  ->logError($this->t('No schema for !config_name', array('!config_name' => $name)));
    +          }
    

    If we want to keep the schema optional, we should not make this condition an error.

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

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
11.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,864 pass(es). View

Rebased the patch on the latest in #2414953: Element uses \Drupal::service() as a service locator and prevents injecting a custom typed config manager in config and refactored the event to make it as easy as possible to read. Also surprise surprise we found some invalid config lying about :).

Re #4.1 yep - going to create a new issue.
Re #4.2 yep - fixed.

We need to add some tests of schema errors being caught. The ConfigImportAll test is proof that schema validation for uninstalled modules is working nicely.

Gábor Hojtsy’s picture

I think its key to have a module installed in that test.

Berdir’s picture

So what exactly happens if an error is logged? Does it still import or is it aborted?

Enforcing that only valid configuration can be imported seems like a huge change to HEAD, there are still modules that don't have valid configuration schema.

alexpott’s picture

I talked about this with @Berdir in IRC - he's in favour of moving this to contrib since partial or wrong schemas are common in contrib. One problem with this is that how will they ever get fixed?

Gábor Hojtsy’s picture

I hope translators will put some pressure on them (oh, did I write that?) I mean they will help fix things.

Berdir’s picture

One option would be some kind of warnings, that aren't blocking the import. (a collapsed fieldset that lists the first few, or something).

We have tests that validate it by default, and I think the test coverage for frequently used contrib modules will be much better in 8.x than it was in earlier versions.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anavarre’s picture

Version: 8.3.x-dev » 8.4.x-dev