Work on validation of config

Civi/Inlay/ArraySchema.php

@@ -59,6 +59,22 @@ namespace Civi\Inlay;
  * - Missing required key at 1»children»0 "name"
  * - Unexpected keys at 1»children»0 ["nom"]
  *
+ * ## Coersion
+ *
+ * If you use setCoerce() then it will make reasonable efforts to coerce scalar values to the desired types.
+ * - most things work as you'd expect.
+ * - if the type is integer, then '123' is cast, but '123.4' is not. '123.00' IS cast.
+ * - it will try to cast to the given types in turn. So if the value can't be cast to the first type
+ *   it will try the next type.
+ * - if NULL is in the list of acceptable types, data will be set NULL, if it could not be cast to
+ *   a previous type.
+ *
+ * ## Fallbacks
+ *
+ * If you use setFallbacks(defaultsArray) then when a value is wrong, it will try to fish out a valid value
+ * from the fallback array.
+ *
+ *
  */
 class ArraySchema {
 
@@ -77,23 +93,23 @@ class ArraySchema {
   protected $removeUnexpectedKeys = FALSE;
 
   /**
-   * @param bool
+   * @param ?array
    */
-  protected $removeInvalid = FALSE;
+  protected $fallbacks = NULL;
 
   public static function getOwnSchema(): array {
     return [
       '//' => [ 'MUST', 'array',
         'schema' => [
           0 => ['MUST', 'string', 'oneOfStrictly' => ['MUST', 'MAY']],
-          1 => ['MUST', 'string|NULL', 'regex' => '/^(boolean|integer|double|string|array|NULL|integerString|numeric|empty)([|](boolean|integer|double|string|array|NULL|integerString|numeric|empty))*$/'],
+          1 => ['MUST', 'string|NULL', 'regex' => '/^(boolean|integer|double|string|array|NULL|numeric|empty)([|](boolean|integer|double|string|array|NULL|numeric|empty))*$/'],
           'schema' => ['MAY', 'array', 'recurse' => []],
           'regex' => ['MAY', 'string'],
           'recurse' => ['MAY', 'array', 'schema' => [
             '//' => ['MAY', 'string']
           ]],
           'oneOfStrictly' => ['MAY', 'array', 'schema' => ['/^[0-9]+$/' => ['MUST', 'integer|string|boolean|double|NULL']]],
-          'oneOf' => ['MAY', 'array', 'schema' => ['/^[0-9]+$/' => ['MUST', 'integer|string|integerString|numeric|boolean|double|NULL']]],
+          'oneOf' => ['MAY', 'array', 'schema' => ['/^[0-9]+$/' => ['MUST', 'integer|string|numeric|boolean|double|NULL']]],
           'gt' => ['MAY', 'string|integer|double'],
           'lt' => ['MAY', 'string|integer|double'],
           'gte' => ['MAY', 'string|integer|double'],
@@ -136,32 +152,35 @@ class ArraySchema {
   /**
    * @return static
    */
-  public function setRemoveInvalid(bool $remove = TRUE) {
-    $this->removeInvalid = $remove;
+  public function setFallbacks(array $fallbacks = []) {
+    $this->fallbacks = $fallbacks;
     return $this;
   }
 
   /**
    * @return static
    */
-  public function setCoerce(bool $coerce = TRUE, ?bool $removeUnexpectedKeys = NULL, ?bool $removeInvalid = NULL) {
+  public function setCoerce(bool $coerce = TRUE, ?bool $removeUnexpectedKeys = NULL, ?array $fallbacks = NULL) {
     $this->coerce = $coerce;
     if (is_bool($removeUnexpectedKeys)) {
       $this->setRemoveUnexpectedKeys($removeUnexpectedKeys);
     }
-    if (is_bool($removeInvalid)) {
-      $this->setRemoveInvalid($removeInvalid);
+    if (is_array($fallbacks)) {
+      $this->setRemoveInvalid($fallbacks);
     }
     return $this;
   }
 
+  /**
+   * This is the primary looping function.
+   */
   protected function matches(array &$data, array $ancestry, array $schema): array {
     $dataKeys = array_flip(array_keys($data));
     $allErrors = [];
     foreach ($schema as $keyMatch => $schema) {
       $keyWasFound = 0;
       if (substr($keyMatch, 0, 1) === '/') {
-        // Regexp
+        // Regexp: one regexp key may match 0+ actual keys.
         foreach($data as $key => &$value) {
           if (preg_match($keyMatch, $key)) {
             $keyWasFound++;
@@ -178,7 +197,16 @@ class ArraySchema {
         unset($value);
       }
       else {
-        // $keyMatch is a simple string.
+        // $keyMatch is a simple string. If we didn't find it, use fallback if poss.
+        if (!array_key_exists($keyMatch, $data) && $schema[0] === 'MUST') {
+          if (is_array($this->fallbacks)) {
+            // Can we substitute this in?
+            $newValue = NULL;
+            if ($this->applyFallback($newValue, [...$ancestry, $keyMatch])) {
+              $data[$keyMatch] = $newValue;
+            }
+          }
+        }
         if (array_key_exists($keyMatch, $data)) {
           $keyWasFound++;
           $valueErrors = $this->valueMatch($keyMatch, $schema, $keyMatch, $data[$keyMatch], [...$ancestry, $keyMatch]);
@@ -191,7 +219,6 @@ class ArraySchema {
         }
       }
       if ($schema[0] === 'MUST' && $keyWasFound === 0) {
-        // print "MUST have $keyMatch but not found.\n";
         $allErrors[] = ["Missing required key", $ancestry, $keyMatch];
       }
     }
@@ -302,11 +329,14 @@ class ArraySchema {
     $expectedTypesArray = explode('|', $expectedTypes);
     $errors = $this->matchesExpectedType($expectedTypesArray, $value, $schema, $ancestry);
     if ($errors && $this->coerce && $this->coerceValue($expectedTypesArray, $keyMatch, $schema, $key, $value, $ancestry)) {
-        // We fixed the type, but check again for other conditions (gt, lte, oneOf etc.)
-        $errors = $this->matchesExpectedType($expectedTypesArray, $value, $schema, $ancestry);
+      // We fixed the type, but check again for other conditions (gt, lte, oneOf etc.)
+      $errors = $this->matchesExpectedType($expectedTypesArray, $value, $schema, $ancestry);
     }
-    if ($errors && $this->removeInvalid) {
-      // @todo
+    if ($errors && is_array($this->fallbacks)) {
+      if ($this->applyFallback($value, $ancestry)) {
+        // Check that the fallback fixed it!
+        $errors = $this->matchesExpectedType($expectedTypesArray, $value, $schema, $ancestry);
+      }
     }
     return $errors;
   }
@@ -357,6 +387,30 @@ class ArraySchema {
     return FALSE;
   }
 
+  /**
+   * @return bool
+   *    TRUE means we found a fallback for the value
+   */
+  protected function applyFallback(&$value, array $ancestry): bool {
+    // See if we can coerce by fallbacks
+    $fb = $this->fallbacks;
+
+    foreach (array_slice($ancestry, 0, -1) as $k) {
+      if (is_array($fb[$k] ?? NULL)) {
+        $fb = $fb[$k];
+      }
+      else {
+        return FALSE;
+      }
+    }
+    $k = end($ancestry);
+    if (array_key_exists($k, $fb)) {
+      $value = $fb[$k];
+      return TRUE;
+    }
+    return FALSE;
+  }
+
   public function formatErrorsAsString(array $errors): string {
     $messages = [];
     foreach ($errors as $error) {

Civi/Inlay/Type.php

@@ -415,16 +415,11 @@ abstract class Type {
       $config = $this->migrateConfig($config);
     }
 
-    // This simply ensures all the defaults exist, and that no
-    // other top-level keys exist. It's the implementation used up-to inlay 1.3.5 so
-    // seems sensible to keep it. Most configs are fairly simple key => scalar types.
-    // You'll need to use a migration if you need to apply new defaults below the top level keys.
-    $this->config = array_intersect_key($config + static::$defaultConfig, static::$defaultConfig);
-
-    // Finally, see if we can validate (with some coercion) the config array
-    // against our schema. This does nothing if we don't override getConfigSchema().
-    $errors = $this->validateConfig(TRUE, FALSE);
-
+    // Finally, see if we can coerce the config array to being valid.
+    // If the config is not validated, the inlay's status is set to 'broken'
+    // and you should inspect your logs for 'critical' errors.
+    $this->validateConfig($config, TRUE, FALSE);
+    $this->config = $config;
 
     return $this;
   }
@@ -449,33 +444,42 @@ abstract class Type {
   }
 
   /**
-   * Check the config we have against a schema, if one exists,
-   * and return any errors.
+   * Check the config we have as best we can.
+   *
+   * Implement getConfigSchema() for a deep check and return errors.
+   * Otherwise, we just merge in and limit to top level keys of $defaultConfig.
    */
-  public function validateConfig($coerce = TRUE, $throw = TRUE): array {
+  public function validateConfig(array &$config, $coerce = TRUE, $throw = TRUE): array {
     $errors = [];
     $schema = $this->getConfigSchema();
     if (!empty($schema)) {
       $validator = new ArraySchema($schema);
       if ($coerce) {
-        $validator->setCoerceToInt()->setRemoveUnexpectedKeys();
+        $validator->setCoerce()->setRemoveUnexpectedKeys()->setFallbacks(static::$defaultConfig);
       }
-      $errors = $validator->getErrors($this->config);
-    }
-    if (!empty($errors)) {
-      $data = [
+      $errors = $validator->getErrors($config);
+      if (!empty($errors)) {
+        $data = [
           'id' => $this->getID(),
           'type' => $this->getTypeName(),
           'errors' => $errors,
           'config' => $this->config,
         ];
-      \Civi::log()->critical("Inlay {id} {type} has invalid config! This could mean it is broken, and could (possibly) affect other Inlays.", $data);
-      $this->instanceData['status'] = 'broken';
-      if ($throw) {
-        throw new \RuntimeException("Invalid configuration in Inlay $data[id] of type $data[type]. See logs.");
+        \Civi::log()->critical("Inlay {id} {type} has invalid config! This could mean it is broken, and could (possibly) affect other Inlays.", $data);
+        $this->instanceData['status'] = 'broken';
+        if ($throw) {
+          throw new \RuntimeException("Invalid configuration in Inlay $data[id] of type $data[type]. See logs.");
+        }
       }
     }
-    elseif (!in_array($this->getStatus(), ['on', 'off'])) {
+    else {
+      // This simply ensures all the defaults exist, and that no
+      // other top-level keys exist. It's the implementation used up-to inlay 1.3.5 so
+      // seems sensible to keep it. Most configs are fairly simple key => scalar types.
+      // You'll need to use a migration if you need to apply new defaults below the top level keys.
+      $config = array_intersect_key($config + static::$defaultConfig, static::$defaultConfig);
+    }
+    if (empty($errors) && !in_array($this->getStatus(), ['on', 'off'])) {
       // If it was broken, and is not broken any more, leave it OFF,
       // for safety. We want an admin to turn it ON.
       $this->instanceData['status'] = 'off';

tests/phpunit/Civi/Inlay/ArraySchemaTest.php

@@ -39,18 +39,18 @@ class ArraySchemaTest extends \PHPUnit\Framework\TestCase /*implements HeadlessI
   protected function runTests(string $setDescr, ArraySchema $a, array $casesAndErrorCounts) {
     // print "\n## $setDescr ##\n\n";
     foreach ($casesAndErrorCounts as $i => $c) {
-      if ("$setDescr$i" === 'coerce to string3') {
-        $x=1;
-      }
       if (count($c) < 3) {
         $c[] = NULL;
       }
       [$input, $errorCount, $result] = $c;
-      $x = $input;
-      $errors = $a->getErrors($x);
+      if ($errorCount && $result) {
+        $this->fail("Invalid test: do not pass a result value with an expected error. $setDescr #$i");
+      }
+      $mutableInput = $input;
+      $errors = $a->getErrors($mutableInput);
       $this->assertCount($errorCount, $errors, "$setDescr #$i Failed with input " . json_encode($input, JSON_UNESCAPED_SLASHES) . "\n" . json_encode($errors));
       if (!$errors && $result !== NULL) {
-        $this->assertSame($result, $x, "$setDescr #$i Failed with input " . json_encode($input, JSON_UNESCAPED_SLASHES) . "\n" . json_encode($errors));
+        $this->assertSame($result, $mutableInput, "$setDescr #$i Failed with input " . json_encode($input, JSON_UNESCAPED_SLASHES) . "\n" . json_encode($errors));
       }
     }
   }
@@ -128,7 +128,7 @@ class ArraySchemaTest extends \PHPUnit\Framework\TestCase /*implements HeadlessI
     );
 
     $wanted = ['i' => 123 ];
-    $this->runTests('coerce',
+    $this->runTests('coerce to int',
       (new ArraySchema(['i' => ['MUST', 'integer']]))->setCoerce(),
       [
         [ $wanted, 0 ],
@@ -136,8 +136,8 @@ class ArraySchemaTest extends \PHPUnit\Framework\TestCase /*implements HeadlessI
         [ ['i' => (double) 123.0 ], 0, $wanted ],
         [ ['i' => (double) 123.1 ], 0, $wanted ],
         [ ['i' => '123.0' ], 0, $wanted ], // Put up with .000
-        [ ['i' => '123.1' ], 1, $wanted ], // If we want an int, we'll put up with it in a string, but we're not having decimals.
-        [ ['i' => '234.1' ], 1, $wanted ], // wrong number completely.
+        [ ['i' => '123.1' ], 1 ], // If we want an int, we'll put up with it in a string, but we're not having decimals.
+        [ ['i' => '234.1' ], 1 ], // wrong number completely.
         [ ['i' => [234] ], 1], // can't cast arrays.
         [ ['i' => NULL ], 0, ['i' => 0]], // null gets cast to zero
       ],
@@ -186,8 +186,35 @@ class ArraySchemaTest extends \PHPUnit\Framework\TestCase /*implements HeadlessI
       ],
     );
 
-  }
+    $as = (new ArraySchema([
+        'i' => ['MUST', 'integer'],
+        'trunk' => ['MAY', 'array', 'schema' => [
+            'branch' => ['MUST', 'boolean'],
+          ]
+        ],
+        'foo' => ['MAY', 'string']
+      ]))->setFallbacks(['i' => 567, 'trunk' => ['branch' => true]]);
+    $this->runTests('coerce to fallbacks', $as,
+      [
+        // #1 fred is not an int, so fallback should be used. trunk not required.
+        [['i' => 'fred'], 0, ['i' => 567]], 
+        // #2 'i' is fine. trunk is given but the branch value is invalid (not a bool)
+        [['i' => 1, 'trunk' => ['branch' => 123]], 0, ['i' => 1, 'trunk' => ['branch' => true]]],
+        // #3 'i' is fine, foo is not and we have no fallback: error.
+        [['i' => 1, 'foo' => 1], 1],
+        // #4 'i' is fine. trunk is given but has no proper branch. The whole trunk should be fallback-ed
+        [['i' => 1, 'trunk' => ['twig' => 1]], 0, ['i' => 1, 'trunk' => ['branch' => true]]],
+      ],
+    );
 
+    $as = (new ArraySchema([ 'i' => ['MUST', 'integer'], ]))
+      ->setFallbacks(['i' => 'invalid fallback!']);
+    $this->runTests('dodgy fallback', $as, [[['i' => 'fred'], 1]]);
 
-  // @todo test coersion and removing unrecognised keys
+  }
+  public function testMissingKeyReplace() {
+    $as = (new ArraySchema([ 'i' => ['MUST', 'integer'], ]))
+      ->setFallbacks(['i' => 123]);
+    $this->runTests('missing key', $as, [[[], 0, ['i' => 123]]]);
+  }
 }