Config schema fails to expand dynamic top-level types

I've re-rolled this with the failing patch, attached.

I started debugging this in TypedConfigManager::buildDataDefinition() and i'm not sure if it's correct but what i'm seeing is that:

  $type = $this->replaceName($type, $replace); // $type = 'wrapping.test.plugin_types.wrapper';
  $definition += $this->getDefinition($type);

Inside getDefinition, $type gets turned back into wrapping.test.plugin_types.* which then does a getDefinition('test.plugin_types.[plugin_id]') which does not pick up test.plugin_types.wrapper definition and therefore internal_value does not get it's schema.

OK, this patch fixes the issue. I'm not sure it's exactly right but I wanted to see what else might fail.

+++ b/core/modules/config/tests/config_schema_test/config/schema/config_schema_test.schema.yml
@@ -213,3 +213,23 @@ config_test.dynamic.*.third_party.config_schema_test:
+wrapping.config_schema_test.plugin_types:
+  type: config_object
+  mapping:
+    tests:
+      type: sequence
+      sequence:
+        - type: wrapping.test.plugin_types.[plugin_id]
+
+wrapping.test.plugin_types.*:
+  type: test.plugin_types.[plugin_id]
+  mapping:
+    wrapper_value:
+      type: string
+
+test.plugin_types.wrapper:
+  type: test.plugin_types
+  mapping:
+    internal_value:
+      type: string

Where is the plugin_id key defined (with its type)?

I think it's just missing from this test example? test.plugin_types.* would provide the definition for that?

All right, its just in the dynamic type but it is defined in the base type. Makes sense.

I think the patch looks generally good, it would be nice to outline the solution in the summary, so committers have an understanding. Two notes though:

1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -100,9 +100,13 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
   +      $definition += $this->getDefinition($type, TRUE, $replace);
   +    }
   +    else {
   +      // Add default values from type definition.
   +      $definition += $this->getDefinition($type);
   

   Would be good to add a short summary comment on why the prior call is different.

2. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -116,10 +120,17 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
   -  public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
   -    $definitions = $this->getDefinitions();
   
   @@ -131,6 +142,15 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $value = NULL) {
   +    $definitions = $this->getDefinitions();
   

   So you just added this new value argument or is that already on the interface?

Just a note: this issue is now blocking Panels and Panelizer as well, and the patch from #14 allows everything to work for me!

Would be good to add a short summary comment on why the prior call is different.

So you just added this new value argument or is that already on the interface?

I added the new parameter, when we're handing sub-types we need to pass the parent definition along to getDefinition() which is why they're different from the calling code. This does raise a good point, anyone using TypedConfigManager needs to be aware that without passing $value along to getDefinition(), sub-types won't be expanded correctly, in addition, we should update the interface docs.

All right, if the public interface needs to change, that may not be allowed before Drupal 9. How does PHP react if an optional argument is added on an interface but not yet added in an implementation? Does that count as a fatal for not implementing the interface properly or not? http://php.net/manual/en/language.oop5.interfaces.php does not seem to document that, maybe this comment is relevant: http://php.net/manual/en/language.oop5.interfaces.php#77950 -- that would suggest we cannot add a new optional argument on an interface and assume that will not break BC?

Yeah that would be a fatal, we could probably write this in a different way to avoid backward compatibility. We'd need an internal method that did something like what getDefinition() does now which can be used correctly by buildDataDefinition() and then we'd keep getDefinition() as is, and add a new method like, getComplexDefinition()?

Although, that's all just a workaround, the right thing is to add the new parameter as it is required for getDefinition() to correctly calculate the definition, unless we can solve that another way? I don't think it's possible to derive it.

We need to update the documentation on TypedConfigManagerInterface::getDefinition()... I'm not wild about adding a parameter to that method considering it comes from \Drupal\Component\Plugin\Discovery\DiscoveryInterface

If we proceed with the patch as is we're going to have to make a tough choice about was this means for BC. I'm not sure this is acceptable in a patch release.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -138,6 +158,14 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
+      // If the sub type has a bracket then lets look that up as well.
+      if (isset($value) && strpos($definition['type'], ']')) {
+        $sub_type = $this->determineType($this->replaceName($definition['type'], $value), $definitions);
+        $merge = $definitions[$sub_type];
+        $definition = NestedArray::mergeDeepArray(array($merge, $definition), TRUE);
+      }

What happens if we get another bracket?

I don't think we actually have to change the interface, we're only using the extra parameter internally. We can just pull out the logic of getDefinition() into an internal helper method, and then keep the interface the same.

I need to revisit this. Running protected $strictConfigSchema = FALSE; in all my tests is unfortunate.

You may be interested in #2580389: Allow test classes to specify the config object exceptions - that lets you exclude individual objects from the validation.

1. +++ b/core/modules/config/src/Tests/ConfigSchemaTest.php
   @@ -506,4 +506,72 @@ public function testConfigSchemaInfoAlter() {
   +    $untyped_to_typed = $untyped_values;
   ...
   +    $untyped_to_typed = $untyped_values;
   

   what is this for?

2. +++ b/core/modules/config/tests/config_schema_test/config/schema/config_schema_test.schema.yml
   @@ -213,3 +213,50 @@ config_test.dynamic.*.third_party.config_schema_test:
   +        - type: wrapping.test.double_brackets.[foo]
   ...
   +  type: test.double_brackets.[foo].[bar]
   

   this might be a bit more clear if we dont use "foo" to mean two different possible things.

3. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -116,10 +120,17 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
   +   * @param $base_plugin_id
   

   missing type

4. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -131,13 +142,38 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
   +      // If the sub type has a bracket then lets look that up as well.
   

   could be better english grammar.

5. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
   @@ -131,13 +142,38 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
   +        $merge = $definitions[$sub_type];
   

   merge? maybe a better var name, or inline.

thanks for the issue summary update, and the conversation while we discussed this, latest patch addresses the feedback I had and alex's, thanks!

This approach looks great - nice work.

1. +++ b/core/modules/config/src/Tests/ConfigSchemaTest.php
   @@ -506,4 +506,70 @@ public function testConfigSchemaInfoAlter() {
   +   * Tests saving config when the type is wrapped by a dynamic type.
   ...
   +   * Tests saving config when the type is wrapped by a dynamic type.
   

   Same description for two different tests - which is a bit confusing. Perhaps we can improve the test documentation to show what schemas are being used where - so reviewers have to think less :)

2. Less importantly and out-of-scope here - can we file a followup to examine what to do with $exception_on_invalid - it could be just document that it does nothing - and it would be good to have a test for this behaviour. Or maybe we could implement it.

Added credit ticking for reviewers.

Changed the description for the second test, I hope that's descriptive enough.

So how will this code throw or not an exception based on this boolean?

Committed 9ccbfda and pushed to 8.1.x. Thanks!

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -116,10 +117,17 @@ public function buildDataDefinition(array $definition, $value, $name = NULL, $pa
+  protected function determineType($base_plugin_id, array $definitions) {

@@ -131,6 +139,27 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE) {
+  protected function getDefinitionWithReplacements($base_plugin_id, array $replacements, $exception_on_invalid = TRUE) {

To commit this bug fix to 8.0.x we need to prefix these methods with an underscore.

Thanks Tim.

Committed 4335d12 and pushed to 8.0.x. Thanks!

Yay, thanks all! Great seeing important bugs fixed with config schema :) All of contrib will be happy :)

Could be related to this: #2661710: Problems with page_manager.page_variant.* schema

This introduced some problems... #2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition

(Edit: crossposted with alex)

Found a nasty bug with this: #2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition.

Page Manager only works because we don't have any tests that are adding more than one block of different types to the same display variant.

See also #2663376: The core block_settings schema defines schema for block_content blocks for another confusing thing that is partially hiding this the problem here.