Merging data types leads to incomplete definition

1. +++ b/core/modules/config/tests/config_schema_test/config/schema/config_schema_test.schema.yml
   @@ -270,3 +270,30 @@ test.double_brackets.turtle.horse:
   +test.double_brackets.cat:*.*:
   
   +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -631,6 +631,24 @@ public function testConfigSaveWithWrappingSchemaDoubleBrackets() {
   +          'id' => 'cat:persion.dog',
   

   why the colon in the id? is that what indicates an inheritance? ... or is the colon just part of an arbitrary string, and not syntactically significant (maybe it is a convention used in panels id strings and so used here in a similar use case example to match something practical that people might use).

2. what gets lost in the inheritance (merge) the child stuff or the parent stuff?

   webflow says: I guess it is because we didn't fully resolve the parent dynamic type, so the parent property was missing?

   (in the current test, without the fix, the config gets either 1 or 3 properties.. I wonder if we could count the properties in the assert, nah, probably better to assert the expected ones are there with the values than to just count them)

   maybe that means what is missing is the parent stuff, in this case the breed.

   wait...

   +++ b/core/modules/config/tests/config_schema_test/config/schema/config_schema_test.schema.yml
   @@ -270,3 +270,30 @@ test.double_brackets.turtle.horse:
   +test.double_brackets.breed:
   +  type: test.double_brackets
   +  mapping:
   +    breed:
   +      type: string
   

   test.double_brackets.breeed "extends" test.double_brackets

   so like php classes and extending, breed is a property from a "subclass". so... id, foo, and bar are parent properties, and breed is a child property?

   ??? (trying to use the vocabulary to understand what the words in the issue mean, and how we can document what this is doing)

   maybe re-reading http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet would help.

3. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -631,6 +631,24 @@ public function testConfigSaveWithWrappingSchemaDoubleBrackets() {
   
   @@ -631,6 +631,24 @@ public function testConfigSaveWithWrappingSchemaDoubleBrackets() {
   +    \Drupal::configFactory()->getEditable('wrapping.config_schema_test.other_double_brackets')
   +      ->setData($typed_values)
   +      ->save();
   +    $tests = \Drupal::service('config.typed')->get('wrapping.config_schema_test.other_double_brackets')->get('tests')->getElements();
   +    $definition = $tests[0]->getDataDefinition()->toArray();
   +    $this->assertEqual($definition['type'], 'wrapping.test.other_double_brackets.*||test.double_brackets.cat:*.*');
   

   the test fails nicely on the ->save() because it can't save data for a property (?) that is missing from the bad merge.

   it would be extra good to also assert the properties are there, and have the values expected in an actual assert.

4. This test has many levels of inheritance, and two of them are dynamic. I wonder if this will work for any number of dynamic things in a chain (merge).

   Can answer this two ways probably, looking at the fix in context (to see where it is in loops), and a test that has 3 dynamic types would prove it would work for any number.

5. I wonder if we can make this test more clear with some better names for things? ... or comments in the test or schema?

6. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
   @@ -631,6 +631,24 @@ public function testConfigSaveWithWrappingSchemaDoubleBrackets() {
   +    $typed_values = [
   +      'tests' => [
   ...
   +    \Drupal::configFactory()->getEditable('wrapping.config_schema_test.other_double_brackets')
   

   I was wondering why the test did this in the code instead of a config yml file. I looked; the rest of ConfigSchemaTest follows that pattern. ok. :)

@yanniboi Thanks, that additional assert makes me feel like the test is more complete.

I triggered a test run on the patch on 8.1.x so we can see how it does there.

I think it would be good to get a test only patch (and have tests run on it against 8.1.x and 8.2.x).

I would be comfortable with making another issue to look into the 3 dynamic test case, cause I think thinking exactly what that would look like will be challenging and take time, and end up blocking this fix.

I'm going to make that test only patch now.

the tests only patch

Those fails look appropriate to me. (combined with the review, discussion and recent changes to the test: rtbc)

Re-uploading the whole patch from #15 so that when the automatic retest runs, it will run on a passing patch so that the status doesn't change to needs work

opened #2755825: Add tests for 3 dynamic merges to verify dynamic data types dependencies lead to complete definitions (not urgent) for a place to think about the test case of 3 levels

It seems like the last test fail was caused by some CI issue. I retested the last patch and tests are okay.

I also tested this patch manually and I can confirm it works great! For example, it solves page_manager blocks translation issue.

Reverting status to RTBC.

This issue causes problems for contributed modules that run schema checks on of their config. Marking as major ("Block contributed projects with no workaround."). It also causes fatal errors via the UI if the config checker service is enabled (i.e. dev workflows).

Committed and pushed 1fbdcc7 to 8.4.x and 9298caf to 8.3.x. Thanks!