ENH Use FieldValidators for FormField validation

src/Core/Validation/FieldValidation/DateFieldValidator.php

@@ -2,29 +2,114 @@
 
 namespace SilverStripe\Core\Validation\FieldValidation;
 
+use Error;
+use Exception;
+use InvalidArgumentException;
 use SilverStripe\Core\Validation\FieldValidation\FieldValidator;
 use SilverStripe\Core\Validation\ValidationResult;
 
 /**
  * Validates that a value is a valid date, which means that it follows the equivalent formats:
  * - PHP date format Y-m-d
- * - SO format y-MM-dd i.e. DBDate::ISO_DATE
+ * - ISO format y-MM-dd i.e. DBDate::ISO_DATE
  *
  * Blank string values are allowed
  */
 class DateFieldValidator extends FieldValidator
 {
+    /**
+     * Minimum value as a date string
+     */
+    private ?string $minValue;
+
+    /**
+     * Minimum value as a unix timestamp
+     */
+    private ?int $minTimestamp;
+
+    /**
+     * Maximum value as a date string
+     */
+    private ?string $maxValue;
+
+    /**
+     * Maximum value as a unix timestamp
+     */
+    private ?int $maxTimestamp;
+
+    /**
+     * Converter to convert date strings to another format for display in error messages
+     *
+     * @var callable
+     */
+    private $converter;
+
+    public function __construct(
+        string $name,
+        mixed $value,
+        ?string $minValue = null,
+        ?string $maxValue = null,
+        ?callable $converter = null,
+    ) {
+        // Convert Y-m-d args to timestamps
+        // Intermiediate variables are used to prevent "must not be accessed before initialization" PHP errors
+        // when reading properties in the constructor
+        $minTimestamp = null;
+        $maxTimestamp = null;
+        if (!is_null($minValue)) {
+            $minTimestamp = $this->dateToTimestamp($minValue);
+        }
+        if (!is_null($maxValue)) {
+            $maxTimestamp = $this->dateToTimestamp($maxValue);
+        }
+        if (!is_null($minTimestamp) && !is_null($maxTimestamp) && $maxTimestamp < $minTimestamp) {
+            throw new InvalidArgumentException('maxValue cannot be less than minValue');
+        }
+        $this->minValue = $minValue;
+        $this->maxValue = $maxValue;
+        $this->minTimestamp = $minTimestamp;
+        $this->maxTimestamp = $maxTimestamp;
+        $this->converter = $converter;
+        parent::__construct($name, $value);
+    }
+
     protected function validateValue(): ValidationResult
     {
         $result = ValidationResult::create();
         // Allow empty strings
         if ($this->value === '') {
             return $result;
         }
-        // Not using symfony/validator because it was allowing d-m-Y format strings
-        $date = date_parse_from_format($this->getFormat(), $this->value ?? '');
-        if ($date === false || $date['error_count'] > 0 || $date['warning_count'] > 0) {
+        // Validate value is a valid date
+        try {
+            $timestamp = $this->dateToTimestamp($this->value ?? '');
+        } catch (Exception|Error) {
             $result->addFieldError($this->name, $this->getMessage());
+            return $result;
+        }
+        // Validate value is within range
+        if (!is_null($this->minTimestamp) && $timestamp < $this->minTimestamp) {
+            $minValue = $this->minValue;
+            if (!is_null($this->converter)) {
+                $minValue = call_user_func($this->converter, $this->minValue) ?: $this->minValue;
+            }
+            $message = _t(
+                __CLASS__ . '.TOOSMALL',
+                'Value cannot be less than {minValue}',
+                ['minValue' => $minValue]
+            );
+            $result->addFieldError($this->name, $message);
+        } elseif (!is_null($this->maxTimestamp) && $timestamp > $this->maxTimestamp) {
+            $maxValue = $this->maxValue;
+            if (!is_null($this->converter)) {
+                $maxValue = call_user_func($this->converter, $this->maxValue) ?: $this->maxValue;
+            }
+            $message = _t(
+                __CLASS__ . '.TOOLARGE',
+                'Value cannot be greater than {maxValue}',
+                ['maxValue' => $maxValue]
+            );
+            $result->addFieldError($this->name, $message);
         }
         return $result;
     }
@@ -38,4 +123,17 @@ protected function getMessage(): string
     {
         return _t(__CLASS__ . '.INVALID', 'Invalid date');
     }
+
+    /**
+     * Parse a date string into a unix timestamp using the format specified by getFormat()
+     */
+    private function dateToTimestamp(string $date): int
+    {
+        // Not using symfony/validator because it was allowing d-m-Y format strings
+        $date = date_parse_from_format($this->getFormat(), $date);
+        if ($date === false || $date['error_count'] > 0 || $date['warning_count'] > 0) {
+            throw new InvalidArgumentException('Invalid date');
+        }
+        return mktime($date['hour'], $date['minute'], $date['second'], $date['month'], $date['day'], $date['year']);
+    }
 }

src/Core/Validation/FieldValidation/FieldValidationTrait.php

@@ -49,6 +49,18 @@ public function validate(): ValidationResult
         return $result;
     }
 
+    /**
+     * Get the value of this field for use in validation via FieldValidators
+     *
+     * Intended to be overridden in subclasses when there is a need to provide something different
+     * from the value of the field itself, for instance DBComposite and CompositeField which need to
+     * provide a value that is a combination of the values of their children
+     */
+    public function getValueForValidation(): mixed
+    {
+        return $this->getValue();
+    }
+
     /**
      * Get instantiated FieldValidators based on `field_validators` configuration
      */
@@ -61,12 +73,10 @@ private function getFieldValidators(): array
             throw new RuntimeException($message);
         }
         /** @var FieldValidationInterface|Configurable $this */
-        $name = $this->getName();
+        // Use a default name if the field doesn't have one rather than throwing an exception
+        // because some fields may not have a name, i.e. CompositeField
+        $name = $this->getName() ?: _t(__CLASS__ . '.UNNAMEDFIELD', 'Unnamed field');
         $value = $this->getValueForValidation();
-        // Field name is required for FieldValidators when called ValidationResult::addFieldMessage()
-        if ($name === '') {
-            throw new RuntimeException('Field name is blank');
-        }
         $classes = $this->getClassesFromConfig();
         foreach ($classes as $class => $argCalls) {
             $args = [$name, $value];
@@ -122,15 +132,30 @@ private function getClassesFromConfig(): array
             if (!is_array($argCalls)) {
                 throw new RuntimeException("argCalls for FieldValidator $class is not an array");
             }
-            // array_unique() is used to dedupe any cases where a subclass defines the same FieldValidator
-            // this config can happens when a subclass defines a FieldValidator that was already defined on a parent
-            // class, though it calls different methods
-            $argCalls = array_unique($argCalls);
+            $argCalls = $this->makeUnique($argCalls);
             $classes[$class] = $argCalls;
         }
         foreach (array_keys($disabledClasses) as $class) {
             unset($classes[$class]);
         }
         return $classes;
     }
+
+    /**
+     * Dedupe any cases where a subclass defines the same FieldValidator
+     * his config can happens when a subclass defines a FieldValidator that was already defined
+     * on a parent class, though it calls different methods
+     */
+    private function makeUnique(array $argCalls): array
+    {
+        // there may be multiple null values, which we need to retain so make them unqiue first
+        // note that we can't use a short function in combination with $n++ in array_map as $n will always be 0
+        $n = 0;
+        $argCalls = array_map(function ($v) use (&$n) {
+            return is_null($v) ? '{null}-' . $n++ : $v;
+        }, $argCalls);
+        $argCalls = array_unique($argCalls);
+        $argCalls = array_map(fn($v) => str_contains($v, '{null}-') ? null : $v, $argCalls);
+        return $argCalls;
+    }
 }

src/Core/Validation/FieldValidation/MultiOptionFieldValidator.php

@@ -2,7 +2,6 @@
 
 namespace SilverStripe\Core\Validation\FieldValidation;
 
-use InvalidArgumentException;
 use SilverStripe\Core\Validation\ValidationResult;
 use SilverStripe\Core\Validation\FieldValidation\OptionFieldValidator;
 
@@ -12,19 +11,20 @@
 class MultiOptionFieldValidator extends OptionFieldValidator
 {
     /**
-     * @param mixed $value - an array of values to be validated
+     * @param mixed $value - an iterable set of values to be validated
      */
     public function __construct(string $name, mixed $value, array $options)
     {
-        if (!is_iterable($value) && !is_null($value)) {
-            throw new InvalidArgumentException('Value must be iterable');
-        }
         parent::__construct($name, $value, $options);
     }
 
     protected function validateValue(): ValidationResult
     {
         $result = ValidationResult::create();
+        if (!is_iterable($this->value) && !is_null($this->value)) {
+            $result->addFieldError($this->name, $this->getMessage());
+            return $result;
+        }
         foreach ($this->value as $value) {
             $this->checkValueInOptions($value, $result);
         }

src/Core/Validation/FieldValidation/NumericFieldValidator.php

@@ -39,8 +39,14 @@ public function __construct(
     protected function validateValue(): ValidationResult
     {
         $result = ValidationResult::create();
-        if (!is_numeric($this->value) || is_string($this->value)) {
-            // Must be a numeric value, though not as a numeric string
+        if (!is_numeric($this->value)) {
+            $message = _t(__CLASS__ . '.NOTNUMERIC', 'Must be numeric');
+            $result->addFieldError($this->name, $message);
+        } elseif (is_numeric($this->value) && is_string($this->value)) {
+            // This is a separate check from the the one above because for DBField the type is important
+            // though for FormField form submissions values will usually have a string type
+            // though should cast to the correct int or float type before validation
+            // i.e. we don't want to tell CMS users to not use a string when they're using a TextField
             $message = _t(__CLASS__ . '.WRONGTYPE', 'Must be numeric, and not a string');
             $result->addFieldError($this->name, $message);
         } elseif (!is_null($this->minValue) && $this->value < $this->minValue) {

src/Core/Validation/FieldValidation/OptionFieldValidator.php

@@ -31,8 +31,12 @@ protected function validateValue(): ValidationResult
     protected function checkValueInOptions(mixed $value, ValidationResult $result): void
     {
         if (!in_array($value, $this->options, true)) {
-            $message = _t(__CLASS__ . '.NOTALLOWED', 'Not an allowed value');
-            $result->addFieldError($this->name, $message);
+            $result->addFieldError($this->name, $this->getMessage());
         }
     }
+
+    protected function getMessage(): string
+    {
+        return _t(__CLASS__ . '.NOTALLOWED', 'Not an allowed value');
+    }
 }

src/Forms/CompositeField.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Forms;
 
+use SilverStripe\Core\Validation\FieldValidation\CompositeFieldValidator;
 use SilverStripe\Dev\Debug;
 
 /**
@@ -13,6 +14,9 @@
  */
 class CompositeField extends FormField
 {
+    private static array $field_validators = [
+        CompositeFieldValidator::class,
+    ];
 
     /**
      * @var FieldList
@@ -63,7 +67,6 @@ public function __construct($children = null)
             $children = new FieldList($children);
         }
         $this->setChildren($children);
-
         parent::__construct(null, false);
     }
 
@@ -115,12 +118,17 @@ public function getChildren()
         return $this->children;
     }
 
+    public function getValueForValidation(): mixed
+    {
+        return $this->getChildren()->toArray();
+    }
+
     /**
      * Returns the name (ID) for the element.
      * If the CompositeField doesn't have a name, but we still want the ID/name to be set.
      * This code generates the ID from the nested children.
      */
-    public function getName()
+    public function getName(): string
     {
         if ($this->name) {
             return $this->name;
@@ -266,8 +274,6 @@ public function setForm($form)
         return $this;
     }
 
-
-
     public function setDisabled($disabled)
     {
         parent::setDisabled($disabled);
@@ -522,19 +528,4 @@ public function debug(): string
         $result .= "</ul>";
         return $result;
     }
-
-    /**
-     * Validate this field
-     *
-     * @param Validator $validator
-     * @return bool
-     */
-    public function validate($validator)
-    {
-        $valid = true;
-        foreach ($this->children as $child) {
-            $valid = ($child && $child->validate($validator) && $valid);
-        }
-        return $this->extendValidationResult($valid, $validator);
-    }
 }