Problem/motivation

Schema discovery can't recognize schema defintion with more than one wild card selection. For example:

breakpoint.breakpoint.module.toolbar.* works but not breakpoint.breakpoint.*.*.*

which is blocking #1912308-26: Create configuration schemas for breakpoint module

Proposed solution

Updte SchemaDiscovery::getFallbackName() to check for fallback recursively.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.7 KB

Here is the patch to solve this issue.

vijaycs85’s picture

Updating method doc-comment to reflect changes...

vijaycs85’s picture

Please ignore patch in #2. here is the right patch for the comments in #2

vijaycs85’s picture

Issue tags: +Needs tests

Just had a word with @alexpott and adding tag as advised...

vijaycs85’s picture

Adding tests and comments...

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Why do you think it is important to have two separate files with same formatted keys and similar schema? Sounds like one would be fine to test this. Also testid and testdescription in the keys can just loose the test prefix I think, becoming id and description.

Gábor Hojtsy’s picture

Issue tags: +sprint

Also putting on sprint for D8MI.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
5.62 KB
3.6 KB

Thanks for the review @Gábor Hojtsy. Updated key names. The reason for two files is that we want to prove that it works for multiple files with same format. But my test wasn't right. Updated test with proper file name to test this.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/Schema/SchemaDiscovery.phpundefined
@@ -48,7 +48,7 @@ public function getDefinition($base_plugin_id) {
-    elseif (strpos($base_plugin_id, '.') && ($name = $this->getFallbackName($base_plugin_id)) && isset($this->definitions[$name])) {
+    elseif (strpos($base_plugin_id, '.') && $name = $this->getFallbackName($base_plugin_id)) {

How come can/would you remove the isset()? It is still possible that a name would not be found, right? How does this not caught by tests (we do have tests for keys without schema). What do I misunderstand?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.phpundefined
@@ -129,6 +129,32 @@ function testSchemaMapping() {
+    // This should be the schema of config_test.someschema.somemodule.*.*.
+    $expected = array();
+    $expected['label'] = 'Schema multiple filesytem marker test';
+    $expected['class'] = '\Drupal\Core\Config\Schema\Mapping';
+    $expected['mapping']['id']['type'] = 'string';
+    $expected['mapping']['id']['label'] = 'ID';
+    $expected['mapping']['description']['type'] = 'text';
+    $expected['mapping']['description']['label'] = 'Description';

Same expectation as above, so you can remove all these lines and just use $expected like already set up above?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
5.22 KB
+++ b/core/lib/Drupal/Core/Config/Schema/SchemaDiscovery.phpundefined
@@ -95,13 +95,27 @@ protected function loadAllSchema() {
+      if (isset($this->definitions[$replaced])) {
+        return $replaced;

isset() has been moved to getFallbackNmae() to check recursively.

Removed the assert test copy.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks for the insight. Now only two comments :)

+++ b/core/lib/Drupal/Core/Config/Schema/SchemaDiscovery.phpundefined
@@ -95,13 +95,27 @@ protected function loadAllSchema() {
+   * @return null|string
+   *   Same name with the last part(s) replaced by the filesystem marker.
+   *   for example, breakpoint.breakpoint.module.toolbar.narrow check for
+   *   definition in below order:
+   *     breakpoint.breakpoint.module.toolbar.*
+   *     breakpoint.breakpoint.module.*.*
+   *     breakpoint.breakpoint.*.*.*

Document it would return null if no matching element found IMHO.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.phpundefined
@@ -129,6 +129,24 @@ function testSchemaMapping() {
+    $definition = config_typed()->getDefinition('config_test.someschema.somemodule.section_two.subsection');
+    // This should be the same schema in $expected.
+    $this->assertEqual($definition, $expected, 'Retrieved the right metadata for config_test.someschema.somemodule.section_two.subsection');

Change comment to this IMHO:

// The other file should have the same schema.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.45 KB
5.26 KB

Updated...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks for the great tests.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updating summary with blocked defect details...