Proposal: Custom Validation Objects #912

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
5 participants
@marcopeg

I've packed up a solution to use custom validation rules from outside the model class namespace.

I was inspired by a discussion and code-posting I found here (http://goo.gl/8u4FN) between MarkStory and saleh.

I changed /lib/Cake/Model/CakeValidationRule.php adding a code block to process() method to search for custom rules into external objects.

I find this solution mandatory to CakePHP letting developers to pack validation rules into their plugins!
I wrote a post to my blog to explain the problem and this solution: http://bit.ly/RXxw43

Bye,
MPeg.

marcopeg added some commits Oct 25, 2012

Allow Custom Validator Objects
I propose a way to allow models to use custom validation rules from
external classes, even packed into plugins.

Proposal documentation and explanation:
http://bit.ly/RXxw43
Custom Validation Objects
updated inline comment
@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ * ...
+ *
+ */
+ } elseif ( strpos($this->_rule , '::') ){

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Oct 25, 2012

Member

there shouldnt be any extra spaces (i count three)

@dereuromark

dereuromark Oct 25, 2012

Member

there shouldnt be any extra spaces (i count three)

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Oct 25, 2012

Member

not really, before commas there are also no spaces. but after.

@dereuromark

dereuromark Oct 25, 2012

Member

not really, before commas there are also no spaces. but after.

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member
} elseif (strpos($this->_rule ,'::')) {

should be

} elseif (strpos($this->_rule,'::')) {

It's a good idea to run your code though phpcs using the cake code standard

@AD7six

AD7six Oct 25, 2012

Member
} elseif (strpos($this->_rule ,'::')) {

should be

} elseif (strpos($this->_rule,'::')) {

It's a good idea to run your code though phpcs using the cake code standard

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+
+ // Contestualize lazy loading to validation repository even into plugins
+ $location = 'Model/Validation';
+ if ( $plugin ) $location = $plugin.'.'.$location;

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Oct 25, 2012

Member

but between concat there should: $plugin . '.' . $location

@dereuromark

dereuromark Oct 25, 2012

Member

but between concat there should: $plugin . '.' . $location

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@ADmad

This comment has been minimized.

Show comment Hide comment
@ADmad

ADmad Oct 25, 2012

Member

Please follow cake's coding standards. Use the code sniffer. Also remove comments from the code.

Member

ADmad commented Oct 25, 2012

Please follow cake's coding standards. Use the code sniffer. Also remove comments from the code.

@marcopeg

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

willco

willco

@ADmad

This comment has been minimized.

Show comment Hide comment
@ADmad

ADmad Oct 25, 2012

Member

The validator can already use behavior functions too for validation so you can just keep all your custom functions in a behavior for easy manageability and usability. So I am not sure if the ability to use any arbitrary function for validation is really necessary.

Member

ADmad commented Oct 25, 2012

The validator can already use behavior functions too for validation so you can just keep all your custom functions in a behavior for easy manageability and usability. So I am not sure if the ability to use any arbitrary function for validation is really necessary.

@AD7six

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
if (isset($methods[$rule])) {
$this->_ruleParams[] = array_merge($validator, $this->_passedOptions);
$this->_ruleParams[0] = array($field => $this->_ruleParams[0]);
$this->_valid = call_user_func_array($methods[$rule], $this->_ruleParams);
} elseif (class_exists('Validation') && method_exists('Validation', $this->_rule)) {
$this->_valid = call_user_func_array(array('Validation', $this->_rule), $this->_ruleParams);
+
+

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member

double new line

@AD7six

AD7six Oct 25, 2012

Member

double new line

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@AD7six

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
if (isset($methods[$rule])) {
$this->_ruleParams[] = array_merge($validator, $this->_passedOptions);
$this->_ruleParams[0] = array($field => $this->_ruleParams[0]);
$this->_valid = call_user_func_array($methods[$rule], $this->_ruleParams);
} elseif (class_exists('Validation') && method_exists('Validation', $this->_rule)) {
$this->_valid = call_user_func_array(array('Validation', $this->_rule), $this->_ruleParams);
+
+
+ /**

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member

no inline docs - use the funcion doc block

@AD7six

AD7six Oct 25, 2012

Member

no inline docs - use the funcion doc block

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@AD7six

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
@@ -265,14 +265,55 @@ public function process($field, &$data, &$methods) {
$validator = $this->_getPropertiesArray();
$rule = strtolower($this->_rule);
+
+

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member

double new line

@AD7six

AD7six Oct 25, 2012

Member

double new line

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@AD7six

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ list($plugin, $class) = pluginSplit($this->_rule);
+ list($className,$method) = explode('::',$class);
+
+ // Contestualize lazy loading to validation repository even into plugins

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member

no idea what Contestualize means, but please avoid inline comments. if it's not in a doc block - it doesn't show up in the api.

@AD7six

AD7six Oct 25, 2012

Member

no idea what Contestualize means, but please avoid inline comments. if it's not in a doc block - it doesn't show up in the api.

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@AD7six

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
} elseif (is_string($validator['rule'])) {
$this->_valid = preg_match($this->_rule, $data[$field]);
+
+
+

This comment has been minimized.

Show comment Hide comment
@AD7six

AD7six Oct 25, 2012

Member

more extra newlines

@AD7six

AD7six Oct 25, 2012

Member

more extra newlines

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

fixed

@marcopeg

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

@ADmad behaviors is meant to collect shared model logic.

I think to model logic as "what a model does with data".
Validation rules are "how data was expected to be".

Add or remove or changes validation rules applied to model's data is a model logic and should be shifted to a behavior.

Extend core validation methods adding some general pourpose validation is not responsibility of any behavior.

CakePHP core collects validation rules in a class who's not a behavior so I think my proposal is a good and correct interpretation of difference between behaviors and validation objects.

(i will fix my code to fit CakePHP standards in a few hours)

@ADmad behaviors is meant to collect shared model logic.

I think to model logic as "what a model does with data".
Validation rules are "how data was expected to be".

Add or remove or changes validation rules applied to model's data is a model logic and should be shifted to a behavior.

Extend core validation methods adding some general pourpose validation is not responsibility of any behavior.

CakePHP core collects validation rules in a class who's not a behavior so I think my proposal is a good and correct interpretation of difference between behaviors and validation objects.

(i will fix my code to fit CakePHP standards in a few hours)

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 25, 2012

Member

Where are the tests? This will not be merged without tests.

Member

markstory commented Oct 25, 2012

Where are the tests? This will not be merged without tests.

@markstory

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 25, 2012

Member

Additionally new features will not be merged onto the master branch. You will probably have to open a new pull request against the 2.3 branch.

Member

markstory commented Oct 25, 2012

Additionally new features will not be merged onto the master branch. You will probably have to open a new pull request against the 2.3 branch.

@@ -271,6 +274,13 @@ public function process($field, &$data, &$methods) {
$this->_valid = call_user_func_array($methods[$rule], $this->_ruleParams);

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 25, 2012

Member

Missing braces.

@markstory

markstory Oct 25, 2012

Member

Missing braces.

@@ -257,6 +257,9 @@ public function isUpdate($exists = null) {
/**
* Dispatches the validation rule to the given validator method
*
+ * Use "PluginName.ClassName::method" as validation rule name to refer to a custom validator object.
+ * /App/Plugin/Model/Validation/ClassName.php
+ *
* @return boolean True if the rule could be dispatched, false otherwise

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 25, 2012

Member

Missing spaces after ,. Please use the code sniffer for these types of errors.

@markstory

markstory Oct 25, 2012

Member

Missing spaces after ,. Please use the code sniffer for these types of errors.

@markstory

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
@@ -271,6 +274,13 @@ public function process($field, &$data, &$methods) {
$this->_valid = call_user_func_array($methods[$rule], $this->_ruleParams);
} elseif (class_exists('Validation') && method_exists('Validation', $this->_rule)) {
$this->_valid = call_user_func_array(array('Validation', $this->_rule), $this->_ruleParams);
+ } elseif (strpos($this->_rule ,'::')) {
+ list($plugin, $class) = pluginSplit($this->_rule);

This comment has been minimized.

Show comment Hide comment
@markstory

markstory Oct 25, 2012

Member

Braces are required around all control structures.

@markstory

markstory Oct 25, 2012

Member

Braces are required around all control structures.

@marcopeg

This comment has been minimized.

Show comment Hide comment
@marcopeg

marcopeg Oct 25, 2012

@markstory i'm not really sure how to produce tests for this code. it just load a class from a repo then search inside for a static method to call.

I will create a new pull request to the 2.3 branch.

@markstory i'm not really sure how to produce tests for this code. it just load a class from a repo then search inside for a static method to call.

I will create a new pull request to the 2.3 branch.

@dereuromark

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Oct 25, 2012

Member

closing as duplicate of the new PR

Member

dereuromark commented Oct 25, 2012

closing as duplicate of the new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment