Issue #2293063 by Gábor Hojtsy: Fixed Config schema mapping parsing is...

Issue #2293063 by Gábor Hojtsy: Fixed Config schema mapping parsing is inconsistently forgiving, does not conform to the interface.

core/lib/Drupal/Core/Config/Schema/Mapping.php

@@ -27,22 +27,18 @@ class Mapping extends ArrayElement implements ComplexDataInterface {
   protected $propertyDefinitions;
 
   /**
-   * Overrides ArrayElement::parse()
-   *
-   * Note this only returns elements that have a data definition.
+   * {@inheritdoc}
    */
   protected function parse() {
     $elements = array();
     foreach ($this->getPropertyDefinitions() as $key => $definition) {
-      if (isset($this->value[$key]) || array_key_exists($key, $this->value)) {
-        $elements[$key] = $this->parseElement($key, $this->value[$key], $definition);
-      }
+      $elements[$key] = $this->parseElement($key, $this->value[$key], $definition);
     }
     return $elements;
   }
 
   /**
-   * Implements Drupal\Core\TypedData\ComplexDataInterface::get().
+   * {@inheritdoc}
    *
    * Since all configuration objects are mappings the function will except a dot
    * delimited key to access nested values, for example, 'page.front'.
@@ -53,19 +49,22 @@ public function get($property_name) {
     $elements = $this->getElements();
     if (isset($elements[$root_key])) {
       $element = $elements[$root_key];
+      // If $property_name contained a dot recurse into the keys.
+      while ($element && ($key = array_shift($parts)) !== NULL) {
+        if (method_exists($element, 'get')) {
+          $element = $element->get($key);
+        }
+        else {
+          $element = NULL;
+        }
+      }
     }
-    else {
-      throw new SchemaIncompleteException(String::format("The configuration property @key doesn't exist.", array('@key' => $property_name)));
+    if (isset($element)) {
+      return $element;
     }
-
-    // If $property_name contained a dot recurse into the keys.
-    foreach ($parts as $key) {
-     if (!is_object($element) || !method_exists($element, 'get')) {
-        throw new SchemaIncompleteException(String::format("The configuration property @key does not exist.", array('@key' => $property_name)));
-      }
-      $element = $element->get($key);
+    else {
+      throw new \InvalidArgumentException(String::format("The configuration property @key doesn't exist.", array('@key' => $property_name)));
     }
-    return $element;
   }
 
   /**
@@ -130,9 +129,9 @@ public function getPropertyDefinition($name) {
   public function getPropertyDefinitions() {
     if (!isset($this->propertyDefinitions)) {
       $this->propertyDefinitions = array();
-      foreach ($this->definition['mapping'] as $key => $definition) {
-        $value = isset($this->value[$key]) ? $this->value[$key] : NULL;
-        $this->propertyDefinitions[$key] = $this->buildDataDefinition($definition, $value, $key);
+      foreach ($this->getAllKeys() as $key) {
+        $definition = isset($this->definition['mapping'][$key]) ? $this->definition['mapping'][$key] : array();
+        $this->propertyDefinitions[$key] = $this->buildDataDefinition($definition, $this->value[$key], $key);
       }
     }
     return $this->propertyDefinitions;

core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php

@@ -78,15 +78,11 @@ public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $co
    */
   protected function checkValue($key, $value) {
     $error_key = $this->configName . ':' . $key;
-    $element = FALSE;
-    try {
-      $element = $this->schema->get($key);
-    }
-    catch (SchemaIncompleteException $e) {
-      if (is_scalar($value) || $value === NULL) {
-        return array($error_key => 'Missing schema');
-      }
+    $element = $this->schema->get($key);
+    if ($element instanceof Undefined) {
+      return array($error_key => 'Missing schema.');
     }
+
     // Do not check value if it is defined to be ignored.
     if ($element && $element instanceof Ignore) {
       return array();
@@ -112,7 +108,7 @@ protected function checkValue($key, $value) {
     else {
       $errors = array();
       if (!$element instanceof ArrayElement) {
-        $errors[$error_key] = 'Non-scalar value but not defined as an array (such as mapping or sequence)';
+        $errors[$error_key] = 'Non-scalar value but not defined as an array (such as mapping or sequence).';
       }
 
       // Go on processing so we can get errors on all levels. Any non-scalar

core/lib/Drupal/Core/Config/StorableConfigBase.php

@@ -9,7 +9,6 @@
 
 use Drupal\Component\Utility\String;
 use Drupal\Core\Config\Schema\Ignore;
-use Drupal\Core\Config\Schema\SchemaIncompleteException;
 use Drupal\Core\TypedData\PrimitiveInterface;
 use Drupal\Core\TypedData\Type\FloatInterface;
 use Drupal\Core\TypedData\Type\IntegerInterface;
@@ -137,8 +136,15 @@ protected function getSchemaWrapper() {
   /**
    * Validate the values are allowed data types.
    *
-   * @throws UnsupportedDataTypeConfigException
-   *   If there is any invalid value.
+   * @param string $key
+   *   A string that maps to a key within the configuration data.
+   * @param string $value
+   *   Value to associate with the key.
+   *
+   * @return null
+   *
+   * @throws \Drupal\Core\Config\UnsupportedDataTypeConfigException
+   *   If the value is unsupported in configuration.
    */
   protected function validateValue($key, $value) {
     // Minimal validation. Should not try to serialize resources or non-arrays.
@@ -167,21 +173,15 @@ protected function validateValue($key, $value) {
    *   The value cast to the type indicated in the schema.
    *
    * @throws \Drupal\Core\Config\UnsupportedDataTypeConfigException
-   *   Exception on unsupported/undefined data type deducted.
+   *   If the value is unsupported in configuration.
    */
   protected function castValue($key, $value) {
-    $element = FALSE;
-    try {
-      $element = $this->getSchemaWrapper()->get($key);
-    }
-    catch (SchemaIncompleteException $e) {
-      // @todo Consider making schema handling more strict by throwing
-      // SchemaIncompleteException for all incomplete schema conditions *and*
-      // throwing it forward. See https://drupal.org/node/2183983.
-      // Until then, we need to handle the Undefined case below.
-    }
+    $element = $this->getSchemaWrapper()->get($key);
     // Do not cast value if it is unknown or defined to be ignored.
     if ($element && ($element instanceof Undefined || $element instanceof Ignore)) {
+      // Do validate the value (may throw UnsupportedDataTypeConfigException)
+      // to ensure unsupported types are not supported in this case either.
+      $this->validateValue($key, $value);
       return $value;
     }
     if (is_scalar($value) || $value === NULL) {

core/modules/config/src/Tests/ConfigSchemaTest.php

@@ -85,6 +85,13 @@ function testSchemaMapping() {
     $expected['type'] = 'undefined';
     $expected['definition_class'] = '\Drupal\Core\TypedData\DataDefinition';
     $this->assertEqual($definition, $expected, 'Automatic type detected for a list is undefined.');
+    $definition = $config['testnoschema']->getDataDefinition()->toArray();
+    $expected = array();
+    $expected['label'] = 'Undefined';
+    $expected['class'] = '\Drupal\Core\Config\Schema\Undefined';
+    $expected['type'] = 'undefined';
+    $expected['definition_class'] = '\Drupal\Core\TypedData\DataDefinition';
+    $this->assertEqual($definition, $expected, 'Automatic type detected for an undefined integer is undefined.');
 
     // Simple case, straight metadata.
     $definition = \Drupal::service('config.typed')->getDefinition('system.maintenance');

core/modules/config/src/Tests/SchemaCheckTraitTest.php

@@ -64,12 +64,15 @@ public function testTrait() {
     $ret = $this->checkConfigSchema($this->typedConfig, 'config_test.types', $config_data);
     $this->assertIdentical($ret, TRUE);
 
-    // Add a new key and new array to test the error messages.
+    // Add a new key, a new array and overwrite boolean with array to test the
+    // error messages.
     $config_data = array('new_key' => 'new_value', 'new_array' => array()) + $config_data;
+    $config_data['boolean'] = array();
     $ret = $this->checkConfigSchema($this->typedConfig, 'config_test.types', $config_data);
     $expected = array(
-      'config_test.types:new_key' => 'Missing schema',
-      'config_test.types:new_array' => 'Non-scalar value but not defined as an array (such as mapping or sequence)',
+      'config_test.types:new_key' => 'Missing schema.',
+      'config_test.types:new_array' => 'Missing schema.',
+      'config_test.types:boolean' => 'Non-scalar value but not defined as an array (such as mapping or sequence).',
     );
     $this->assertIdentical($ret, $expected);
   }

core/modules/config/tests/config_schema_test/config/install/config_schema_test.someschema.yml

@@ -2,3 +2,4 @@ testitem: 'Since this file at least has top level schema in config_test.schema.y
 testlist:
  - 'Direct string items are identified and other items are'
  - 'recognized as undefined types.'
+testnoschema: 12

core/modules/config/tests/config_test/config/schema/config_test.schema.yml

@@ -41,9 +41,13 @@ config_test.query.*:
       label: 'Label'
     array:
       type: sequence
-      label: 'Array'
+      label: 'Array level 1'
       sequence:
-        - type: string
+        - type: sequence
+          label: 'Array level 2'
+          sequence:
+            - type: integer
+              label: 'Value'
     number:
       type: integer
       label: 'number'

core/modules/field/config/schema/field.schema.yml

@@ -45,8 +45,8 @@ field.field.*.*:
         - type: sequence
           label: 'Indexes'
           sequence:
-            - type: string
-              label: 'Column'
+            - type: ignore
+              label: 'Index'
 
 field.instance.*.*.*:
   type: config_entity

core/modules/filter/tests/filter_test/config/schema/filter_test.schema.yml

+# Schema for the configuration files of the Filter test module.
+
+filter_settings.filter_test_restrict_tags_and_attributes:
+  type: filter
+  label: 'Filter to restirct HTML tags and attributes'
+  mapping:
+    restrictions:
+      type: mapping
+      label: 'Restrictions'
+      mapping:
+        allowed:
+          type: sequence
+          label: 'Allowed tags and attributes'
+          sequence:
+            - type: ignore
+              label: 'Tag and optionally list of attributes'
+        forbidden_tags:
+          type: sequence
+          label: 'Forbidden tags'
+          sequence:
+            - type: boolean
+              label: 'Tag'

core/modules/link/src/Tests/LinkFieldTest.php

@@ -376,10 +376,10 @@ function testLinkFormatter() {
       'rel' => array(NULL, 'nofollow'),
       'target' => array(NULL, '_blank'),
       'url_only' => array(
-        array('url_only' => array(FALSE)),
-        array('url_only' => array(FALSE), 'url_plain' => TRUE),
-        array('url_only' => array(TRUE)),
-        array('url_only' => array(TRUE), 'url_plain' => TRUE),
+        array('url_only' => FALSE),
+        array('url_only' => FALSE, 'url_plain' => TRUE),
+        array('url_only' => TRUE),
+        array('url_only' => TRUE, 'url_plain' => TRUE),
       ),
     );
     foreach ($options as $setting => $values) {

core/modules/migrate_drupal/config/install/migrate.migration.d6_vocabulary_field_instance.yml

@@ -12,8 +12,6 @@ process:
     plugin: migration
     migration: d6_taxonomy_vocabulary
     source: vid
-  'settings.allowed_values.0.vocabulary': @field_name
-  'settings.allowed_values.0.parent': constants.parent
 destination:
   plugin: entity:field_instance_config
 migration_dependencies:

core/modules/migrate_drupal/src/Tests/d6/MigrateVocabularyFieldInstanceTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\migrate_drupal\Tests\d6;
 
+use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\migrate\MigrateExecutable;
 use Drupal\migrate_drupal\Tests\MigrateDrupalTestBase;
 
@@ -65,6 +66,15 @@ protected function setUp() {
       'entity_type' => 'node',
       'name' => 'tags',
       'type' => 'taxonomy_term_reference',
+      'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
+      'settings' => array(
+        'allowed_values' => array(
+          array(
+            'vocabulary' => 'tags',
+            'parent' => 0,
+          ),
+        ),
+      ),
     ))->save();
 
     $migration = entity_load('migration', 'd6_vocabulary_field_instance');
@@ -84,15 +94,11 @@ public function testVocabularyFieldInstance() {
     $field_id = 'node.article.tags';
     $field = entity_load('field_instance_config', $field_id);
     $this->assertEqual($field->id(), $field_id, 'Field instance exists on article bundle.');
-    $settings = $field->getSettings();
-    $this->assertEqual('tags', $settings['allowed_values'][0]['vocabulary'], "Vocabulary has correct settings.");
 
     // Test the page bundle as well.
     $field_id = 'node.page.tags';
     $field = entity_load('field_instance_config', $field_id);
     $this->assertEqual($field->id(), $field_id, 'Field instance exists on page bundle.');
-    $settings = $field->getSettings();
-    $this->assertEqual('tags', $settings['allowed_values'][0]['vocabulary'], "Vocabulary has correct settings.");
 
     $this->assertEqual(array('node', 'article', 'tags'), entity_load('migration', 'd6_vocabulary_field_instance')->getIdMap()->lookupDestinationID(array(4, 'article')));
   }

core/modules/options/config/schema/options.schema.yml

@@ -35,11 +35,8 @@ field.list_float.settings:
   label: 'List (float) settings'
   mapping:
     allowed_values:
-      type: sequence
+      type: ignore
       label: 'Allowed values list'
-      sequence:
-        - type: string
-          label: 'Value'
     allowed_values_function:
       type: string
       label: 'Allowed values function'

core/modules/update/tests/modules/update_test/config/schema/update_test.schema.yml

@@ -8,14 +8,21 @@ update_test.settings:
       type: sequence
       label: 'System info'
       sequence:
-        - type: string
-          label: 'Value'
+        - type: sequence
+          label: 'Items'
+          sequence:
+            - type: string
+              label: 'Item'
     update_status:
       type: sequence
       label: 'Update status'
       sequence:
-        - type: string
-          label: 'Value'
+        - type: mapping
+          label: 'Module'
+          mapping:
+            status:
+              type: integer
+              label: 'Value'
     xml_map:
       type: sequence
       label: 'XML map'

core/modules/user/config/schema/user.views.schema.yml

@@ -9,11 +9,15 @@ views.access.perm:
       label: 'Permission'
 
 views.access.role:
-  type: sequence
-  label: 'Role'
-  sequence:
-    - type: string
-      label: 'Role'
+  type: mapping
+  label: 'Roles'
+  mapping:
+    role:
+      type: sequence
+      label: 'List of roles'
+      sequence:
+        - type: string
+          label: 'Role'
 
 views.argument.user_uid:
   type: views.argument.numeric