Cphalcon: [BUG]: Form Populating Entity Before Validation

[davidofferman]

According to the documentation, calling the method Form::isValid() with an entity will only populate the entity after the validation is successful. I'm not sure if the documentation is wrong or the code.

Validates the form. The first element is the data that has been provided by the user. This is usually the $_POST array. There is an optional entity parameter, which, if passed, will be populated by the user input after all validations and filtering have been completed.

https://docs.phalcon.io/4.0/en/forms#methods

Problem Code
https://github.com/phalcon/cphalcon/blob/1314654047d9002a77d554a9381642fe71762bc3/phalcon/Forms/Form.zep#L561-L567

To Reproduce

$entity = new stdClass;

$input = new Phalcon\Forms\Element\Text("test");
$input->addValidator(new Phalcon\Validation\Validator\Alpha());

$form = new Phalcon\Forms\Form();
$form->add($input);

$form->isValid(["test" => "123"], $entity);
print_r($entity);

Expected behavior
I expect the entity to not have any values set.

The expected behavior is useful when the model has setters that throw exceptions on invalid data. The form will validate before trying to set invalid values that would throw an exception.

Details

• Phalcon version: 4.0.0
• PHP Version: 7.3.8

[ruudboon]

I think this a documentation issue. Looking at the code we need a rewrite to achieve this. @niden Do you agree?

[niden]

@davidofferman Here is what I see.

I pass an array ($_POST) and an entity (stdClass) to isValid

From the lines you highlighted:

• The code checks if the entity is an object
  
  
  
  • If yes it calls bind()
  
  
• bind loops through the _POST array and processes each entry by name
  
  
  
  • checks if the element with that name exists
  
  
  • if yes it checks if it is whitelisted (if yes it is not processed)
  
  
  • Sanitizes the value based on the filters assigned to the element with the same name
  
  
  • If there is a setter with that name in the entity, it is called and we continue the loop
  
  
  • if not then the filtered property is assigned to the entity
  
  

I am pretty sure that this has not changed between v3 and v4. If we are to change this behavior it will break backwards compatibility.

So from what I see the docs are correct and align with the code - unless I am misreading the code.

Now you want to be able to have the entity unaffected by the validation. I guess you can not pass the entity at all in there so it will not be changed. Not trying to be a smart butt here but wouldn't that satisfy the expected behavior you describe? Any more use cases you can offer so that I can understand better your way of thinking about this?

[davidofferman]

@niden, that's exactly what I started doing, using stdClass as an entity parameter.

I think you're correct. I misread the documentation as "if the validation passed", not if an entity is passed to the isValid() method.

[ruudboon]

Maybe we should clarify the docs a bit. The docs can be interpreted that the passed entity will only be set if validation is passing and otherwise won’t be touched.
Looks like regardless the outcome we always populate the passed entity.

[niden]

Actually the documents needs to be a bit more clear.

Once you pass the entity, (if an object) it will be populated using bind. Using the same entity, the beforeValidation event will be called on it and after that all the validators will run on the form using the same entity (the altered one).

We can easily create a clone of the entity and not populate it, while preserving running the validators on it but that will break backwards compatibility.

I think the best way forward with this is to explain the whole process thoroughly, as I have done above, in the documentation so that developers know what to expect and potentially (if they want to keep the original object) clone it before passing it in the isValid.

Thoughts? @davidofferman @ruudboon

[niden]

https://docs.phalcon.io/4.0/en/forms

[ruudboon]

@niden Perfect! Looks clear to me now. Closing this. @davidofferman feel free to reopen if you think we missed something.