juergenweb / FrontendForms

A module for ProcessWire CMS to create and validate forms on the frontend easily using the Valitron library.
MIT License
15 stars 1 forks source link

`setMinTime(), setMaxTime()` both MUST be `0` to disable them - error if only one is `0`. #10

Closed donatasben closed 2 months ago

donatasben commented 2 months ago

I was experiencing an issue when form threw an error after second attempt to post the same form (if first attempt didn't pass validation). I came to conclusion that this happens if you set only one of setMinTime(), setMaxTime() to zero. Then the encryptDecrypt() function receives NULL instead of expected string and errors out on second attempt to post form.

How to reproduce:

  1. Create form with some mandatory fields
  2. Set setMinTime(2)
  3. Set setMaxTime(0)
  4. Try to fill the form and omit one required field, post.
  5. Receive "form invalid" message
  6. Fill all the required fields, post
  7. Receive an error

SCR-20240709-msin

Or sometimes

SCR-20240709-mrmh

Both seem to be related, as it tries to calculate the time diff.

"-load_time is not present in form" might happen because I'm using a custom AJAX js function to load form with custom hooks.

But if I set both setMinTime() and setMaxTime() to some value - or both to 0 - it works fine.

Request is to make it clearer in the docs about this behaviour, or account for single 0 value in the module code.

Thanks!

juergenweb commented 2 months ago

Hello @donatasben

I have tried to reproduce your error messages but everything works on my side.

The error messages say that there is no hidden field "load-time" present in the form. This is only the case if there is no time measurement at all (min-time and max-time = 0) and this is fine in this case. If at least one of them is set, the hidden field must be present.

if (($this->getMinTime()) || $this->getMaxTime()) { $hiddenField4 = new InputHidden('load_time'); $hiddenField4->setAttribute('value', self::encryptDecrypt((string)$this->load_time)); $this->add($hiddenField4); }

If it is not there, getMinTime() and getMaxTime() must be 0, null or ''. That must be the reason in your case, so it would be interesting what values will be returned for this. You can use Tracy Debugger to check the values before the if-condition.

I am 100% sure, that there is something going on with your Ajax call or your hook (but I do not know what). I have tried to reproduce the issue, but without success.

I came to conclusion that this happens if you set only one of setMinTime(), setMaxTime() to zero.

No that is not the case. Min and max time are completely independent of each other, so it has no effect if the one or the other is 0.

Sorry, that I cannot help you, but I guess this issue is completely related to your setup and impossible for me find out what is going on. :-(

donatasben commented 2 months ago

Probably you are right @juergenweb. It might be just my implementation. With my AJAX call I actually render the requested page (form action url = current page) with a hook that prints only the FrontendForms output, skips the rest of the page as full page might be quite large, to save loading speed. AJAX posts all the fields as it should, the form render uses the same setup PHP code that is used to render the form initially on the page.

The only thing I changed to fix my issue was setting both setMinTime() and setMaxTime() to 0, or both to non-0 values. I tested this several times on my website thus I came to this conclusion.

I tried this after reading this in FF docs:

Good to know, but you do not have to do something special The value entered for the min time refers to filling out all empty required fields of a form. If some fields contain a value after submission, the time will be reduced for filling out the form, because there are less fields left. So checking the min time takes care about how manyfields are filled out at the time of submission. This leads to that, that the min time will be reduced, if there a less fields left. So do not be surprised, if the the min time changes during multiple submission attempts.

My values were: setMinTime(2) and setMaxTime(0) and there were several fields in the form. After submit and validation there was one required field left unfilled - then maybe the calculation to reduce "minTime" was causing it to be maybe too small and erroring out somewhere. Or maybe it gets rounded to a 0 and disables this time diff functonality all-together - and doesn't render the "load_time" input... Or maybe the too small "minTime" amount becomes the null value at some point in the code... Couldn't spare the time to test with a fresh minimal form setup code for now.

SCR-20240709-mrgj
juergenweb commented 2 months ago

Hello @donatasben

Or maybe it gets rounded to a 0 and disables this time diff functonality all-together - and doesn't render the "load_time" input... Or maybe the too small "minTime" amount becomes the null value at some point in the code...

These are interesting aspects:

One thing I can say is that it will never become null after the calculation, because it is typehinted:

$numberOfRequiredFieldWithValues = count(array_filter($requiredFields)); // calculate new min time depending on the required fields, which has no value at the moment if ($numberOfRequiredFieldWithValues) { $newMinTime = (round(($this->form->getMinTime() * (count($requiredFields) - $numberOfRequiredFieldWithValues)) / count($requiredFields))); $this->form->setMinTime((int)$newMinTime); // typehinted }

What we can try is to change the value to at least to "1", so if the rounded calculation will have a result smaller than "1" (eg. 0,5 or 0), than set it to 1. To test this in your case please go to the FormValidation.php which you will find under Formelements/FormValidation.php and change the code inside the checkTimeDiff() method near line 80 to this one:

// calculate new min time depending on the required fields, which has no value at the moment if ($numberOfRequiredFieldWithValues) { $newMinTime = (round(($this->form->getMinTime() * (count($requiredFields) - $numberOfRequiredFieldWithValues)) / count($requiredFields))); if($newMinTime < 1) $newMinTime = 1; // this is new $this->form->setMinTime((int)$newMinTime); }

There is a new line inside this code:

if($newMinTime < 1) $newMinTime = 1;

This line sets the minTime at least to 1 second, so it could never become 0.

If it works, I will add it to the next update. If not, I need to know what value does the minTime and maxTime have before the error occures. In this case please run a Tracy bd() call (I guess you have Tracy Debugger installed ;-)) inside the Form.php at around line 2296 (This is the part where the error propably occured):

//create hidden field to send the timestamp (encoded) when the form was loaded bd($this->getMinTime()); bd($this->getMaxTime()); if (($this->getMinTime()) || $this->getMaxTime()) { $hiddenField4 = new InputHidden('load_time'); $hiddenField4->setAttribute('value', self::encryptDecrypt((string)$this->load_time)); $this->add($hiddenField4); }

Only to mention: The encryption of the loading time is only for security reasons, so no one can manipulate the loading time inside the console.

Please let me know what happens.

Jürgen

donatasben commented 2 months ago

Hi @juergenweb I was able to recreate the issue with a simple clean form (based from the module examples) by setting setMinTime(1) and setMaxTime(0); then leaving one field empty of multiple required ones when filling the form. After failed validation notice I tried to fill that field and there it was again - the error in encryptDecrypt() function, that receives a null instead of expected string: SCR-20240709-msin

But using your suggested addition f($newMinTime < 1) $newMinTime = 1; fixes the issue!

I dumped the values for if (($this->getMinTime()) || $this->getMaxTime()) and yes - the getMinTime() gets reduced to 0 that causes the -load_time input to not being rendered. If I set in the form: setMinTime(1), then the value gets rounded to 0.0 if small percent of fields are left unfilled after initial validation.

All in all - your suggested fix solves this issue.

juergenweb commented 2 months ago

Interesting! I have been never reached such an edge-case-scenario, therefore I have never got such an error message, but glad that we could solve this issue together. :-) You can leave the code changes inside your site and I will add it to Github, so it will be in the code at the next update automatically.