NEW Show more information in ValidationException message #11562

emteknetnz · 2025-01-16T05:04:40Z

Also added in a bunch of strong typing while was doing it, removed nullable params and instead use blank string to denote "nothing", and changing a string|bool param param to just string

src/Security/Member.php

GuySartorelli

Haven't tested locally yet - but the below stick out to me

GuySartorelli · 2025-02-03T00:21:59Z

src/Core/Validation/ConstraintValidator.php

+     * @param $value The value to validate
     * @param Constraint|Constraint[] $constraints a constraint or array of constraints to validate against
-     * @param string $fieldName The field name the value relates to, if relevant
+     * @param $fieldName The field name the value relates to, if relevant


@param must always include the type. IDEs (and probably our API docs) don't know what to do with a non-typed param and they treat the param name as the type and the first word of the description as the name.

So @param $value The value to validate makes IDEs think $value is the type, and The is the name.

Either remove the @param entirely or revert the change. Personally I'd just remove the $value and $fieldName @params as they're self-explanatory, and only keep the $constraints @param.

This applies to any other PHPDoc changes in this PR.

GuySartorelli · 2025-02-03T00:35:16Z

src/Core/Validation/ValidationException.php

+                if ($this->doShowAdditionalInfo()) {
+                    if ($message['fieldName']) {
+                        $exceptionMessage .= ' - fieldName: ' . $message['fieldName'];
+                    }
+                    $dataClass = $result->getDataClass();
+                    if (is_subclass_of($dataClass, DataObject::class, true)) {
+                        $exceptionMessage .= ', recordID: ' . $result->getRecordID();
+                        $exceptionMessage .= ', dataClass: ' . $dataClass;
+                    }
+                }


Is there ever a reason not to include this additional info in the exception message? e.g. where ValidationException gets thrown for a CMS form submission and $result is a ValidationResult, the actual message isn't parsed by the UI - instead the validation result's individual messages are included in JSON in the response and are used appropriately.

GuySartorelli · 2025-02-03T00:35:56Z

src/Core/Validation/ValidationException.php

+                        $exceptionMessage .= ', recordID: ' . $result->getRecordID();
+                        $exceptionMessage .= ', dataClass: ' . $dataClass;
+                    }
+                }
                break;
            }
        } elseif (is_string($result)) {


Here, where only a string is provided and no ValidationResult, seems to be where the conditional $this->doShowAdditionalInfo() should be used.

We should definitely be adding this extra context here when it's appropriate.

GuySartorelli · 2025-02-03T00:37:53Z

src/Core/Validation/ValidationException.php

+        // e.g. dev/build
+        if (is_a(Controller::curr(), DevelopmentAdmin::class)) {
+            return true;
+        }


Why specifically that controller? I feel like it makes more sense to have a configurable deny list, and say "If the controller is in the deny list, return false" (e.g. LeftAndMain and maybe even up to FormSchemaController should be in the deny list, but if the current controller is front-end or custom, we should assume the extra context is needed unless developers say otherwise)

GuySartorelli · 2025-02-03T00:38:15Z

src/Core/Validation/ValidationResult.php

@@ -16,6 +17,7 @@
 class ValidationResult
 {
    use Injectable;
+    use Configurable;


Why add configurable here?

GuySartorelli · 2025-02-03T00:41:03Z

src/Core/Validation/ValidationResult.php

+    /**
+     * The record ID of the object being validated.
+     */
+    private int $recordID = 0;


In various places we allow mixed for ID (e.g. see the recent updated to SS_List and related, which have mixed $id for the byID() method). We should do the same here - there's no guarantee that all projects use numeric IDs for all of their models.

Someone might have a model (not a subclass of DataObject) that integrates with a third-party service and uses a GUID - and they may want to use a ValidationResult with that model.

Suggested change

private int $recordID = 0;

private mixed $recordID = null;

Please also update the constructor, setter, and getter.

GuySartorelli · 2025-02-03T00:42:45Z

src/Core/Validation/ValidationResult.php

+    protected array $messages = [];
+
+    /**
+     * The class of the DataObject being validated.


Suggested change

* The class of the DataObject being validated.

* The class of the model being validated.

GuySartorelli · 2025-02-03T00:45:56Z

src/Core/Validation/ValidationResult.php

+        if ($thisDataClass === '' && $otherDataClass !== '') {
+            $this->setDataClass($otherDataClass);
+        } elseif ($thisDataClass !== '' && $otherDataClass === '') {
+            $other->setDataClass($thisDataClass);
+        }
+        if ($thisRecordID === 0 && $otherRecordID !== 0) {
+            $this->setRecordID($otherRecordID);
+        } elseif ($thisRecordID !== 0 && $otherRecordID === 0) {
+            $other->setRecordID($thisRecordID);
+        }


We shouldn't be altering $other.

Suggested change

if ($thisDataClass === '' && $otherDataClass !== '') {

$this->setDataClass($otherDataClass);

} elseif ($thisDataClass !== '' && $otherDataClass === '') {

$other->setDataClass($thisDataClass);

}

if ($thisRecordID === 0 && $otherRecordID !== 0) {

$this->setRecordID($otherRecordID);

} elseif ($thisRecordID !== 0 && $otherRecordID === 0) {

$other->setRecordID($thisRecordID);

}

if ($thisDataClass === '' && $otherDataClass !== '') {

$this->setDataClass($otherDataClass);

}

if ($thisRecordID === 0 && $otherRecordID !== 0) {

$this->setRecordID($otherRecordID);

}

GuySartorelli · 2025-02-03T00:51:45Z

src/ORM/DataObject.php

@@ -1257,7 +1257,7 @@ public function forceChange()
     */
    public function validate(): ValidationResult
    {
-        $result = ValidationResult::create();
+        $result = ValidationResult::create(get_class($this), $this->ID);


Is this the only place this gets set in core right now?

What about if I call validate() on a form, and the form belongs to a specific model? E.g. I could conceivably set up a PHP Form for some record that can be submitted from CLI using the fancy symfony console IO functionality.
Personally I think that scenario is likely to contain all the relevant context already, but want to get your thoughts and make sure that sort of thing has been considered.

emteknetnz mentioned this pull request Jan 16, 2025

Expand uncaught ValidationException messages #11423

Open

3 tasks

emteknetnz force-pushed the pulls/6.0/validation-values branch from e148ac2 to 2a63e66 Compare January 16, 2025 05:05

emteknetnz changed the title ~~ENH Pass invalid value to ValidationResult~~ NEW Show more information in ValidationException message Jan 16, 2025

emteknetnz force-pushed the pulls/6.0/validation-values branch from 2a63e66 to 89727e1 Compare January 16, 2025 22:46

emteknetnz closed this Jan 16, 2025

emteknetnz reopened this Jan 16, 2025

emteknetnz force-pushed the pulls/6.0/validation-values branch 5 times, most recently from 6b90450 to 53569d9 Compare January 17, 2025 02:59

emteknetnz commented Jan 17, 2025

View reviewed changes

src/Security/Member.php Show resolved Hide resolved

emteknetnz force-pushed the pulls/6.0/validation-values branch from 53569d9 to dd52956 Compare January 17, 2025 04:15

emteknetnz force-pushed the pulls/6.0/validation-values branch 2 times, most recently from c433cf9 to 263d324 Compare January 30, 2025 03:16

NEW Show more information in ValidationException message

adf3ca1

emteknetnz force-pushed the pulls/6.0/validation-values branch from 263d324 to adf3ca1 Compare January 30, 2025 23:17

emteknetnz marked this pull request as ready for review January 31, 2025 04:03

GuySartorelli requested changes Feb 3, 2025

View reviewed changes

GuySartorelli mentioned this pull request Feb 3, 2025

API Rename FormField Value to getFormattedValue #11550

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Show more information in ValidationException message #11562

NEW Show more information in ValidationException message #11562

emteknetnz commented Jan 16, 2025 •

edited

Loading

GuySartorelli left a comment

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

GuySartorelli Feb 3, 2025

	* The class of the DataObject being validated.
	* The class of the model being validated.

NEW Show more information in ValidationException message #11562

Are you sure you want to change the base?

NEW Show more information in ValidationException message #11562

Conversation

emteknetnz commented Jan 16, 2025 • edited Loading

GuySartorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz commented Jan 16, 2025 •

edited

Loading