search

themosis / framework

The Themosis framework core.
https://framework.themosis.com/
GNU General Public License v2.0
670 stars 121 forks source link

Oddity in the Form handleRequest method #874

Closed Juje closed 1 year ago

Juje commented 1 year ago

Hi,

Today I was creating a form and I wanted to have a custom rule on that form which worked perfectly using the setOptions() method on the field and then adding a rule. But the problem came when the rule failed. Because this cleared the invalid value in the form which was really weird and after source diving I found that the Form\Form::handleRequest() uses $this->validator->valid() to get the data to put back into the form. So if a field fails it will not get it's value back. So I locally changed the method to getData() instead and then it worked perfectly. So I'm wondering why valid() was used instead of getData()?

This is the line I'm talking about in the main branch: https://github.com/themosis/framework/blob/511cd1737b451cb69194f47176ebf82ff39cb653/src/Forms/Form.php#L302

jlambe commented 1 year ago

Hi, well I personally expect the form input to be empty if entered data is false with an error message displayed on screen why it is failing. Perhaps we could introduce a way to give the developer the option on how the form should behave with falsy values and call the appropriate validator method on submission.

Juje commented 1 year ago

In some cases I indeed agree. But I'm currently building a config screen for editing a YML file and I don't want to lose the current progress... So ya making it configurable by a developer would indeed make sense.

Juje commented 1 year ago

So should I create a PR or?

jlambe commented 1 year ago

@Juje I will appreciate a PR for sure but if you're willing to pass time on this feature, please make it against the main branch that contains the code for upcoming release. I'm not going to publish a newer version for 2. Or are you on a project that is using version 2 ?

Juje commented 1 year ago

No problem I'll create a PR with pleasure since I need it for a feature :wink:.

And I've been using main for a while now so no problem.

Juje commented 1 year ago

The PR has been created at https://github.com/themosis/framework/pull/878.

Juje commented 1 year ago

The PR has been merged so this issue can be closed :slightly_smiling_face:.