Move "required" and "allowEmpty" options from `ValidationRule` to `Valid... #941

Merged
merged 1 commit into from Nov 11, 2012

Projects

None yet

5 participants

@ADmad
Member
ADmad commented Nov 4, 2012

...ationSet`

@markstory markstory commented on the diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationRule.php
-
-/**
- * Checks whether the field failed the `field should be present` validation
- *
- * @param array $data data to check rule against
- * @return boolean
- */
- public function checkRequired($field, &$data) {
- return (
- (!isset($data[$field]) && $this->isRequired() === true) ||
- (
- isset($data[$field]) && (empty($data[$field]) &&
- !is_numeric($data[$field])) && $this->allowEmpty === false
- )
- );
- }
@markstory
markstory Nov 5, 2012 Member

We'll have to document these methods being moved in the migration guide.

@ADmad
ADmad Nov 5, 2012 Member

Yes of course, documentation will follow.

@markstory markstory commented on the diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationRule.php
@@ -259,7 +188,7 @@ public function isUpdate($exists = null) {
*
* @return boolean True if the rule could be dispatched, false otherwise
*/
- public function process($field, &$data, &$methods) {
+ public function process($field, $data, $methods) {
@markstory
markstory Nov 5, 2012 Member

Yay, fewer places using pass by reference is a good thing.

@markstory markstory and 2 others commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
+ * @param boolean|string $isRequired Valid values are true, false, 'create', 'update'
+ * @return void
+ */
+ public function setIsRequired($isRequired) {
+ $this->_isRequired = $isRequired;
+ }
+
+/**
+ * Sets whether a field value is allowed to be empty
+ *
+ * @param boolean|string $allowEmpty Valid values are true, false, 'create', 'update'
+ * @return void
+ */
+ public function setAllowEmpty($allowEmpty) {
+ $this->_allowEmpty = $allowEmpty;
+ }
@markstory
markstory Nov 5, 2012 Member

Do we want combined get/set methods for these properties?

@ADmad
ADmad Nov 5, 2012 Member

Yes, get/set methods would be nicer. How about a common method to get/set the various options, _isRequired, _allowEmpty, _validationDomain ?

@ADmad
ADmad Nov 5, 2012 Member

On second thought perhaps its best to keep separate functions for each, make for a more readable API.

@lorenzo
lorenzo Nov 5, 2012 Member

I would follow our own style and have combined getter and setters like mark suggested

@ADmad
ADmad Nov 5, 2012 Member

@lorenzo I didn't mean 2 functions to get and set each property. What I meant was 1 get/set method each for each of the properties _isRequired, _allowEmpty, _validationDomain so total 3 methods.

In my previous comment I was thinking of using a common function like option($name, $value = null).

@markstory markstory and 2 others commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
$rule->isUpdate($isUpdate);
if ($rule->skip()) {
continue;
}
-
- $checkRequired = $rule->checkRequired($this->field, $data);
- if (!$checkRequired && array_key_exists($this->field, $data)) {
- if ($rule->checkEmpty($this->field, $data)) {
- break;
- }
- $rule->process($this->field, $data, $this->_methods);
+ if ($this->checkEmpty($this->field, $data)) {
@markstory
markstory Nov 5, 2012 Member

The results of this are not going to change, is there a good reason to call this for each rule? Or am I misreading the diff.

@ADmad
ADmad Nov 5, 2012 Member

The results can change per rule of a field. Similar reasoning as I explained above for checkRequired() as _allowEmpty can also take values true, false, 'create', 'update'.

@lorenzo
lorenzo Nov 5, 2012 Member

I guess what mark says is that you can cache the value of checkEmpty, so it is not called per rule.

@markstory markstory and 2 others commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
@@ -120,20 +165,25 @@ public function validate($data, $isUpdate = false) {
$this->reset();
$errors = array();
foreach ($this->getRules() as $name => $rule) {
+ $this->isUpdate($isUpdate);
+ if ($this->checkRequired($this->field, $data)) {
+ $errors[] = __d('cake', 'This field must exist in data');
+ break;
+ }
@markstory
markstory Nov 5, 2012 Member

Do we need to call this method for each rule? Or would once beforehand be sufficient?

@ADmad
ADmad Nov 5, 2012 Member

Yes we do need to call it for each rule. Consider this rules array for a particular field:

'field' => [
    '_isRequired' => 'create',
    'rule1' => [
        'rule' => 'rule1',
        'on' => 'update'
    ],
    'rule2' => [
        'rule' => 'rule2',
        'on' => 'create'
    ]
]

So when the function params $isUpdate is false, for the first rule "rule1" checkRequired() will return false since the rule is applicable only for updates but for "rule2" checkRequired() will return true.

@lorenzo
lorenzo Nov 5, 2012 Member

Hmm, that does not look right to me, it is either required or not required. The only differences hound be that the rule will not be processed if it is not required

@ADmad
ADmad Nov 5, 2012 Member

Okay I will think over it some more and try to refactor.

@markstory markstory and 1 other commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
@@ -145,6 +195,80 @@ public function validate($data, $isUpdate = false) {
}
/**
+ * Sets the recordExists configuration value for this rule,
+ * ir refers to wheter the model record it is validating exists
+ * exists in the collection or not (create or update operation)
+ *
+ * If called with no parameters it will return whether this rule
+ * is configured for update operations or not.
+ *
+ * @return boolean
+ **/
@markstory
markstory Nov 5, 2012 Member

Should be */ (end nitpick) 😄

@ADmad
ADmad Nov 5, 2012 Member

A mistake indeed.

@markstory
Member

Would it also make sense to rename the classes a bit, since we're changing things?

  • Model\Validation\Collection
  • Model\Validation\Rule

Perhaps its not worth the candle to rename classes though.

@ADmad
Member
ADmad commented Nov 5, 2012

I would say keep the class names as they are, they are more easily recognizable this way.

@lorenzo lorenzo commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
@@ -145,6 +195,80 @@ public function validate($data, $isUpdate = false) {
}
/**
+ * Sets the recordExists configuration value for this rule,
+ * ir refers to wheter the model record it is validating exists
@lorenzo
lorenzo Nov 5, 2012 Member

Typo: it instead of ir

@lorenzo lorenzo commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
+ *
+ * @return boolean
+ **/
+ public function isUpdate($exists = null) {
+ if ($exists === null) {
+ return $this->_recordExists;
+ }
+ return $this->_recordExists = $exists;
+ }
+
+/**
+ * Returns whether the field can be left blank according to `allowEmpty`
+ *
+ * @return boolean
+ */
+ public function allowedEmpty() {
@lorenzo
lorenzo Nov 5, 2012 Member

Maybe call it emptyAllowed()? Not sure if that reads better to native English speakers

@lorenzo lorenzo and 1 other commented on an outdated diff Nov 5, 2012
...e/Test/TestCase/Model/Validator/ValidationSetTest.php
+ }
+
+/**
+ * testValidateWithIsRequired method
+ *
+ * @return void
+ */
+ public function testValidateWithIsRequired() {
+ $data = array(
+ 'title' => '',
+ 'body' => 'a body'
+ );
+ $expectedRequired = array('This field must exist in data');
+
+ $Field = new ValidationSet('notthere', array(
+ '_isRequired' => true,
@lorenzo
lorenzo Nov 5, 2012 Member

The "required" word is very confusing, since you are making this big change maybe we want to do something like rails? They use validatePresence, which accurately describes that behavior.

@ADmad
ADmad Nov 5, 2012 Member

Agreed, I too was going to ask for suggestions for a more appropriate name.

@lorenzo lorenzo and 1 other commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
@@ -145,6 +195,80 @@ public function validate($data, $isUpdate = false) {
}
/**
+ * Sets the recordExists configuration value for this rule,
+ * ir refers to wheter the model record it is validating exists
+ * exists in the collection or not (create or update operation)
+ *
+ * If called with no parameters it will return whether this rule
+ * is configured for update operations or not.
+ *
+ * @return boolean
+ **/
+ public function isUpdate($exists = null) {
+ if ($exists === null) {
+ return $this->_recordExists;
+ }
+ return $this->_recordExists = $exists;
@lorenzo
lorenzo Nov 5, 2012 Member

It's function should propagate the isUpdate value to all internal rules

@ADmad
ADmad Nov 5, 2012 Member

So change this to a protected function?

@lorenzo lorenzo and 1 other commented on an outdated diff Nov 5, 2012
lib/Cake/View/Helper/FormHelper.php
foreach ($validationRules as $rule) {
- $rule->isUpdate($isUpdate);
+ $rule->isUpdate($this->requestType === 'put');
@lorenzo
lorenzo Nov 5, 2012 Member

This should not happen, the is update should be propagated in the validation set

@ADmad
ADmad Nov 5, 2012 Member

Ok, then I guess I will remove this whole foreach loop and move it to a function in ValidationSet class itself.

@lorenzo lorenzo and 1 other commented on an outdated diff Nov 5, 2012
lib/Cake/Model/Validator/ValidationSet.php
@@ -120,20 +165,25 @@ public function validate($data, $isUpdate = false) {
$this->reset();
$errors = array();
foreach ($this->getRules() as $name => $rule) {
+ $this->isUpdate($isUpdate);
@lorenzo
lorenzo Nov 5, 2012 Member

This should not happen inside the foreach, IMO

@ADmad
ADmad Nov 5, 2012 Member

Yup, will move that out.

@lorenzo
Member
lorenzo commented Nov 5, 2012

The idea is more than awesome and something that should have been done a long time ago. You are a hero for daring to change this :)

My only concern is BC: I'm sure I won't be the only one having troubles to migrate to 2.3 as cleanly as we did with 2.1 and 2.2. Any ideas to not break people apps as hard as this seems to be?

@ADmad
Member
ADmad commented Nov 5, 2012

@lorenzo Assuming it's even achievable it would be a total mess of a code to try and keep backwards compatibility. That's the reason I targeted this change for 3.0 instead of 2.3.

@markstory markstory and 3 others commented on an outdated diff Nov 6, 2012
...e/Test/TestCase/Model/Validator/ValidationSetTest.php
+ }
+
+/**
+ * testValidateWithvalidatePresence method
+ *
+ * @return void
+ */
+ public function testValidateWithvalidatePresence() {
+ $data = array(
+ 'title' => '',
+ 'body' => 'a body'
+ );
+ $expectedRequired = array('This field must exist in data');
+
+ $Field = new ValidationSet('notthere', array(
+ '_validatePresence' => true,
@markstory
markstory Nov 6, 2012 Member

I like that this property is no longer called required as that was a bit of a mismatch with what the property did. While I think validatePresence makes more sense I wonder if _requireKey or _requirePresent might make for more readable code.


$validate = [
  'title' => ['_requirePresent' => 'create']
];

for example.

@ADmad
ADmad Nov 6, 2012 Member

I am fine with _requirePresent. I will wait a while to hear @lorenzo 's opinion on this or if someone else have one and then update the name.

@dereuromark
dereuromark Nov 6, 2012 Member

i like "_requirePresence" (noun after a verb), but thats just a minor knicknack.

@markstory
markstory Nov 7, 2012 Member

I think using _requirePresence sounds odd next to allowEmpty as the tense is different.

$validate = [
  'title' => [
    '_requirePresent' => true,
    '_allowEmpty' => true
  ]
];

Reads more naturally to me than:

$validate = [
  'title' => [
    '_requirePresence' => true,
    '_allowEmpty' => true
  ]
];

Lastly requirePresent is 1 character shorter 😉 . I'm fine with either in the end, as both are an improvement over 'required'.

@ADmad
ADmad Nov 7, 2012 Member

Hmm I responded to this earlier today but my comment seems to have disappeared. Pretty sure it was successfully submitted. Anyway here it is again (sorry if you guys are getting notification mail again for the same):

I am with @markstory on this one, _requirePresent is the same tense as _allowEmpty so my vote goes to _requirePresent. So the vote tally is 2 against 2 right now but @markstory is a native speaker so unless someone else has an opinion I say the scale is tipped in _requirePresent 's favor 😄

@markstory markstory and 1 other commented on an outdated diff Nov 6, 2012
lib/Cake/Test/TestCase/View/Helper/FormHelperTest.php
'between' => array('rule' => array('between', 5, 30)),
),
- 'imalsonotrequired2' => array(
- 'alpha' => array('rule' => 'alphaNumeric', 'allowEmpty' => true),
- 'between' => array('rule' => array('between', 5, 30), 'allowEmpty' => true),
+ 'imnotrequiredeither' => array(
+ '_isRequired' => true,
@markstory
markstory Nov 6, 2012 Member

Looks like a few _isRequired got left behind.

@ADmad
ADmad Nov 6, 2012 Member

I will just remove it from helper tests. Adding the "required" css class only depends on value of "allowEmpty" anyway. That's why I didn't get any fails for test cases. No point in having unneeded options.

@lorenzo
Member
lorenzo commented Nov 6, 2012

@ADmad I did not realize this was for 3.0... My only concern so far is that we don't actually know how the model system will be like for sure at the end, but I guess we will be able to adapt it since it is a separate set of classes already :)

I think this is a case that would be nice for 2.x too, will ponder some options to backport it at some point

@markstory
Member

@lorenzo I don't think model validation will change dramatically in 3.0 though. We have a fairly robust validation system these days and outside of the problems this change would address I'm not aware of any big problems in it.

@ADmad
Member
ADmad commented Nov 7, 2012

@lorenzo Yup the validator classes are not directly tied to models in any way. We only pass in the rules and methods for validation. Even if the ORM changes I doubt the way we do validation will change.

Perhaps keeping BC with 2.x would be possible. Since the existing 'required' and 'allowEmpty' keys are always supposed to be in the first rule we can either have ModelValidatior manipulate the Model::$validate array before passing to ValidationSet or do it in ValidationSet::__construct() itself.
Problem would occur only if someone was manipulation the existing CakeValidationRule rule objects at runtime to change "required" and "allowEmpty".
Edit: Plus the obvious fact that many methods in the 2 classes have been removed or changed.

@markstory
Member

I think this looks pretty good 😄

@ADmad
Member
ADmad commented Nov 10, 2012

@markstory I will wait till tomorrow in case someone else has any comments and then squash the commits and merge it in.

@lorenzo
Member
lorenzo commented Nov 11, 2012

Looks good to me

@ADmad ADmad merged commit db4bf84 into cakephp:3.0 Nov 11, 2012
@renan

Shouldn't it be checkValidatePresent?

Member

yip, will fix it.

@renan

Perhaps also change $exists to a conformant name?

@ADmad ADmad deleted the ADmad:3.0-validation branch May 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment