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.

Files: 
CommentFileSizeAuthor
#12 1940440-schema-wild-card-12.patch5.26 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 53,103 pass(es). View
#12 1940440-diff-10-12.txt1.45 KBvijaycs85
#10 1940440-schema-wild-card-10.patch5.22 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 53,093 pass(es). View
#10 1940440-diff-8-10.txt1.26 KBvijaycs85
#8 1940440-schema-wild-card-8-testonly.patch3.6 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 53,089 pass(es), 2 fail(s), and 0 exception(s). View
#8 1940440-schema-wild-card-8.patch5.62 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 52,922 pass(es), 2 fail(s), and 1 exception(s). View
#8 1940440-diff-5-8.txt5.33 KBvijaycs85
#5 1940440-schema-wild-card-5-testonly.patch4.01 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 53,102 pass(es), 2 fail(s), and 0 exception(s). View
#5 1940440-schema-wild-card-5.patch6.02 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 53,069 pass(es). View
#3 1940440-schema-wild-card-3.patch1.5 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 53,057 pass(es). View
#2 1940440-schema-wild-card-2.patch1.89 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 53,060 pass(es). View
#1 1940440-schema-wild-card-1.patch1.7 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 52,924 pass(es), 2 fail(s), and 0 exception(s). View

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 52,924 pass(es), 2 fail(s), and 0 exception(s). View

Here is the patch to solve this issue.

vijaycs85’s picture

FileSize
1.89 KB
PASSED: [[SimpleTest]]: [MySQL] 53,060 pass(es). View

Updating method doc-comment to reflect changes...

vijaycs85’s picture

FileSize
1.5 KB
PASSED: [[SimpleTest]]: [MySQL] 53,057 pass(es). View

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

FileSize
6.02 KB
PASSED: [[SimpleTest]]: [MySQL] 53,069 pass(es). View
4.01 KB
FAILED: [[SimpleTest]]: [MySQL] 53,102 pass(es), 2 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 52,922 pass(es), 2 fail(s), and 1 exception(s). View
3.6 KB
FAILED: [[SimpleTest]]: [MySQL] 53,089 pass(es), 2 fail(s), and 0 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 53,093 pass(es). View
+++ 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
PASSED: [[SimpleTest]]: [MySQL] 53,103 pass(es). View

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