Make schema testing code available as generic API

The related issue has been committed, so moving back to active

After giving a try to this one, this is what I've found:

a) There's no simple way to implement some generic data validation in Drupal core. We need some generic validation for it to be reused in tests and in contrib modules. The Drupal generic validation way is now using Symfony validation constraints and I'm afraid implementing such complex thing would be out of scope for D8 atm.

b) Yes, we can just move these methods to somewhere else but that would be a very limited validation and doesn't even use PrimitiveTypeConstraintValidator which is what would take properly implementing a minimal reusable validation of TypedData elements. It is not good enough to validate generic configuration data, though it gets the job done for this single test.

Thus just moving this around for it to be reusable will be more harm than good if others are tempted to use it for implementing schema validation. My recommendation is we close this one as "won't fix", and think about something better (proper validation) for contrib modules.

FYI I'll be giving a try to some properly built config schema validation in a contrib module, not for Drupal core though.

References (Proper Drupal 8 reusable validation):
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Validatio...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Validatio...

I still think that code is useless for anything else than testing. If you want full schema validation (using typed data validation, etc..) take a look at this module, https://drupal.org/sandbox/reyero/config_schema

I found this code very useful, and opened #2274211: ConfigSchemaTestBase should be a trait to do the same thing.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTestBase.php
@@ -7,42 +7,16 @@
 /**
  * Provides a base class to help test configuration schema.
  */
 abstract class ConfigSchemaTestBase extends WebTestBase {

We should move all the functionality into the trait or maybe have another test trait that uses the new schema trait and then the classes that used to extend form ConfigSchemaTestBase can extend from KernelTestBase or WebTestBase as appropriate. We already discussed the calling of assertion in traits for testing in IRC and decided it was okay.

Yep

This seems like a good idea to me:

trait SchemaCheckTestTrait {
  use SchemaCheckTrait;
  public function assertConfigSchema(TypedConfigManagerInterface $typed_config, $config_name, $config_data) {
   ...
  }
}

But wouldn't that require #2260053: Make test helper traits explicitly define expected TestBase methods first?

Here is an update, though it depends on #2260053: Make test helper traits explicitly define expected TestBase methods, no fatal.

1. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
   @@ -2,26 +2,22 @@
   + * Contains Drupal\Core\Config\Schema\SchemaCheckTrait.
   

   Contains \Drupal

2. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
   @@ -2,26 +2,22 @@
   + * Provides a class for checking configuration schema.
   

   This is not a class, its a trait now

3. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
   @@ -53,22 +42,25 @@
   +   * @return array|FALSE|TRUE
   +   *   False if no schema found. List of errors if any found. TRUE if fully
   +   *   valid.
   

   Can be array|bool, and the description should have FALSE not False.

4. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
   @@ -111,12 +104,13 @@ protected function checkValue($key, $value) {
   +        return array($error_key => "Variable type is $type but applied schema class is $class.");
   ...
   +        $errors[$error_key] = 'Non-scalar value but not defined as an array (such as mapping or sequence)';
   

   Now that these aren't just assertion messages, should we translate them?

5. +++ b/core/modules/config/src/Tests/SchemaCheckTestTrait.php
   @@ -0,0 +1,52 @@
   +      //@TODO: Since the use of this trait is under TestBase, it works.
   +      // Can be fixed as part of https://drupal.org/node/2260053.
   ...
   +      //@TODO: Since the use of this trait is under TestBase, it works.
   +      // Can be fixed as part of https://drupal.org/node/2260053.
   ...
   +        //@TODO: Since the use of this trait is under TestBase, it works.
   +        // Can be fixed as part of https://drupal.org/node/2260053.
   

   // @todo Since, and the second line should be indented 2 spaces

Thanks for the review @tim.plunkett. Here is an update on each item:

#20.1 - Fixed
#20.2 - Fixed
#20.3 - Fixed
#20.4 - Not fixed - as per #500866: [META] remove t() from assert message, we don't t() assert messages.
#20.5 - Fixed

Looks good, thanks!

I have added the code to create instance of trait using phpunit built in function getObjectForTrait

Thanks for the patch @nvinayvinay, but I don't see why are we getting getObjectForTrait while you can directly use them in your test class. If you still fee it is an improvement, please apply patch in #21 and make your changes on top of it.

Setting status back to review for the patch in #21.

As per #22 it is ready to go.(Cleanly applies to current HEAD)...

Re uploading the same patch in #21

29: 2270815-config-schema-trait-21.patch queued for re-testing.

Reroll...

As per #22 it is ready to go.

Committed 463fd53 and pushed to 8.x. Thanks!

yay, thanks!

See #2287223: Use KernelTestBase for config schema tests where possible for an easy follow-up.

See #2287193: checkConfigSchema() can fail to report errors for an essential followup