Proposal: Custom Validation Objects #913

Closed
wants to merge 22 commits into
from

Conversation

Projects
None yet
7 participants
@marcopeg

My proposal is to create a Model/Validation repo where to store validation rules collection classes.

This way we can keep model's logic into behaviors and validation logic into validation classes.

I don't know how to pack tests for this pull request.
Any suggestion welcome!

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
@ADmad

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);
+ list($className,$method) = explode('::',$class);

This comment has been minimized.

@ADmad

ADmad Oct 25, 2012

Member

Space missing after comma ,

@ADmad

ADmad Oct 25, 2012

Member

Space missing after comma ,

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed

@ADmad

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ list($plugin, $class) = pluginSplit($this->_rule);
+ list($className,$method) = explode('::',$class);
+ $location = 'Model/Validation';
+ if ( $plugin ) $location = $plugin . '.' . $location;

This comment has been minimized.

@ADmad

ADmad Oct 25, 2012

Member

Extra space after ( and before ). If block should always have braces {} even for single lines.

@ADmad

ADmad Oct 25, 2012

Member

Extra space after ( and before ). If block should always have braces {} even for single lines.

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed

@ADmad

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ $location = 'Model/Validation';
+ if ( $plugin ) $location = $plugin . '.' . $location;
+ App::uses($className, $location);
+ if ( class_exists($className) ) $this->_valid = call_user_func_array(array($className, $method), $this->_ruleParams);

This comment has been minimized.

@ADmad

ADmad Oct 25, 2012

Member

Same comments are above. As requested earlier read the coding standards page on manual or used the code sniffer.

@ADmad

ADmad Oct 25, 2012

Member

Same comments are above. As requested earlier read the coding standards page on manual or used the code sniffer.

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed

This comment has been minimized.

@majna

majna Oct 25, 2012

Contributor

It's better to check for is_callable() here. Function call_user_func_array() could return false if Validation method doesn't exists or isn't visible in given context (which should be an error/exception, not invalid model data). Maybe I'm wrong, please check/test this case.

@majna

majna Oct 25, 2012

Contributor

It's better to check for is_callable() here. Function call_user_func_array() could return false if Validation method doesn't exists or isn't visible in given context (which should be an error/exception, not invalid model data). Maybe I'm wrong, please check/test this case.

@AD7six

This comment has been minimized.

Show comment
Hide comment
@AD7six

AD7six Oct 25, 2012

Member

Please add a comment when you've checked your PR with the code sniffer, so we can focus on the changes and not the formatting discrepancies.

Member

AD7six commented Oct 25, 2012

Please add a comment when you've checked your PR with the code sniffer, so we can focus on the changes and not the formatting discrepancies.

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
@@ -271,6 +274,15 @@ 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 ,'::')) {

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

still not the right spacing here

@dereuromark

dereuromark Oct 25, 2012

Member

still not the right spacing here

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
@@ -271,6 +274,17 @@ 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);
+ list($className,$method) = explode('::', $class);

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

missing space

@dereuromark

dereuromark Oct 25, 2012

Member

missing space

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

you dont need to always comment that its fixed. we can all see that :-)

@dereuromark

dereuromark Oct 25, 2012

Member

you dont need to always comment that its fixed. we can all see that :-)

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ list($plugin, $class) = pluginSplit($this->_rule);
+ list($className,$method) = explode('::', $class);
+ $location = 'Model/Validation';
+ if ( $plugin ) $location = $plugin . '.' . $location;

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

too many spaces, also always use {}

@dereuromark

dereuromark Oct 25, 2012

Member

too many spaces, also always use {}

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

fixed (i will install code sniffer quite soon!)

@marcopeg

marcopeg Oct 25, 2012

fixed (i will install code sniffer quite soon!)

@majna

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ if (class_exists($className) && method_exists($className, $method)) {
+ $this->_valid = call_user_func_array(array($className, $method), $this->_ruleParams);
+ } else {
+ $this->_valid = false;

This comment has been minimized.

@majna

majna Oct 25, 2012

Contributor

You should trigger_error here if validation handler is not found?

@majna

majna Oct 25, 2012

Contributor

You should trigger_error here if validation handler is not found?

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

I consider non existing validation methods as failed validations... It's simplest than triggering errors.

Anyway we can listen other contributors thoughts and implement it.

@marcopeg

marcopeg Oct 25, 2012

I consider non existing validation methods as failed validations... It's simplest than triggering errors.

Anyway we can listen other contributors thoughts and implement it.

This comment has been minimized.

@markstory

markstory Oct 26, 2012

Member

An error is better. Failing the validation rule gives the wrong feedback.

@markstory

markstory Oct 26, 2012

Member

An error is better. Failing the validation rule gives the wrong feedback.

This comment has been minimized.

@marcopeg

marcopeg Oct 26, 2012

ok, triggered an error.
i'm not sure if to tell user the rule should not be solved (actual implementation) or implement a deep investigation to define if class or method does not exist.

your opinion?

@marcopeg

marcopeg Oct 26, 2012

ok, triggered an error.
i'm not sure if to tell user the rule should not be solved (actual implementation) or implement a deep investigation to define if class or method does not exist.

your opinion?

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
@@ -271,6 +274,17 @@ 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, '::')) {

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

using strpos without type safe checks is never a good idea!

@dereuromark

dereuromark Oct 25, 2012

Member

using strpos without type safe checks is never a good idea!

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

How do you suggest to change this line?

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

@marcopeg

marcopeg Oct 25, 2012

How do you suggest to change this line?

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

This comment has been minimized.

@dereuromark

dereuromark Oct 25, 2012

Member

autsch. no. you usually use strpos with !== false

@dereuromark

dereuromark Oct 25, 2012

Member

autsch. no. you usually use strpos with !== false

This comment has been minimized.

@marcopeg

marcopeg Oct 25, 2012

mmm I don't think so.

if given string is like "::something" then "!== false" is a true condition... but given string is a very bad error because does not contain any class name.

I need to enter the if tree only if "::" is upper than one.

@marcopeg

marcopeg Oct 25, 2012

mmm I don't think so.

if given string is like "::something" then "!== false" is a true condition... but given string is a very bad error because does not contain any class name.

I need to enter the if tree only if "::" is upper than one.

This comment has been minimized.

@dereuromark

dereuromark Nov 2, 2012

Member

I disagree. Passing "::somestring" would be invalid either way and the developers fault (you cant possibly cover all mistakes possible here).
But using the type safe check we would at least result in a meaningful exception: "trigger_error(__d('cake_dev', 'Could not find custom validation rule %s'" instead of trying to execute it as a preg match string.

@dereuromark

dereuromark Nov 2, 2012

Member

I disagree. Passing "::somestring" would be invalid either way and the developers fault (you cant possibly cover all mistakes possible here).
But using the type safe check we would at least result in a meaningful exception: "trigger_error(__d('cake_dev', 'Could not find custom validation rule %s'" instead of trying to execute it as a preg match string.

This comment has been minimized.

@marcopeg

marcopeg Nov 3, 2012

understood, thank your for explanation.

@marcopeg

marcopeg Nov 3, 2012

understood, thank your for explanation.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 26, 2012

Member

An easy way to write tests for this is to include a sample validation object in the TestApp directory, and create a simple validation rule there, and use it. Ensure that it can both pass & fail validation on a model field. Without tests this will not be merged sorry.

Member

markstory commented Oct 26, 2012

An easy way to write tests for this is to include a sample validation object in the TestApp directory, and create a simple validation rule there, and use it. Ensure that it can both pass & fail validation on a model field. Without tests this will not be merged sorry.

trigger_error
added a trigger_error instruction when try to load a non existing
method or non existing custom validation class as suggested by
@markstory
@ADmad

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ $this->_valid = call_user_func_array(array($className, $method), $this->_ruleParams);
+ } else {
+ trigger_error(__d('cake_dev', 'Could not find custom validation rule %s', $this->_rule ), E_USER_WARNING);
+ }

This comment has been minimized.

@ADmad

ADmad Oct 26, 2012

Member

Extra space before ),

@ADmad

ADmad Oct 26, 2012

Member

Extra space before ),

@ADmad

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ list($plugin, $class) = pluginSplit($this->_rule);
+ list($className, $method) = explode('::', $class);
+ $location = 'Model/Validation';
+ if ($plugin) $location = $plugin . '.' . $location;

This comment has been minimized.

@ADmad

ADmad Oct 26, 2012

Member

Missing braces { }. Please take the trouble of carefully reading the coding standards page on the manual and setting up the code sniffer and fix all the formatting errors. We appreciate your contribution but your patch won't be merged in if it doesn't confer to cake's coding standards. Its really not practical for us to go about pointing out each mistake.

Once you are done please also squash your commits to avoid unnecessary noise in the repo history.

@ADmad

ADmad Oct 26, 2012

Member

Missing braces { }. Please take the trouble of carefully reading the coding standards page on the manual and setting up the code sniffer and fix all the formatting errors. We appreciate your contribution but your patch won't be merged in if it doesn't confer to cake's coding standards. Its really not practical for us to go about pointing out each mistake.

Once you are done please also squash your commits to avoid unnecessary noise in the repo history.

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 26, 2012

Finally installed phpcs, but be carefull... it does not alert me about white spaces near "()" or commas...
I tested it breaking code rules intentionally. It alert me about white lines... but not so more.

I installed pear and phpcs following cakepho git's rules and repos...

Anyway at writing time no alert is given by the phpcs utility.

@markstory i will create a test controller+model today. I create that files into the test-app then commit to this PR, right?

Finally installed phpcs, but be carefull... it does not alert me about white spaces near "()" or commas...
I tested it breaking code rules intentionally. It alert me about white lines... but not so more.

I installed pear and phpcs following cakepho git's rules and repos...

Anyway at writing time no alert is given by the phpcs utility.

@markstory i will create a test controller+model today. I create that files into the test-app then commit to this PR, right?

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 26, 2012

Member

@thepeg you also need to install the CakePHP codesniffer rules, and use them to check your code.

Member

markstory commented Oct 26, 2012

@thepeg you also need to install the CakePHP codesniffer rules, and use them to check your code.

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 29, 2012

@markstory i created a model, a custom validation object and a test controller in /test_app as requested.

All my code passed the phpcs test with CakePHP standards.

Running CustomValidationObject controller a debug will display validation errors for test data passed to the model.
A waring is thrown by a non existing validation rule. It is an intentional error to demonstrate how my code triggers errors if custom rule/object does not exists.

@markstory i created a model, a custom validation object and a test controller in /test_app as requested.

All my code passed the phpcs test with CakePHP standards.

Running CustomValidationObject controller a debug will display validation errors for test data passed to the model.
A waring is thrown by a non existing validation rule. It is an intentional error to demonstrate how my code triggers errors if custom rule/object does not exists.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 30, 2012

Member

Things look good on the phpcs front. However, there still aren't any test cases for the feature.

Member

markstory commented Oct 30, 2012

Things look good on the phpcs front. However, there still aren't any test cases for the feature.

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 30, 2012

@markstory uh, I didn't sync committed changes!

here are listed test files:
/test_app/Controller/CustomValidationObjectController.php
/test_app/Model/CustomValidationModel.php
/test_app/Model/Validation/CustomValidationObject.php

@markstory uh, I didn't sync committed changes!

here are listed test files:
/test_app/Controller/CustomValidationObjectController.php
/test_app/Model/CustomValidationModel.php
/test_app/Model/Validation/CustomValidationObject.php

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Oct 30, 2012

Member

In the localised plugin recently the Validation folder was moved directly under the app folder, so for consistency here too it would be better to use app/Validation instead of app/Model/Validation

Member

ADmad commented Oct 30, 2012

In the localised plugin recently the Validation folder was moved directly under the app folder, so for consistency here too it would be better to use app/Validation instead of app/Model/Validation

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 30, 2012

@ADmad i'm not sure if your suggestion is really consistent to the CakePHP framework.
Let me explain my point of view.

Custom validation objects are always related to models.
Validation itself is a model stuff.

I'm quite sure to maintain app/Model/Validation structure for my plugin and I suggest to roll back in your.

@markstory what do you think about this question?

@ADmad i'm not sure if your suggestion is really consistent to the CakePHP framework.
Let me explain my point of view.

Custom validation objects are always related to models.
Validation itself is a model stuff.

I'm quite sure to maintain app/Model/Validation structure for my plugin and I suggest to roll back in your.

@markstory what do you think about this question?

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Oct 30, 2012

Member

..in your.

@thepeg The "Localised" plugin is not my personal plugin. It's an offical CakePHP plugin maintained by the core devs.

Member

ADmad commented Oct 30, 2012

..in your.

@thepeg The "Localised" plugin is not my personal plugin. It's an offical CakePHP plugin maintained by the core devs.

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 30, 2012

my mistake :-)

... but I think app/Validation is not the best organization for that kind of objects.

just wait for @markstory opinion... it's trivial to move from app/Model/Validation to app/Validation and vice-versa... it is not a technical problem, just organization!

thank you for comment this PR and suggest this argument!

my mistake :-)

... but I think app/Validation is not the best organization for that kind of objects.

just wait for @markstory opinion... it's trivial to move from app/Model/Validation to app/Validation and vice-versa... it is not a technical problem, just organization!

thank you for comment this PR and suggest this argument!

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Oct 30, 2012

Member

For what it's worth I too prefer app/Model/Validation. But at the end of the day all I want is consistency.

Member

ADmad commented Oct 30, 2012

For what it's worth I too prefer app/Model/Validation. But at the end of the day all I want is consistency.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 30, 2012

Member

I prefer Model/Validation, as the model class is the one loading these validation classes. The localised repo, has extensions to the core Validation class, allowing you to do Validation::postal($value, 'nl'); I think they are different enough to be in different places.

I wonder if only allowing static methods is going to be a problem?

Member

markstory commented Oct 30, 2012

I prefer Model/Validation, as the model class is the one loading these validation classes. The localised repo, has extensions to the core Validation class, allowing you to do Validation::postal($value, 'nl'); I think they are different enough to be in different places.

I wonder if only allowing static methods is going to be a problem?

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 31, 2012

Would you suggest an alternative?
Next days I will study the localised plugin to propose an enhancement to my PR.

Would you suggest an alternative?
Next days I will study the localised plugin to propose an enhancement to my PR.

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Oct 31, 2012

Member

maybe the localized plugin then should be adjusted to also use app/Model/Validation - for the sake of consistency...

Member

dereuromark commented Oct 31, 2012

maybe the localized plugin then should be adjusted to also use app/Model/Validation - for the sake of consistency...

@AD7six

This comment has been minimized.

Show comment
Hide comment
@AD7six

AD7six Oct 31, 2012

Member

I agree with markstory regarding different purposes

But we currently have:

  • Cake\\Utility\\Validation
  • Localized\\Validation\\XXValidation
  • Model\\Validator

And with this PR

  • Plugin\\Model\\Validation\\X

4 different places for validation related classes seems a bit much.

Member

AD7six commented Oct 31, 2012

I agree with markstory regarding different purposes

But we currently have:

  • Cake\\Utility\\Validation
  • Localized\\Validation\\XXValidation
  • Model\\Validator

And with this PR

  • Plugin\\Model\\Validation\\X

4 different places for validation related classes seems a bit much.

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 31, 2012

Please consider this assertion:
"validation is a model related stuff"

isn't true?

How do you do (the team) to make a decision about this kind of things?
This is mi first PR (but not the last) so I need to learn!

Please consider this assertion:
"validation is a model related stuff"

isn't true?

How do you do (the team) to make a decision about this kind of things?
This is mi first PR (but not the last) so I need to learn!

@AD7six

This comment has been minimized.

Show comment
Hide comment
@AD7six

AD7six Oct 31, 2012

Member

@thepeg not sure what your last comment means - models handle data, and data is the thing that gets validated. Validation is a model related task.

Decisions are made by discussion/agreement :).

Member

AD7six commented Oct 31, 2012

@thepeg not sure what your last comment means - models handle data, and data is the thing that gets validated. Validation is a model related task.

Decisions are made by discussion/agreement :).

@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Oct 31, 2012

@AD7six you wrote what I mean, better :-)

so, can I keep my code implementing a "Model/Validation" folder structure?

@AD7six you wrote what I mean, better :-)

so, can I keep my code implementing a "Model/Validation" folder structure?

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Nov 1, 2012

Member

@markstory the different purposes might be easily apparent to us experienced users but not to new users. 4 different locations for validation related classes which @AD7six pointed out will surely confuse them.

So I propose moving the localised validation classes back under Localized\Lib\Validation or under Localised\Model\Validation similar to this PR.

@thepeg No change needed to your PR 😄. I think pretty much everyone is fine with having custom validation classes under Model\Validation

Member

ADmad commented Nov 1, 2012

@markstory the different purposes might be easily apparent to us experienced users but not to new users. 4 different locations for validation related classes which @AD7six pointed out will surely confuse them.

So I propose moving the localised validation classes back under Localized\Lib\Validation or under Localised\Model\Validation similar to this PR.

@thepeg No change needed to your PR 😄. I think pretty much everyone is fine with having custom validation classes under Model\Validation

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Nov 1, 2012

Member

@ADmad I agree that 4 is too many, especially given there are only 2 kinds of validation extensions we support. I would prefer we only have 2 places where custom validators can live 😄

@AD7six I don't think plugins should count towards the total, as they are packaged sets of libraries. They basically duplicate the app folders.

Member

markstory commented Nov 1, 2012

@ADmad I agree that 4 is too many, especially given there are only 2 kinds of validation extensions we support. I would prefer we only have 2 places where custom validators can live 😄

@AD7six I don't think plugins should count towards the total, as they are packaged sets of libraries. They basically duplicate the app folders.

@ADmad

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ $location = $plugin . '.' . $location;
+ }
+ App::uses($className, $location);
+ if (class_exists($className) && method_exists($className, $method)) {

This comment has been minimized.

@ADmad

ADmad Nov 1, 2012

Member

Why not just use is_callable() here.

@ADmad

ADmad Nov 1, 2012

Member

Why not just use is_callable() here.

This comment has been minimized.

@marcopeg

marcopeg Nov 2, 2012

good idea!

@ADmad

View changes

lib/Cake/Test/test_app/Controller/CustomValidationObjectController.php
+
+ public $autoRender = false;
+
+ public $uses = array( 'CustomValidationModel' );

This comment has been minimized.

@ADmad

ADmad Nov 1, 2012

Member

Extra space after ( and before )

@ADmad

ADmad Nov 1, 2012

Member

Extra space after ( and before )

@ADmad

View changes

lib/Cake/Test/test_app/Controller/CustomValidationObjectController.php
+ public $uses = array( 'CustomValidationModel' );
+
+ public function index() {
+ $data = array( 'CustomValidationModel' => array(

This comment has been minimized.

@ADmad

ADmad Nov 1, 2012

Member

Extra space after array(

@ADmad

ADmad Nov 1, 2012

Member

Extra space after array(

+ ));
+ $this->CustomValidationModel->set($data);
+ $this->CustomValidationModel->validates();
+ debug($this->CustomValidationModel->validationErrors);

This comment has been minimized.

@ADmad

ADmad Nov 1, 2012

Member

Left over debug statement.

@ADmad

ADmad Nov 1, 2012

Member

Left over debug statement.

This comment has been minimized.

@marcopeg

marcopeg Nov 2, 2012

I'm not sure what you mean.
This debug() statement is part of the test to display validation errors generated by the model.

Please tell me how to change it to make this code meaningful to the test.

@marcopeg

marcopeg Nov 2, 2012

I'm not sure what you mean.
This debug() statement is part of the test to display validation errors generated by the model.

Please tell me how to change it to make this code meaningful to the test.

This comment has been minimized.

@ADmad

ADmad Nov 2, 2012

Member

You are not supposed to use debug() in test code to check variable contents. Do you you see such debugs anywhere in the other core tests? Use asserts in the test cases to compare the validation errors property against expected data.

@ADmad

ADmad Nov 2, 2012

Member

You are not supposed to use debug() in test code to check variable contents. Do you you see such debugs anywhere in the other core tests? Use asserts in the test cases to compare the validation errors property against expected data.

This comment has been minimized.

@marcopeg

marcopeg Nov 3, 2012

but I don't need to test a validation rule to work... I need to test a custom validation object is imported and initialized.
Don't know how to write that code.

Can you help?

@marcopeg

marcopeg Nov 3, 2012

but I don't need to test a validation rule to work... I need to test a custom validation object is imported and initialized.
Don't know how to write that code.

Can you help?

This comment has been minimized.

@markstory

markstory Nov 6, 2012

Member

The included controller file isn't necessary at all. Nor is the additional model class. You should add a unit test case to the existing model validation test suite. Here example of a method similar to what you would want.

@markstory

markstory Nov 6, 2012

Member

The included controller file isn't necessary at all. Nor is the additional model class. You should add a unit test case to the existing model validation test suite. Here example of a method similar to what you would want.

@ADmad

View changes

lib/Cake/Test/test_app/Model/CustomValidationModel.php
+ public $useTable = false;
+
+ public $validate = array(
+ 'name' => 'notEmpty',

This comment has been minimized.

@ADmad

ADmad Nov 1, 2012

Member

Extra spaces before =>, same issue on next line.

@ADmad

ADmad Nov 1, 2012

Member

Extra spaces before =>, same issue on next line.

@dereuromark

View changes

lib/Cake/Model/Validator/CakeValidationRule.php
+ $location = $plugin . '.' . $location;
+ }
+ App::uses($className, $location);
+ if ( is_callable(array($className,$method)) ) {

This comment has been minimized.

@dereuromark

dereuromark Nov 2, 2012

Member

arent here still lots of coding standard violations?

@dereuromark

dereuromark Nov 2, 2012

Member

arent here still lots of coding standard violations?

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Nov 3, 2012

Member

I like the idea, but I think it is still half-baked. Not confortable with the idea of static only methods and it leaves things out like configuring validation. But it might be a good start while we figure out a more flexible approach.

Member

lorenzo commented Nov 3, 2012

I like the idea, but I think it is still half-baked. Not confortable with the idea of static only methods and it leaves things out like configuring validation. But it might be a good start while we figure out a more flexible approach.

fix
thanks to @dereuromark
@marcopeg

This comment has been minimized.

Show comment
Hide comment
@marcopeg

marcopeg Nov 3, 2012

@lorenzo This conditional tree handles stati methods.

May be another PR will implement a different if-tree to handle an object/method rule format... like:

} elseif (is_array($this->_rule) && is_callable($this->_rule)) {

NOTE: above code is written just as a hypothesi...

marcopeg commented Nov 3, 2012

@lorenzo This conditional tree handles stati methods.

May be another PR will implement a different if-tree to handle an object/method rule format... like:

} elseif (is_array($this->_rule) && is_callable($this->_rule)) {

NOTE: above code is written just as a hypothesi...

@lorenzo lorenzo closed this Jan 28, 2013

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Jan 28, 2013

Member

Please reopen targeting the 2.4 branch

Member

lorenzo commented Jan 28, 2013

Please reopen targeting the 2.4 branch

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