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

feat: add Form helpers for Validation Errors #6384

Merged
merged 18 commits into from
Aug 22, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 17, 2022

Description
Fixes #6380
Supersedes #6381

About the bug:

  • The Validation class reads/writes the validation errors in the session for redirect.
  • This behavior is undocumented.
  • But Validation::run() must reset the validation errors in the session.
  • So if a validation runs before the errors output in the form, the validation errors in the session will be deleted.

Changes:

  • add new Form helper functions to get Validation Errors in the Session or shared Validation object
    • validation_errors() to get the validation errors array
    • validation_list_errors() to get the rendered HTML of the validation errors
    • validation_show_error() to get the rendered HTML of the single validation error
  • remove session interaction from the Validation class

How to Use
Controller:

        if (! $this->validate($rules)) {
            return redirect()->back()->withInput();
        }

View:

    <?= validation_list_errors() ?>

or

    <?php $errors = validation_errors(); ?>
    <?php if ($errors): ?>
    <div class="errors" role="alert">
        <ul>
        <?php foreach ($errors as $error): ?>
            <li><?= $error ?></li>
        <?php endforeach ?>
        </ul>
    </div>
    <?php endif ?>

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft August 17, 2022 04:20
@kenjis kenjis added breaking change Pull requests that may break existing functionalities 4.3 enhancement PRs that improve existing functionalities labels Aug 17, 2022
@kenjis kenjis force-pushed the feat-validation-errors branch from 927dc43 to d92376b Compare August 17, 2022 04:25
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Aug 17, 2022
@MGatner
Copy link
Member

MGatner commented Aug 17, 2022

It makes me a little uneasy, but I scoured the docs and we don't actually have the current behavior documented anywhere. This has also caused lots of confusion, and I for one have never taken advantage of the session errors as an actual feature. I do like the new implementation.

My opinion: proceed with the PR and let's get someone else to give an opinion on the potential breaking impact.

@MGatner
Copy link
Member

MGatner commented Aug 17, 2022

@paulbalandan or @iRedds?

@kenjis kenjis marked this pull request as ready for review August 17, 2022 10:38
@kenjis
Copy link
Member Author

kenjis commented Aug 17, 2022

Yes, this behavior is undocumented. So if we think it is not an official feature, this PR is NOT a breaking change,
just an enhancement.

I've asked about the behavior in the forum:
https://forum.codeigniter.com/showthread.php?tid=82735

@iRedds
Copy link
Collaborator

iRedds commented Aug 17, 2022

Unserialize.
I don't understand why validation errors are serialized in the withErrors method. That is, a double serialization is obtained:
error -> serialization -> put to session -> serialization on session save
And when we try to get errors, we also do unserialization twice.

@iRedds
Copy link
Collaborator

iRedds commented Aug 17, 2022

Some might think that the validation_errors() function can directly receive errors from the validator instance.

@iRedds
Copy link
Collaborator

iRedds commented Aug 17, 2022

This is outside the scope of this PR, but I would like to show my vision of dealing with validation errors. I want to say right away that it is based on Laraway.

  1. Make the RedirectResponse::withErrors() method public and add the ErrorBag|array $errors = [] argument.
    This will allow the method to be called without using withInput(), as well as the ability to set errors yourself.
  2. Make it possible for the view to set shared data that cannot be deleted via view(.., [], ['saveData' => false]).
  3. Implement laraway system middleware that will be executed before custom filters (for request) and after custom filters (for response). That is, the principle is the same as the PSR-15 middleware.
  4. Items 2 and 3 will make it possible to add a shared variable with validation errors from the previous request to the view. Without calling an extra function like in this PR.

@paulbalandan
Copy link
Member

I agree with the shared error bag across the application, just like the ShareErrorsFromSession middleware from laravel.

@kenjis
Copy link
Member Author

kenjis commented Aug 17, 2022

Unserialize.

Indeed, but it is outside the scope of this PR.

@kenjis
Copy link
Member Author

kenjis commented Aug 17, 2022

Some might think that the validation_errors() function can directly receive errors from the validator instance.

We can add the fallback to get Validation from Services and return the errors.

@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

We can add the fallback to get Validation from Services and return the errors.

I did.
And added validation_list_errors() helper.

Now we can use exactly the same view file whether we are displaying Form directly or redirecting to Form.

@kenjis kenjis changed the title feat: add validation_errors() form helper feat: add validation_errors() and validation_list_errors() form helper Aug 18, 2022
@kenjis kenjis changed the title feat: add validation_errors() and validation_list_errors() form helper feat: add form helpers for Validation Errors Aug 18, 2022
@kenjis kenjis force-pushed the feat-validation-errors branch 2 times, most recently from cef6fb7 to 7ea834b Compare August 18, 2022 06:49
@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

Added validation_show_error() function.

@MGatner
Copy link
Member

MGatner commented Aug 18, 2022

Big dreams! ErrorBag sounds great but for the issue at hand I like the proposed changes.

The double serialization is a good point - I would like to see that fixed in the same release as this PR, before people start actually using this new feature.

@kenjis
Copy link
Member Author

kenjis commented Aug 18, 2022

The double serialization is a good point - I would like to see that fixed in the same release as this PR, before people start actually using this new feature.

Okay, probably nobody or very few people use this session errors, so I will fix it in this PR tomorrow.

@kenjis
Copy link
Member Author

kenjis commented Aug 19, 2022

Removed serialize(), and added the docs.

@kenjis kenjis force-pushed the feat-validation-errors branch from 50c4ccd to 03d250a Compare August 19, 2022 02:52
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Thanks @kenjis! Looks good to me. Anyone else have remaining comments? Should we start a project card for ErrorBag?

@kenjis kenjis force-pushed the feat-validation-errors branch from 03d250a to 18b52bf Compare August 19, 2022 23:33
@kenjis
Copy link
Member Author

kenjis commented Aug 19, 2022

Rebased and added three commits.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Very nice User Guide write-up! I think this is ready, just added some thoughts for discussion.

@@ -506,6 +500,8 @@ public function setRuleGroup(string $group)

/**
* Returns the rendered HTML of the errors as defined in $template.
*
* You can also use validation_list_errors() in Form helper.
Copy link
Member

Choose a reason for hiding this comment

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

Should we deprecate this function and require the use of the helper? It's always been rather weird that our Validation class has a dependency on View.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is certainly possible to deprecate this method since it duplicates functionality.
If so, what would we do with the error view files and templates setting?
See https://codeigniter4.github.io/CodeIgniter4/libraries/validation.html#configuration

Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like your functions still use those components. What else would you do with them?

Copy link
Member Author

Choose a reason for hiding this comment

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

My question is: If we decouple View from the Validation class, where do we put the validation error templates and its configuration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Counterquestion. Why are error patterns needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's error pattern?

user_guide_src/source/helpers/form_helper.rst Show resolved Hide resolved
user_guide_src/source/installation/upgrade_430.rst Outdated Show resolved Hide resolved
@kenjis kenjis force-pushed the feat-validation-errors branch from 13e93a5 to 8b2a669 Compare August 21, 2022 02:22
Co-authored-by: MGatner <[email protected]>
@kenjis kenjis force-pushed the feat-validation-errors branch from 0abfa49 to f322d5d Compare August 21, 2022 02:36
GET/POST data and the Validation Errors are used together when a validation error occurs.
Therefore, it is not bad that withInput() also saves the validation errors.
@kenjis kenjis force-pushed the feat-validation-errors branch from f322d5d to a12b231 Compare August 21, 2022 02:38
@kenjis
Copy link
Member Author

kenjis commented Aug 21, 2022

I've changed my mind, and removed @TODOs : a12b231

@kenjis kenjis changed the title feat: add form helpers for Validation Errors feat: add Form helpers for Validation Errors Aug 22, 2022
@kenjis kenjis merged commit 7d7d967 into codeigniter4:4.3 Aug 22, 2022
@kenjis kenjis deleted the feat-validation-errors branch August 22, 2022 06:51
@kenjis
Copy link
Member Author

kenjis commented Aug 22, 2022

I've merged to pass the "Test User Guide / Check User Guide syntax".

/github/workspace/user_guide_src/source/incoming/routing.rst:458: WARNING: undefined label: spark-routes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants