We need to be able to strict apply schema during configuration saves in particular tests.

This patch adds the ability to set a property on a test to enable this. It adds the property to TestBase and allows any KernelTestBase or WebTestBase to set this mode.

  /**
   * Set to TRUE to strict check all configuration saved.
   *
   * @see \Drupal\Core\Config\Testing\ConfigSchemaChecker
   *
   * @var bool
   */
  protected $strictConfigSchema = FALSE;

Once enabled any configuration save that does not adhere to configuration schema will throw an exception.

This will help test issues like #2368349: Entity view and form display configuration schemas are too verbose / key ones missing

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
11.99 KB

Here's an implementation. We have to put the event listener in core since a test might not have simpletest or config enabled.

effulgentsia’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
    @@ -0,0 +1,94 @@
    +   * Removes stale static cache entries when configuration is saved.
    

    That's not the main thing this method does. Also, could use an @throws.

  2. +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
    @@ -0,0 +1,94 @@
    +    // Check that we have a valid for all config saved during tests. Do not
    

    A valid what?

  3. +++ b/core/modules/config/src/Tests/SchemaConfigListenerTest.php
    @@ -0,0 +1,70 @@
    +  public function testTrait() {
    

    Does this test have anything to do with traits? Same for the web test.

  4. +++ b/core/modules/config/src/Tests/SchemaConfigListenerTest.php
    @@ -0,0 +1,70 @@
    +    // Test a non existing schema.
    

    Nice comment. Could use one for the two other code blocks. Same for the web test.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
12.14 KB

@effulgentsia thank you the review

re #2

  1. Fixed
  2. Removed - it is covered by the method's doc block.
  3. Fixed
  4. Fixed
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Wim Leers’s picture

Why not do this on every config save, rather than only during tests? That'd greatly improve DX. Is this really so slow? Do we have so many config writes?

Wim Leers’s picture

Only found silly nitpicks in review.

  1. +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
    @@ -0,0 +1,93 @@
    +   * @param ConfigCrudEvent $event
    

    Namespace missing.

  2. +++ b/core/modules/config/src/Tests/SchemaConfigListenerTest.php
    @@ -0,0 +1,72 @@
    +    // Test a non existing schema.
    
    +++ b/core/modules/config/src/Tests/SchemaConfigListenerWebTest.php
    @@ -0,0 +1,77 @@
    +    // Test a non existing schema.
    

    s/non existing/non-existing/

  3. +++ b/core/modules/config/src/Tests/SchemaConfigListenerWebTest.php
    @@ -0,0 +1,77 @@
    +    $msg = 'Expected SchemaIncompleteException thrown';
    

    I don't know of Drupal using such shorthand variable names anywhere else.

  4. +++ b/core/modules/config/src/Tests/SchemaConfigListenerWebTest.php
    @@ -0,0 +1,77 @@
    +      $this->pass('Schema errors for config_test.types with the following errors: config_test.types:foo: Missing schema., config_test.types:array: Variable type is integer but applied schema class is Drupal\Core\Config\Schema\Sequence.');
    

    <message>., <next> looks weird (period followed by comma).

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
9.19 KB
16.4 KB
  1. Fixed
  2. Fixed
  3. well $msg_type is used in update.module but changed to $message so fixed
  4. Fixed

Also I realised one of the tests was wrong.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php
index 730f84f..6792b6f 100644
--- a/core/modules/simpletest/src/WebTestBase.php
+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -816,6 +816,16 @@ protected function setUp() {
       // testing specific overrides
       file_put_contents($directory . '/settings.php', "\n\$test_class = '" . get_class($this) ."';\n" . 'include DRUPAL_ROOT . \'/\' . $site_path . \'/settings.testing.php\';' ."\n", FILE_APPEND);
     }
+    if ($this->strictConfigSchema) {
+      $yaml = new \Symfony\Component\Yaml\Yaml();
+      $services = $yaml->parse($directory . '/services.yml');
+      $services['services']['simpletest.config_schema_checker'] = [
+        'class' => 'Drupal\Core\Config\Testing\ConfigSchemaChecker',
+        'arguments' => ['@config.typed'],
+        'tags' => [['name' => 'event_subscriber']]
+      ];
+      file_put_contents($directory . '/services.yml', $yaml->dump($services));
+    }
     $settings_services_file = DRUPAL_ROOT . '/' . $this->originalSite . '/testing.services.yml';
     if (file_exists($settings_services_file)) {
       // Copy the testing-specific service overrides in place.

Sorry for coming so late to this, but this breaks the testing.services.yml support introduced in #2229011: Tests are no longer modifiable. Specifically, the line that comes directly after the cutoff in the hunk above, copies the testing services file (if it exists) to $directory . '/services.yml', thus, it conflicts with the addition here. Both should be possible to have at the same time.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
16.38 KB

@tstoeckler yep nice spot! Just moving the adding of the service should work without issue.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, that's very neat. I hadn't thought of that.

Wim Leers’s picture

What about #5?

Why not do this on every config save, rather than only during tests? That'd greatly improve DX. Is this really so slow? Do we have so many config writes?

Follow-up, because we don't want to block this issue on that discussion?

alexpott’s picture

@wim yep I will explore turning it on always in the general config schema issue. atm doing so would require us to fix all the schema in this issue.

Wim Leers’s picture

D'oh, right, I think I've been told this in IRC already. Very much looking forward to getting instant feedback when a config entity (or config schema!) is wrong! :) That'll be a great DX improvement.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 121bbc1 on 8.0.x
    Issue #2371843 by alexpott: Add event listener to check schema on config...

Status: Fixed » Closed (fixed)

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