Skip to content

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Aug 15, 2022

Fixes #381

redirect()->withInput() or withErrors() saves validation errors in the Session.
And the check of the validation errors in Model (checkQueryReturn()),
$this->validation->getErrors() returns the errors in the Session.

How to Test

--- a/app/Config/Filters.php
+++ b/app/Config/Filters.php
@@ -36,6 +36,7 @@ class Filters extends BaseConfig
             // 'honeypot',
             // 'csrf',
             // 'invalidchars',
+            'session' => ['except' => 'login*'],
         ],
         'after' => [
             'toolbar',
--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -2,10 +2,22 @@
 
 namespace App\Controllers;
 
+use Config\Services;
+
 class Home extends BaseController
 {
     public function index()
     {
-        return view('welcome_message');
+        $this->validation = Services::validation();
+
+        $this->validation->setRules([
+            'email' => 'required|valid_email|unique_email[{user_id}]',
+        ]);
+
+        if (! $this->validation->withRequest($this->request)->run()) {
+            return redirect()
+                ->to('logout')
+                ->withInput();
+        }
     }
 }

@datamweb
Copy link
Collaborator

@kenjis, I think we should merge this PR.
The number of people involved in PR seems to be high.
according to what you did recently for hotfix CI, we can proceed to write the test after the merge.

@jozefrebjak
Copy link
Contributor

+1 for merge now, It's breaking app at all.

@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2022

To merge, we need reviews.

@jozefrebjak
Copy link
Contributor

@kenjis For me this PR solved validation in forms.

@MGatner
Copy link
Member

MGatner commented Aug 16, 2022

I don't like the production use of reflection. It's hard to trace this back in mobile, but couldn't this be solved by handling our validations from a clean instance?

// Validate the registration
if (! single_service('validation')->setRules($this->registrationRules)->run($event)) {
...

@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2022

couldn't this be solved by handling our validations from a clean instance?

No, because the session data ($_SESSION) is global.

See codeigniter4/CodeIgniter4#6381
It helps your understanding.

@MGatner
Copy link
Member

MGatner commented Aug 16, 2022

That helps! Okay I am on board with the Reflection solution. It's a hack but we have a fix in framework at least. Maybe add a note that if the minimum CI4 version is ever increased past 4.2.4 then it can be refactored. Or alternatively, check the actual version and have Reflection be the fallback solution.

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.

See comment above

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

After applying comment MGatner , I don't see any problem.

@kenjis kenjis force-pushed the fix-ValidationException-withErrors branch from 0ede5ff to 886d97f Compare August 17, 2022 02:20
@kenjis kenjis changed the title fix: redirect()->withInput() or withErrors() causes ValidationException fix: redirect()->withInput() causes ValidationException Aug 18, 2022
@kenjis kenjis force-pushed the fix-ValidationException-withErrors branch from 886d97f to f0b5512 Compare August 21, 2022 07:46
@kenjis kenjis requested a review from MGatner August 21, 2022 07:47
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.

👍

@kenjis kenjis merged commit fddc794 into codeigniter4:develop Aug 21, 2022
@kenjis kenjis deleted the fix-ValidationException-withErrors branch August 21, 2022 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants