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

Form Validation and Entity are not playing well together #14203

Closed
scrnjakovic opened this issue Jun 24, 2019 · 1 comment · Fixed by #15752
Closed

Form Validation and Entity are not playing well together #14203

scrnjakovic opened this issue Jun 24, 2019 · 1 comment · Fixed by #15752
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@scrnjakovic
Copy link
Contributor

If you pass both data and entity parameters to $form->isValid(), even though you expect $validation->getValue($field) to fallback to data if entity does not have that value (like From does), it doesn't happen. Validation will not try to read value from any other data source if entity is present.

For an example if you implemented CSRF check through validation, that wont work. That is, unless you have some sick, twisted desire to have a CSRF getter in your entities.

If you split dateOfBirth into three fields, glue them back and set to entity after validation, and you attach i.e. presence validators to those fields, the form will still fail validation if you didn't implement getters for those three fields in your entity. That's shame because it forces you to polute the entity with getters you don't need as we can utilize form's getters for displaying calculated values i.e. $form->getMonth().

/**
* Gets the a value to validate in the array/object data source
*
* @param string field
* @return mixed
*/
public function getValue(string field)
{
var entity, method, value, data, values,
filters, fieldFilters, dependencyInjector,
filterService, camelizedField;
let entity = this->_entity;
// If the entity is an object use it to retrieve the values
if typeof entity == "object" {
let camelizedField = camelize(field);
let method = "get" . camelizedField;
if method_exists(entity, method) {
let value = entity->{method}();
} else {
if method_exists(entity, "readAttribute") {
let value = entity->readAttribute(field);
} else {
if isset entity->{field} {
let value = entity->{field};
} else {
let value = null;
}
}
}
}
else {
let data = this->_data;
if typeof data != "array" && typeof data != "object" {
throw new Exception("There is no data to validate");
}
// Check if there is a calculated value
let values = this->_values;
if fetch value, values[field] {
return value;
}
let value = null;
if typeof data == "array" {
if isset data[field] {
let value = data[field];
}
} else {
if typeof data == "object" {
if isset data->{field} {
let value = data->{field};
}
}
}
}
if typeof value == "null" {
return null;
}

Another thing that is giving me troubles is the fact that $form->isValid($data, $entity) automatically calls $this->bind($data, $entity) but doesn't accept whitelist parameter, forcing you to call $form->isValid($data) and then $form->bind($data, $entity), which in turn makes the entity unavailable to validators because $form->isValid() passes entity to validation class only if it was provided to the function directly, it does not pass entity from $this->_entity as a fallback.

So my proposal is following:

  1. Rewrite Phalcon\Validation::getValue() method so that it tries to retrieve value from the array as well, before it gives up when it can't find one in the entity. But it should not write back filtered values if they came from an array while entity is present, as validation is unaware of whitelisting.

  2. Rewrite Phalcon\Forms\Form::isValid() to accept whitelist as a third parameter which will be passed onto Phalcon\Forms\Form::bind()

@niden niden added 4.1 labels Jun 29, 2019
@niden niden added this to To do in 4.1 Release via automation Jun 29, 2019
@niden niden added bug A bug report status: medium Medium and removed Bug - Medium labels Dec 23, 2019
@ruudboon ruudboon added this to Backlog in Phalcon Roadmap Dec 24, 2019
@niden niden removed this from To do in 4.1 Release Dec 24, 2019
@Jeckerson Jeckerson added 4.1.1 The issues we want to solve in the 4.1.1 release and removed 4.1.0 labels Dec 14, 2020
@Jeckerson Jeckerson added 5.0 The issues we want to solve in the 5.0 release and removed 4.1.1 The issues we want to solve in the 4.1.1 release labels Mar 26, 2021
@niden niden moved this from Backlog to Active Sprint Tasks in Phalcon Roadmap Mar 28, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 2, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 2, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 2, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 3, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 4, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 4, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 4, 2021
@niden niden linked a pull request Nov 5, 2021 that will close this issue
5 tasks
@niden niden moved this from Active Sprint Tasks to Working on it in Phalcon Roadmap Nov 5, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
@BeMySlaveDarlin BeMySlaveDarlin linked a pull request Nov 7, 2021 that will close this issue
5 tasks
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
BeMySlaveDarlin pushed a commit to BeMySlaveDarlin/cphalcon that referenced this issue Nov 7, 2021
@niden
Copy link
Sponsor Member

niden commented Nov 7, 2021

Resolved in #15752

@niden niden closed this as completed Nov 7, 2021
Phalcon Roadmap automation moved this from Working on it to Implemented Nov 7, 2021
@niden niden moved this from Implemented to Released in Phalcon Roadmap Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Archived in project
Phalcon Roadmap
  
Released
Development

Successfully merging a pull request may close this issue.

4 participants