Problem/Motivation

Drupal\config\Tests\ConfigSchemaTestBase has a fabulous testing facility to ensure config schema completeness in its checkValue() method. This would be amazing to be reused as a debugging tool. Eg. #2266427: Add more info on schema validity to configuration inspector for debugging could use this code to summarize no/full/partial schema validity for configuration as well as collect data granular enough to help with debugging and solving config schema issues much faster.

Proposed resolution

Abstract the logic from checkValue() out so that contrib can use this. Instead of directly throwing failures from it, collect the errors and let the implementor handle them as needed.

Remaining tasks

Agree on approach.
Implement.

User interface changes

None.

API changes

No. API addition with new schema testing functionality instead.

Comments

Gábor Hojtsy’s picture

Status: Active » Postponed

Postponing #2264179: Clarify use of property/undefined and add an ignore type in configuration schema because that is RTBC and touches around the same code.

Jose Reyero’s picture

Status: Postponed » Active

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

Jose Reyero’s picture

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...

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php. View

Here is a quick approach of this using a trait that contrib can use as well. Not sure how safe the property names are then, may need to be rejiggered a bit. Also not really tested, looking forward to testbot results. No need to override fail() anymore since we collect all the errors in an array instead. This will be useful for contrib to map to the actual config and show actual places where errors are found.

Status: Needs review » Needs work

The last submitted patch, 4: schema-validate-reuse.patch, failed testing.

YesCT’s picture

Issue tags: +Traits
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,599 pass(es), 0 fail(s), and 6,457 exception(s). View
1.02 KB

Ugh, PHP error.

Status: Needs review » Needs work

The last submitted patch, 7: schema-validate-reuse-7.patch, failed testing.

Jose Reyero’s picture

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

Gábor Hojtsy’s picture

If this is useless, then core is dumb. A slight variation of the same code is used to typecast configuration, find translatable stuff, etc. So if we are to build something more powerful in contrib than what we did in core is pointless. We'll obviously not be able to ensure any particular contrib would be used on all D8 sites (there is not even Views in contrib to try and use ctools as a "trojan horse"). It also does not seem to be possible for that contrib to override all of those dumb implementations in core. So if we think that core is inadequate for extension, then that should be fixed there not with a contrib replacement. I firmly believe that we should help developers comply with core with the same tool that core would use to ensure their compliance, which is this exact code. If this code is dumb, then it needs to be improved not sidestepped as this code will be used by most sites unless your module successfully becomes the next ctools that is to overcome core issues for an extremely popular set of contrib modules.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,741 pass(es). View
440 bytes

Helps to return an empty array when no errors are found.

tim.plunkett’s picture

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

Gábor Hojtsy’s picture

@tim.plunkett: woot, wanna help review this? My goal was to use this as a schema review/debugging tool at #2266427: Add more info on schema validity to configuration inspector for debugging.

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

Gábor Hojtsy’s picture

@alexpott: the only way to reuse this as a general schema vs. config checker is if there is no assertions thrown / test environment assumed in the trait. Do you suggest a trait that extends another trait for the test? :D

alexpott’s picture

Yep

Gábor Hojtsy’s picture

@tim.plunkett: what do you think about this?

tim.plunkett’s picture

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?

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2014_MAY
FileSize
10.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,687 pass(es). View
4.88 KB
tim.plunkett’s picture

  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

vijaycs85’s picture

FileSize
10.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,790 pass(es). View
2.85 KB

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint

All right, let's do this and we can still refine this later based on our findings in #2183983: Find hidden configuration schema issues and similar places.

Wanna keep track of this in D8MI since we plan to use this code to figure out how the rest of the Drupal configs fair against their schema and if there are missing pieces for translation. So adding D8MI tags.

nvinayvinay’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Configuration system, -Traits, -LONDON_2014_MAY +Configuration system Traits LONDON_2014_MAY
FileSize
9.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch schema-validate-reuse-22.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 24: schema-validate-reuse-22.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -Configuration system Traits LONDON_2014_MAY +Configuration system, +Traits, +LONDON_2014_MAY

@nvinayvinay: please post an interdiff and please make your patch against the latest HEAD. Also fixing your tags change.

vijaycs85’s picture

Status: Needs work » Needs review

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.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

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

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Re uploading the same patch in #21

Status: Needs review » Needs work

The last submitted patch, 29: 2270815-config-schema-trait-21.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: 2270815-config-schema-trait-21.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,487 pass(es). View
708 bytes

Reroll...

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

As per #22 it is ready to go.

  • Commit 463fd53 on 8.x by alexpott:
    Issue #2270815 by vijaycs85, Gábor Hojtsy, nvinayvinay: Make schema...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

vijaycs85’s picture

Issue tags: -sprint

yay, thanks!

tim.plunkett’s picture

alexpott’s picture

Status: Fixed » Closed (fixed)

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