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

src/Core/Validation/ConstraintValidator.php

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.

I don't quite understand what you mean by this

There's a call to $this->doShowAdditionalInfo() above which checks that we're either in a CLI context or the current controller is Development Admin

The rest of the time, for instance when doing XHR form submissions, the fieldname, recordID and dataClass aren't useful to return because they are already known by the form/field that's being submitted

What I mean is, why do we need doShowAdditionalInfo() at all?
This discussion can ultimately be handled in #11562 (comment)

As mentioned in #11562 (comment) I think we need to keep doShowAdditionalInfo()

src/Core/Validation/ValidationException.php

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)

The point of this PR is to add additional info to the error message string, e.g. fieldName. fieldName is already part of ValidationResult. However in a CLI or dev/build context, you simply get the error message outputted to the terminal/screen - you don't see the rest of the metadata in the ValidationResult object.

We don't want the fieldname being suffixed to the error message string in a UX context because then you'd end up with a validation message like "Invalid email address - fieldname: Email" showing up underneath the Email field which already has a label "Email"

In a UX context (at least in the CMS) the exception message isn't used, the validation result messages are accessed directly, so the additional context does not display in the edit form.

I think this condition of "where are we? Should we add the info?" isn't needed - it should just be always added to the exception message.

Looking at LeftAndMain::handleRequest() - it's using $e->getMessage(), which a native PHP method for Exception. In ValidationException, whose parent class is Exception, we're calling parent::__construct($exceptionMessage, $code);

LeftAndMain::handleRequest is obviously a pretty central place, so it seems pretty likely this additional info will popup on the frontend where we don't intend it to.

I've upgraded the the allow list to make it configurable

src/Core/Validation/ValidationResult.php

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.

A Form has no awareness of a DataObject, it simply keeps track of key=value style data. The only time it interacts with DataObject is when calling Form::saveInto(DataObjectInterface $dataObject)

It's the same story with most the other things that call ValidationResult::create() and could be related to DataObjects, e.g. FormField, ConstraintValidator, FieldValidator

MemberAuthenticator and PasswordValidator do deal with Member DataObjects, though I don't we shouldn't be displaying any values for those in a CLI context

I've updated the PR to remove the constructor arguments as that functionality is basically only for DataObjects and not for other thing contexts ValidationResult is used in such as Forms. I've updated DataObject::validate() to call $result->setModelClass(static::class); and $result->setRecordID($this->ID); instead

I've updated the PR to remove the constructor arguments as that functionality is basically only for DataObjects

I think the additional context should still be settable in the constructor. It's useful for any scenario in a project where you're setting validation results on any model and you don't expect the result to be displayed in a normal CMS edit context. i.e. projects can use this the same way we're using it in DataObject.

That said, the setters are public so we're not locking people out of that, just making the functionality a little harder to discover - so I won't block merging if that isn't done.

A Form has no awareness of a DataObject

Form has a $record property which gets populated when $form->loadDataFrom() is called.

MemberAuthenticator and PasswordValidator do deal with Member DataObjects, though I don't we shouldn't be displaying any values for those in a CLI context

I assume you meant "I think we shouldn't" - in which case I agree, but none of this functionality deals with values. It deals with model classes and IDs.

Oh yeah you're right of course Form is sometimes aware of DataObject via loadDataFrom().

Still that's not the bit of Form that actually triggers a ValidationException, that happens in FormRequestHandler::httpSubmission()

In that context we could possibly (haven't tested) access the DataObject via $this->form->getRecord(), however however I don't think it makes sense to here as the relevant code will extract the ValidationResult out of the ValidationException, and use that to make a response

Re constructor without arguments, I'd like to keep that as is in this PR.

I don't think there's any further changes required here

GuySartorelli · 2025-02-06T20:54:33Z

src/Forms/Form.php

+     * @param $message the text of the message
+     * @param $type Should be set to good, bad, or warning.
+     * @param $cast Cast type; One of the CAST_ constant definitions.


Need the types as per #11562 (comment)

GuySartorelli · 2025-02-06T20:54:40Z

src/Forms/Form.php

+     * @param $message the text of the message
+     * @param $type Should be set to good, bad, or warning.
+     * @param $cast Cast type; One of the CAST_ constant definitions.


Need the types as per #11562 (comment)

GuySartorelli · 2025-02-06T21:15:05Z

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;
+                    }
+                }


What I mean is, why do we need doShowAdditionalInfo() at all?
This discussion can ultimately be handled in #11562 (comment)

src/Core/Validation/ValidationException.php

GuySartorelli · 2025-02-06T21:18:22Z

src/Core/Validation/ValidationException.php

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


In a UX context (at least in the CMS) the exception message isn't used, the validation result messages are accessed directly, so the additional context does not display in the edit form.

I think this condition of "where are we? Should we add the info?" isn't needed - it should just be always added to the exception message.

GuySartorelli · 2025-02-06T21:24:03Z

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);


I've updated the PR to remove the constructor arguments as that functionality is basically only for DataObjects

I think the additional context should still be settable in the constructor. It's useful for any scenario in a project where you're setting validation results on any model and you don't expect the result to be displayed in a normal CMS edit context. i.e. projects can use this the same way we're using it in DataObject.

That said, the setters are public so we're not locking people out of that, just making the functionality a little harder to discover - so I won't block merging if that isn't done.

GuySartorelli · 2025-02-06T21:27:54Z

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);


A Form has no awareness of a DataObject

Form has a $record property which gets populated when $form->loadDataFrom() is called.

MemberAuthenticator and PasswordValidator do deal with Member DataObjects, though I don't we shouldn't be displaying any values for those in a CLI context

I assume you meant "I think we shouldn't" - in which case I agree, but none of this functionality deals with values. It deals with model classes and IDs.

GuySartorelli · 2025-02-06T21:30:43Z

src/ORM/DataObject.php

        $result = ValidationResult::create();
+        $result->setModelClass(static::class);
+        $result->setRecordID($this->ID);


Since these aren't available through the constructor anymore, calling two methods to set a single model feels a bit off. Maybe there should be a $result->setModel($this);?
Don't have to implement this, just a thought.

I think it makes sense to keep separate as you may want to set a non-numeric recordID that doesn't relate to a DataObject/model. I don't want to add what is essentially a duplicate method

I don't understand how having an ID without knowing what class that ID belongs to would ever be helpful, but probably not worth discussing since the methods are there to set the stuff.

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 3 times, most recently 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

emteknetnz force-pushed the pulls/6.0/validation-values branch 2 times, most recently from b6220e8 to 6123954 Compare February 4, 2025 22:07

GuySartorelli requested changes Feb 6, 2025

View reviewed changes

GuySartorelli reviewed Feb 6, 2025

View reviewed changes

NEW Show more information in ValidationException message

a0bbb38

emteknetnz force-pushed the pulls/6.0/validation-values branch from 6123954 to a0bbb38 Compare February 6, 2025 23:28

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

emteknetnz Feb 4, 2025

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025

GuySartorelli Feb 3, 2025

emteknetnz Feb 4, 2025

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025

GuySartorelli Feb 3, 2025

emteknetnz Feb 4, 2025

emteknetnz Feb 4, 2025

GuySartorelli Feb 6, 2025

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025 •

edited

Loading

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025

GuySartorelli Feb 6, 2025

GuySartorelli Feb 6, 2025

GuySartorelli Feb 6, 2025

GuySartorelli Feb 6, 2025

GuySartorelli Feb 6, 2025

emteknetnz Feb 6, 2025

GuySartorelli Feb 6, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Feb 6, 2025 • edited Loading

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

Choose a reason for hiding this comment

emteknetnz commented Jan 16, 2025 •

edited

Loading

emteknetnz Feb 6, 2025 •

edited

Loading