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

fix: feature test with validation #7548

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 7, 2023

Description
See https://forum.codeigniter.com/showthread.php?tid=87844

Feature testing shares the Validation object between requests.
So the validation result is incorrect after a failed validation.

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 added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 7, 2023
@iRedds
Copy link
Collaborator

iRedds commented Jun 8, 2023

I was sure that we added an error reset to the Validation::run() method. So that when you call it again, even if the validation succeeds, it does not return false.
But I couldn't find it.
For example, in a model, if validation is available, then the state is reset before each run.
Maybe it would be more correct to reset the state directly when running the Validation::run() method?

@kenjis
Copy link
Member Author

kenjis commented Jun 8, 2023

@iRedds Do you mean this? #5861

@kenjis
Copy link
Member Author

kenjis commented Jun 8, 2023

Maybe it would be more correct to reset the state directly when running the Validation::run() method?

We can rest $this->errors at the top of the run() method. But cannot reset others ($this->data, $this->rules, $this->customErrors).

@iRedds
Copy link
Collaborator

iRedds commented Jun 8, 2023

@iRedds Do you mean this? #5861

In principle we can do the same.

We can rest $this->errors at the top of the run() method. But cannot reset others ($this->data, $this->rules, $this->customErrors).

But we only need to reset the errors.

@michalsn
Copy link
Member

michalsn commented Jun 8, 2023

Maybe it would be more correct to reset the state directly when running the Validation::run() method?

This makes sense in theory, but what if people have more than one form on a page and send everything to the same controller?

Each form goes into a separate class where it is validated, and errors are returned at the same time. If we reset the errors every time the run() method is called, we will lose errors from previous validations. This may cause some problems.

@kenjis
Copy link
Member Author

kenjis commented Jun 8, 2023

@michalsn How do we send two forms at the same time?

@kenjis
Copy link
Member Author

kenjis commented Jun 8, 2023

But we only need to reset the errors.

The setRule() actually adds a rule when it specifies a new field.

@kenjis
Copy link
Member Author

kenjis commented Jun 8, 2023

I believe this solution is the best for now. I merge.
If there is something wrong or a better solution, send another PR.

@kenjis kenjis merged commit 470c41f into codeigniter4:develop Jun 8, 2023
@kenjis kenjis deleted the fix-feature-test-validation branch June 8, 2023 23:26
@michalsn
Copy link
Member

michalsn commented Jun 9, 2023

@kenjis With a little JavaScript, it's possible, but I explained it poorly since I didn't mean the JS example.

What I meant is one form with fields like:

form1[field1]
form1[field2]

form2[field1]
form2[field2]

which will be sent to one endpoint and then handled via separate classes.

@kenjis
Copy link
Member Author

kenjis commented Jun 10, 2023

@michalsn What do you mean by via separate classes?

If it means one Validation object validates the form1 data and another Validation object does the form2 data,
it seems we will not lose errors.

Maybe separate classes but they use a shared Validation object?

@michalsn
Copy link
Member

@kenjis

Maybe separate classes but they use a shared Validation object?

Yes, exactly this.

@iRedds
Copy link
Collaborator

iRedds commented Jun 10, 2023

@michalsn Maybe I do not quite understand, but it seems to me that your example looks like bad architecture. + Wrong decision to make a shared validator instance.

@kenjis
Copy link
Member Author

kenjis commented Jun 10, 2023

@michalsn I too think such usage is too tricky and kind of misuse.

@michalsn
Copy link
Member

@iRedds

Maybe I do not quite understand, but it seems to me that your example looks like bad architecture

Like having more than one form on a single page or handling the validation of each form in a separate class?

The context is important because each form is also used separately (alone) in different places of the application. So it makes code reusable and easy to work with.

Wrong decision to make a shared validator instance.

Using a shared validation instance has its advantages, like the ability to use form helper functions (related to displaying error messages). Personally, I see no issues with using a shared instance as long as you know what you're doing.

@kenjis

I too think such usage is too tricky and kind of misuse.

I'm not sure if this behavior was intended, but I can see cases when bending the rules can work out for us. If I had to implement something as I described now, with CI4, I would probably take advantage of this "feature".

The idea was to show that other developers can use the validation class in very different ways and in my opinion, we should leave this class as flexible as possible. Forcing to use reset() would be the opposite of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants