Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log server-side validation errors so it is easier to troubleshoot/debug afform issues #25533

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Feb 8, 2023

Overview

Validation error not always returned to the form making it difficult to debug. Log it so we can easily debug.

Before

Validation error details not returned to form.

After

Validation error detail logged.

Technical Details

Comments

@civibot
Copy link

civibot bot commented Feb 8, 2023

(Standard links)

@civibot civibot bot added the master label Feb 8, 2023
@@ -204,7 +205,7 @@ private static function getRequiredFieldError(string $apiEntity, string $fieldNa
$fullDefn = FormDataModel::getField($apiEntity, $fieldName, 'create');
$isRequired = $attributes['defn']['required'] ?? $fullDefn['required'] ?? FALSE;
if ($isRequired) {
$label = $attributes['defn']['label'] ?? $fullDefn['label'];
$label = $attributes['defn']['label'] ?? $fullDefn['label'] ?? $fieldName;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes label is not set eg. for a select2 and you get the unhelpful error message " is not a required field". This falls back to fieldName so we can work it out.

@@ -68,6 +68,7 @@ protected function processForm() {
\Civi::dispatcher()->dispatch('civi.afform.validate', $event);
$errors = $event->getErrors();
if ($errors) {
\Civi::log()->error('Afform Validation errors: ' . print_r($errors, TRUE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire what about instead \Civi::log()->error('Afform Validation Errors: :errors', [':errors' => $errors])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - that's not the syntax - see https://docs.civicrm.org/dev/en/latest/framework/logging/ - which is based on psr3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so not sure what I'm supposed to do here? Are we happy with what I've put in or should I change it?

@eileenmcnaughton
Copy link
Contributor

I wonder if this is a good time to use a different log prefix - ie

\Civi::log('afform')->error('Afform Validation errors: ' . print_r($errors, TRUE));

The behaviour would be the same by default but sites could configure different behaviour for the log (e.g using the Monolog extension.)

I don't have an opinion on whether extra logging is good - but @colemanw will I think....

@MegaphoneJon
Copy link
Contributor

I've been using this for over a month in production.

@colemanw
Copy link
Member

colemanw commented Jun 8, 2023

@mattwire can you check the review comment by @seamuslee001 above?
I feel like this is ready to merge once that's addressed.

@mattwire
Copy link
Contributor Author

mattwire commented Jun 9, 2023

@eileenmcnaughton I added in the log prefix

@colemanw colemanw merged commit 2af76b5 into civicrm:master Jun 9, 2023
@colemanw
Copy link
Member

colemanw commented Jun 9, 2023

Thanks @mattwire this looks really useful.
Let's merge this & if anyone has further nitpicks they can open a followup PR with their suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants