Remove validation part from Propel core and add Validate behavior #156

Merged
merged 24 commits into from May 8, 2012

Conversation

Projects
None yet
7 participants
Member

cristianoc72 commented Mar 25, 2012

According to #94 issue, we remove the validation part from the Propel core and, at the same time, we add a behavior, to perform validations as easily as in the previous way.

To remove the validation part from the core, we merged @willdurand's remove-validate branch into master (with a couple of fixes in 3 fixtures schemas).

The Validate behavior is based on Symfony Validation Component and exposes about the same api of previous validation system: a ´validate()´ method to perfom validations and a ´getValidationFailures()´ to retrieve validation errors (we're old Propel1 users and we always use the good old validation system).

This behavior is totally configurable in ´schema.xml´. Unfortunately, we was unable to mantain the old xml syntax, but the new beahvior configuration is very easy and readable.

@fzaninotto fzaninotto and 3 others commented on an outdated diff Mar 26, 2012

composer.lock
"packages": [
{
- "package": "monolog/monolog",
+ "package": "doctrine/common",
@fzaninotto

fzaninotto Mar 26, 2012

Member

what does doctrine/common do here?

@cristianoc72

cristianoc72 Mar 26, 2012

Member

Sf2 Validator depends on doctrine/common. ValidatorFactory class uses doctrine code to read configuration via annotations (https://github.com/symfony/Validator/blob/master/ValidatorFactory.php). It's a feature that we don't use but I have not found a way around the problem.

@willdurand

willdurand Mar 26, 2012

Owner

But this dependency should be handled by the composer.json of the
component, not in Propel2. We just need to add the component name AFAIK.

ping @Seldaek

2012/3/26 Cristiano Cinotti <
reply@reply.github.com

 "packages": [
     {
  •        "package": "monolog/monolog",
    
  •        "package": "doctrine/common",
    

Sf2 Validator depends on doctrine/common. ValidatorFactory class uses
doctrine code to read configuration via annotations (
https://github.com/symfony/Validator/blob/master/ValidatorFactory.php).
It's a feature that we don't use but I have not found a way around the
problem.


Reply to this email directly or view it on GitHub:
https://github.com/propelorm/Propel2/pull/156/files#r605913

@cristianoc72

cristianoc72 Mar 26, 2012

Member

I'm sorry may be I did something wrong. I've added only sf2 validator to my composer.json and then I run "composer.phar install". I've never written anything in composer.lock: this line was added automatically by composer.

@Seldaek

Seldaek Mar 27, 2012

This is fine, the lock file just specifies exactly which versions of all the dependencies have been installed. I guess lock files may/should be ignored for libraries though. The main benefit is really for applications and teams working on the same project. In this case there is little value to it, and it can be in .gitignore.

@willdurand

willdurand Mar 27, 2012

Owner

Oh yes... No problem then, I thought it was a change in the composer.json file, not the .lock.

Thanks Jordi.

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

documentation/behaviors/validate.markdown
+ <foreign-key foreignTable="validate_reader">
+ <reference local="reader_id" foreign="id"/>
+ </foreign-key>
+ <foreign-key foreignTable="validate_book">
+ <reference local="book_id" foreign="id"/>
+ </foreign-key>
+ <behavior name="validate">
+ <parameter name="rule1" value="{column: reader_id, validator: NotNull}" />
+ <parameter name="rule2" value="{column: book_id, validator: NotNull}" />
+ <parameter name="rule3" value="{column: reader_id, validator: Type, options: {type: integer}}" />
+ <parameter name="rule4" value="{column: book_id, validator: Type, options: {type: integer}}" />
+ </behavior>
+ </table>
+{% endhighlight %}
+
+And now the validation flow wil be the following:

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

documentation/behaviors/validate.markdown
+{% highlight xml %}
+<!-- THIS EXAMPLE WORKS FINE -->
+
+<!-- your schema -->
+ <behavior name="validate">
+ .......
+ <parameter name="rule1" value="{column: isbn, validator: Regex, options: {pattern: &quot;/[^\d-]+/&quot;, match: false, message: Please enter a valid ISBN }}" />
+ </behavior>
+
+<!-- end of your schema -->
+{% endhighlight %}
+
+
+## Inside Symfony2 ##
+
+The behavior adds to ActiveRecord objects the static `loadValidatorMetadata()` method, wich contains all validation rules. So, inside your Symfony projects, you can perform "usual" Symfony validations:

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

documentation/behaviors/validate.markdown
+## Properties and methods added to ActiveRecord ##
+
+The behavior adds the following properties to your ActiveRecord:
+
+* `alreadyInValidation`: this *protected* property is a flag to prevent endless validation loop, if this object is referenced by another object on which we're performing a validation.
+* `validationFailures`: this *protected* property contains the ConstraintViolationList object.
+
+
+
+
+The behavior adds the following methods to your ActiveRecord:
+
+* `validate`: this *public* method validates the object and all objects related to it.
+* `doValidate`: this *protected* method performs the validation work for complex object models. It's called by `validate()`.
+* `getValidationFailures`: this *public* method gets the ConstraintViolationList object, that contains all ConstraintViolation objects resulted from last call to `validate()` method.
+* `loadValidatorMetadata`: this *public static* method contains all the Constraint objects.
@fzaninotto

fzaninotto Mar 26, 2012

Member

I think the readme is missing a simple section describing how to enable auto validation on save - probably by calling validate() in the preSave() method.

@cristianoc72

cristianoc72 Mar 26, 2012

Member

Great! I had not thought of it. Do you think we should give the possibility to enable auto validation also via schema.xml? I.e.:

<parameter name="auto_validate" value="true" />

@fzaninotto

fzaninotto Mar 26, 2012

Member

Well, you'd have to handle a special case for behavior parameters nor being actual rules... I think it's enough to mention the ability to write a simple preSave() in the doc, don't you?

@fzaninotto fzaninotto commented on the diff Mar 26, 2012

src/Propel/Generator/Behavior/I18n/I18nBehavior.php
@@ -141,16 +141,11 @@ protected function moveI18nColumns()
}
$column = $table->getColumn($columnName);
// add the column
- $i18nTable->addColumn(clone $column);
@fzaninotto

fzaninotto Mar 26, 2012

Member

Instead of removing this, shouldn't the case where both the i18n and the validation behaviors are triggered on the same table be handled? Otherwise it's a regression.

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ *
+ * @return string
+ */
+ protected function addLoadValidatorMetadataMethod()
+ {
+ $script = "
+/**
+ * Configure validators constraints. The Validator object uses this method
+ * to perform object validation.
+ *
+ * @param ClassMetadata \$metadata
+ */
+public static function loadValidatorMetadata(ClassMetadata \$metadata)
+{\n";
+
+ $yaml = new Parser();
@fzaninotto

fzaninotto Mar 26, 2012

Member

This should be refactored to its own method IMHO

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+use Symfony\Component\Yaml\Parser;
+use \InvalidArgumentException;
+
+/**
+ * Validate a model object using Symfony2 Validator component
+ *
+ * @author Cristiano Cinotti
+*/
+class ValidateBehavior extends Behavior
+{
+ /**
+ * @param array $parameters The parameters array
+ *
+ * The parameters array has a structure like this:
+ * array(
+ * "rule1" => "{column: your_column_name, validator: your_validator_name, options: {message: your_error_message, ....}"
@fzaninotto

fzaninotto Mar 26, 2012

Member

I think the behavior should accept both string (YAML) and array values. CML schemas is not the only way to build a model. This can also be done using PHP, or... YAML. So the fact that the rule value is a string should be treated as a special case. That means the code should feature this kind of snippet:

protected function cleanupParameters()
{
  $parser = new Parser();
  $params = $this->getParameters();
  foreach ($params as $key => $value) {
    if (is_string($value)) {
      $params[$key] = $parser->parse($value);;
    }
  }
  $this->setParameters($params);
}

And this method should be called upon initialization.

@cristianoc72

cristianoc72 Mar 28, 2012

Member

You were right! I'm testing the code to implement your comments about I18n and Concrete Ineritance behaviors and it's really easier passing an array, when you create a behavior object via php.

Member

fzaninotto commented Mar 26, 2012

General CS remark: opening brackets must be on the same line.

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
@@ -0,0 +1,253 @@
+<?php
+/**
+ * This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @license MIT License
+ */
+
+namespace Propel\Generator\Behavior\Validate;
+
+use Propel\Generator\Model\Behavior;
+use Symfony\Component\Yaml\Parser;
+use \InvalidArgumentException;
@fzaninotto

fzaninotto Mar 26, 2012

Member

Propel has its own exceptions, you should use Propel\Generator\Exception\InvalidArgumentException.

@fzaninotto fzaninotto commented on the diff Mar 26, 2012

...r/ConcreteInheritance/ConcreteInheritanceBehavior.php
@@ -83,12 +83,6 @@ public function modifyTable()
$this->getTable()->addForeignKey($copiedFk);
}
- // add the validators of the parent table
- foreach ($parentTable->getValidators() as $validator) {
@fzaninotto

fzaninotto Mar 26, 2012

Member

Same remark as for the i18n behavior: the cohabitation of the validation and the concrete_inheritance behaviors should be handled.

@fzaninotto fzaninotto commented on the diff Mar 26, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ if ($deep)
+ {
+ $string .= ", ";
+ }
+
+ return $string;
+
+ }
+
+ /**
+ * Adds the validate() method.
+ * @return string The code to be added to model class
+ */
+ protected function addValidateMethod()
+ {
+
@fzaninotto

fzaninotto Mar 26, 2012

Member

CS: no need for this extra line.

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ *
+ * @return string The code to be added to model class
+ */
+ public function objectAttributes()
+ {
+
+ return $this->renderTemplate('objectAttributes');
+
+ }
+
+ /**
+ * Add a loadValidatorMetadata() method
+ *
+ * @return string
+ */
+ protected function addLoadValidatorMetadataMethod()
@fzaninotto

fzaninotto Mar 26, 2012

Member

Why don't you use templates for this method just like for the others?

@cristianoc72

cristianoc72 Mar 28, 2012

Member

It seemed too complicated. I was wrong.
Done

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...ator/Behavior/Validate/templates/objectDoValidate.php
+protected function doValidate(Validator $validator)
+{
+ $failureMap = new ConstraintViolationList();
+
+ if (!$this->alreadyInValidation)
+ {
+ $this->alreadyInValidation = true;
+ $retval = null;
+
+ <?php if ($hasForeignKeys) : ?>
+ // We call the validate method on the following object(s) if they
+ // were passed to this object by their coresponding set
+ // method. This object relates to these object(s) by a
+ // foreign key reference.
+
+ <?php foreach($aVarNames as $aVarName) : ?>
@fzaninotto

fzaninotto Mar 26, 2012

Member

Don't add indentation before opening phpTags, or else the generated code will be messy.

@fzaninotto fzaninotto commented on the diff Mar 26, 2012

...Propel/Generator/Builder/Om/AbstractObjectBuilder.php
@@ -154,15 +154,6 @@ protected function isAddGenericAccessors()
return (!$table->isAlias() && $this->getBuildProperty('addGenericAccessors'));
}
- /**
- * Whether to add the validate() method.
- * This is based on the build property propel.addValidateMethod
- */
- protected function isAddValidateMethod()
- {
- return $this->getBuildProperty('addValidateMethod');
@fzaninotto

fzaninotto Mar 26, 2012

Member

You should update the documentation accordingly if you remove this parameter.

@fzaninotto fzaninotto commented on the diff Mar 26, 2012

src/Propel/Generator/Builder/Om/PeerBuilder.php
@@ -1818,64 +1818,6 @@ protected static function doOnDeleteSetNull(Criteria \$criteria, ConnectionInter
}
/**
- * Adds the doValidate() method.
- * @param string &$script The script will be modified in this method.
- */
- protected function addDoValidate(&$script)
@fzaninotto

fzaninotto Mar 26, 2012

Member

Awesome. Very glad to get rid of this plumbing.

@fzaninotto fzaninotto and 1 other commented on an outdated diff Mar 26, 2012

...erator/Behavior/Validate/templates/objectValidate.php
@@ -0,0 +1,34 @@
+
+
+/**
+ * Validates the object and all objects related to this table.
+ *
+ * @param object \$validator A Validator class instance
+ * @return boolean Whether all objects pass validation.
+ * @see doValidate()
+ * @see getValidationFailures()
+ */
+public function validate(Validator $validator = null)
@fzaninotto

fzaninotto Mar 26, 2012

Member

What's the purpose of splitting this into two methods (doValidate()) ?

@cristianoc72

cristianoc72 Mar 28, 2012

Member

No purpose: I used the old validator file as canvas, so I left here this method.
Now grouped in validate() method and objectValidate template.

Member

fzaninotto commented Mar 26, 2012

Overall, awesome work. 👏

Member

cristianoc72 commented Mar 26, 2012

@fzaninotto Thanks! The next few nights I'll work hard on your suggestions!

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+* `validate()`: this method performs validation on the ActiveRecord object itself and on all related objects. If the validation is successful it returns true, otherwise false.
+* `getValidationFailures()`: this method returns a [ConstraintViolationList](http://api.symfony.com/2.0/Symfony/Component/Validator/ConstraintViolationList.html) object. If validate() is false, it returns a list of [ConstraintViolation](http://api.symfony.com/2.0/Symfony/Component/Validator/ConstraintViolation.html) objects, if validate() is true, it returns an empty `ConstraintViolationList` object.
+
+
+Now you are ready to perform validations:
+
+{% highlight php %}
+<?php
+
+$author = new Author();
+$author->setLastName('Wilde');
+$author->setFirstName('Oscar');
+$author->setEmail('oscar.wilde@gmail.com');
+
+if (!$author->validate())
+{
@willdurand

willdurand Mar 29, 2012

Owner

In the Propel2 CS, the opening bracking should be on the same line of conditions:

if (cond) {
    // foo
} else if (cond2) {
    // bar
}
@cristianoc72

cristianoc72 Mar 29, 2012

Member

Damn! I completely misunderstood Propel cs. A review is needed.

@willdurand

willdurand Mar 29, 2012

Owner

There is absolutely no problem ;)

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+
+
+Now you are ready to perform validations:
+
+{% highlight php %}
+<?php
+
+$author = new Author();
+$author->setLastName('Wilde');
+$author->setFirstName('Oscar');
+$author->setEmail('oscar.wilde@gmail.com');
+
+if (!$author->validate())
+{
+ foreach ($author->getValidationFailures() as $failure)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+{% highlight php %}
+<?php
+
+$author = new Author();
+$author->setLastName('Wilde');
+$author->setFirstName('Oscar');
+$author->setEmail('oscar.wilde@gmail.com');
+
+if (!$author->validate())
+{
+ foreach ($author->getValidationFailures() as $failure)
+ {
+ echo "Property ".$failure->getPropertyPath().": ".$failure->getMessage()."\n";
+ }
+}
+else

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+$book = new Book();
+$book->setAuthor($author);
+$book->setPublisher($publisher);
+$book->setTitle('The country house');
+$book->setPrice(10,00);
+
+$ret = $book->save();
+
+//if $ret <= 0 means no affected rows, that is validation failed or no object to persist
+if ($ret <= 0)
+{
+ $failures = $this->getValidationFailures();
+
+ //count($failures) > 0 means that we have ConstraintViolation objects and validation failed
+ if (count($failures) > 0)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+
+$author = AuthorQuery::create()->findPk(1);
+$publisher = PublisherQuery::create()->findPk(1);
+
+$book = new Book();
+$book->setAuthor($author);
+$book->setPublisher($publisher);
+$book->setTitle('The country house');
+$book->setPrice(10,00);
+
+$ret = $book->save();
+
+//if $ret <= 0 means no affected rows, that is validation failed or no object to persist
+if ($ret <= 0)
+{
+ $failures = $this->getValidationFailures();
@willdurand

willdurand Mar 29, 2012

Owner

Shouldn't be a call to the getValidationFailures() method on the $book object, not $this?

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+$book->setAuthor($author);
+$book->setPublisher($publisher);
+$book->setTitle('The country house');
+$book->setPrice(10,00);
+
+$ret = $book->save();
+
+//if $ret <= 0 means no affected rows, that is validation failed or no object to persist
+if ($ret <= 0)
+{
+ $failures = $this->getValidationFailures();
+
+ //count($failures) > 0 means that we have ConstraintViolation objects and validation failed
+ if (count($failures) > 0)
+ {
+ foreach ($this->getValidationFailures() as $failure)
@cristianoc72

cristianoc72 Mar 30, 2012

Member
foreach ($this->getValidationFailures() as $failure)

becomes

foreach ($failures as $failure)

and fixed CS, too.

@willdurand willdurand commented on the diff Mar 29, 2012

documentation/behaviors/validate.markdown
+ }
+}
+{% endhighlight %}
+
+
+## Inside Symfony2 ##
+
+The behavior adds to ActiveRecord objects the static `loadValidatorMetadata()` method, which contains all validation rules. So, inside your Symfony projects, you can perform "usual" Symfony validations:
+
+{% highlight php %}
+<?php
+
+//Symfony 2
+
+use Symfony\Component\HttpFoundation\Response;
+use YouVendor\YourBundle\Model\Author;
@willdurand

willdurand Mar 29, 2012

Owner

s/YouVendor/YourVendor

@willdurand

willdurand Apr 15, 2012

Owner

Not fixed there

@cristianoc72

cristianoc72 Apr 19, 2012

Member

Fixed...now

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+<?php
+
+//Symfony 2
+
+use Symfony\Component\HttpFoundation\Response;
+use YouVendor\YourBundle\Model\Author;
+// ...
+
+public function indexAction()
+{
+ $author = new Author();
+ // ... do something to the $author object
+
+ $validator = $this->get('validator');
+ if (!$author->validate($validator))
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

documentation/behaviors/validate.markdown
+// ...
+
+public function indexAction()
+{
+ $author = new Author();
+ // ... do something to the $author object
+
+ $validator = $this->get('validator');
+ if (!$author->validate($validator))
+ {
+ $errors = $author->getValidationFailures();
+
+ return new Response(print_r($errors, true));
+
+ }
+ else

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+
+ /**
+ * @param object $builder The current builder
+ */
+ protected $builder;
+
+ /**
+ * Add behavior methods to model class
+ *
+ * @return string
+ */
+ public function objectMethods($builder)
+ {
+ $array = $this->getParameters();
+ if (empty($array))
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ */
+ public function objectAttributes()
+ {
+ return $this->renderTemplate('objectAttributes');
+ }
+
+ /**
+ * Convert those parameters, containing an array in YAML format
+ * into a php array
+ */
+ protected function cleanupParameters()
+ {
+ $parser = new Parser();
+ $params = $this->getParameters();
+ foreach ($params as $key => $value)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ public function objectAttributes()
+ {
+ return $this->renderTemplate('objectAttributes');
+ }
+
+ /**
+ * Convert those parameters, containing an array in YAML format
+ * into a php array
+ */
+ protected function cleanupParameters()
+ {
+ $parser = new Parser();
+ $params = $this->getParameters();
+ foreach ($params as $key => $value)
+ {
+ if (is_string($value))

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ }
+ $this->setParameters($params);
+ }
+
+ /**
+ * Add loadValidatorMetadata() method
+ *
+ * @return string
+ */
+ protected function addLoadValidatorMetadataMethod()
+ {
+ $params = $this->getParameters();
+ $constraints = array();
+
+ foreach ($params as $key=>$properties)
+ {
@willdurand

willdurand Mar 29, 2012

Owner

same here and for the whole method

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+
+ foreach ($params as $key=>$properties)
+ {
+ if (!isset($properties['column']))
+ {
+ throw new InvalidArgumentException('Please, define the column to validate.');
+ }
+
+ if (!isset($properties['validator']))
+ {
+ throw new InvalidArgumentException('Please, define the validator constraint.');
+ }
+
+ if (!class_exists("Symfony\\Component\\Validator\\Constraints\\".$properties['validator'], true))
+ {
+ throw new InvalidArgumentException('The constraint class '.$properties['validator'].' does not exist.');
@willdurand

willdurand Mar 29, 2012

Owner

You could write your own exception like : UnknownConstraintException or InvalidConstraintException.

@cristianoc72

cristianoc72 Mar 30, 2012

Member

Nice! I had not thought.
I've written ConstraintNotFoundException extending ClassNotFoundException. In fact, it's not an exception thrown for an invalid argument, but because we can't find the correct class, associated to this argument (imho).

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ {
+ throw new InvalidArgumentException('Please, define the column to validate.');
+ }
+
+ if (!isset($properties['validator']))
+ {
+ throw new InvalidArgumentException('Please, define the validator constraint.');
+ }
+
+ if (!class_exists("Symfony\\Component\\Validator\\Constraints\\".$properties['validator'], true))
+ {
+ throw new InvalidArgumentException('The constraint class '.$properties['validator'].' does not exist.');
+ }
+
+ if (isset($properties['options']))
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ if (isset($properties['options']))
+ {
+ if (!is_array($properties['options']))
+ {
+ throw new InvalidArgumentException('The options value, in <parameter> tag must be an array');
+ }
+
+ $properties['options'] = $this->arrayToString($properties['options']);
+ }
+
+ $constraints[] = $properties;
+ $this->builder->declareClass("Symfony\\Component\\Validator\\Constraints\\".$properties['validator']);
+ }
+
+ return $this->renderTemplate('objectLoadValidatorMetadata', array('constraints' => $constraints));
+
@willdurand

willdurand Mar 29, 2012

Owner

useless blank line

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+
+ }
+
+ /**
+ * Convenience method that takes an array and gives a string representing its php definition.
+ * This method will recurse into deeper arrays.
+ *
+ * @param array $array Array to process
+ * @param boolean $deep true if it's a recursive call
+ * @return string The php definition of input array
+ */
+ protected function arrayToString ($array, $deep = false)
+ {
+ $string = "array(";
+ foreach ($array as $key => $value)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ $this->builder->declareClass("Symfony\\Component\\Validator\\Constraints\\".$properties['validator']);
+ }
+
+ return $this->renderTemplate('objectLoadValidatorMetadata', array('constraints' => $constraints));
+
+ }
+
+ /**
+ * Convenience method that takes an array and gives a string representing its php definition.
+ * This method will recurse into deeper arrays.
+ *
+ * @param array $array Array to process
+ * @param boolean $deep true if it's a recursive call
+ * @return string The php definition of input array
+ */
+ protected function arrayToString ($array, $deep = false)
@willdurand

willdurand Mar 29, 2012

Owner

don't put an extra space before the opening curly bracket

@willdurand

willdurand Apr 15, 2012

Owner

btw what's the aim of this method? Is it unit tested?

@cristianoc72

cristianoc72 Apr 19, 2012

Member

Removed. In php, var_export function does the same thing.

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ *
+ * @param array $array Array to process
+ * @param boolean $deep true if it's a recursive call
+ * @return string The php definition of input array
+ */
+ protected function arrayToString ($array, $deep = false)
+ {
+ $string = "array(";
+ foreach ($array as $key => $value)
+ {
+ $string .= "'$key' => ";
+
+ if (is_array($value))
+ {
+ $string .= $this->arrayToString($value, true);
+ }

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ $string .= $this->arrayToString($value, true);
+ }
+ else
+ {
+ $string .= "'$value', ";
+ }
+ }
+ $string .= ")";
+
+ if ($deep)
+ {
+ $string .= ", ";
+ }
+
+ return $string;
+
@willdurand

willdurand Mar 29, 2012

Owner

no need for this extra line

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ {
+ $string .= "'$key' => ";
+
+ if (is_array($value))
+ {
+ $string .= $this->arrayToString($value, true);
+ }
+ else
+ {
+ $string .= "'$value', ";
+ }
+ }
+ $string .= ")";
+
+ if ($deep)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+
+ /**
+ * Adds the validate() method.
+ * @return string The code to be added to model class
+ */
+ protected function addValidateMethod()
+ {
+ $table = $this->getTable();
+ $foreignKeys = $table->getForeignKeys();
+ $hasForeignKeys = (count($foreignKeys) != 0);
+ $aVarNames = array();
+ $refFkVarNames = array();
+ $collVarNames = array();
+
+ if ($hasForeignKeys)
+ {
@willdurand

willdurand Mar 29, 2012

Owner

CS for the whole method

@willdurand willdurand commented on the diff Mar 29, 2012

...or/Validate/templates/objectGetValidationFailures.php
@@ -0,0 +1,12 @@
+
+/**
+ * Gets any ConstraintViolation objects that resulted from last call to validate().
+ *
+ *
@willdurand

willdurand Mar 29, 2012

Owner

useless extra line

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...or/Validate/templates/objectLoadValidatorMetadata.php
@@ -0,0 +1,13 @@
+
+/**
+ * Configure validators constraints. The Validator object uses this method
+ * to perform object validation.
+ *
+ * @param ClassMetadata $metadata
+ */
+public static function loadValidatorMetadata(ClassMetadata $metadata)
@willdurand

willdurand Mar 29, 2012

Owner

should be static public for now

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...erator/Behavior/Validate/templates/objectValidate.php
@@ -0,0 +1,73 @@
+
+
+/**
+ * Validates the object and all objects related to this table.
+ *
+ * @param object $validator A Validator class instance
+ * @return boolean Whether all objects pass validation.
+ * @see doValidate()
+ * @see getValidationFailures()
+ */
+public function validate(Validator $validator = null)
+{
+ if (is_null($validator))
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

...erator/Behavior/Validate/templates/objectValidate.php
+ * @param object $validator A Validator class instance
+ * @return boolean Whether all objects pass validation.
+ * @see doValidate()
+ * @see getValidationFailures()
+ */
+public function validate(Validator $validator = null)
+{
+ if (is_null($validator))
+ {
+ $validator = new Validator(new ClassMetadataFactory(new StaticMethodLoader()), new ConstraintValidatorFactory());
+ }
+
+ $failureMap = new ConstraintViolationList();
+
+ if (!$this->alreadyInValidation)
+ {

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

.../Generator/Behavior/Validate/ValidateBehaviorTest.php
+ *
+ * @author Cristiano Cinotti
+ */
+class ValidateBehaviorTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @private array The names of ValidateAuthor, ValidateBook, ValidatePublisher, ValidateReader classes.
+ * This classes are created by test:prepare command
+ */
+ private $classes;
+
+
+ public function assertPreConditions()
+ {
+ if (!class_exists('Propel\Tests\Bookstore\Behavior\ValidateAuthor'))
+ {
@willdurand

willdurand Mar 29, 2012

Owner

bad CS on brackets for the whole class

@willdurand willdurand and 1 other commented on an outdated diff Mar 29, 2012

.../Generator/Behavior/Validate/ValidateBehaviorTest.php
+<database name="bookstore-behavior">
+ <table name="validate_author" description="Author Table">
+ <column name="id" required="true" primaryKey="true" autoIncrement="true" type="INTEGER" description="Author Id" />
+ <column name="first_name" required="true" type="VARCHAR" size="128" description="First Name" />
+ <behavior name="validate">
+ <parameter name="rule1" value="{validator: NotNull}" />
+ <parameter name="rule2" value="{column: first_name, validator: MaxLength, options: {limit: 128}}" />
+ </behavior>
+ </table>
+ </database>
+EOF;
+ QuickBuilder::buildSchema($schema);
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
@willdurand

willdurand Mar 29, 2012

Owner

You may use the Propel exception by adding the Full Qualified Class Name here:

@expectedException \Propel\...\InvalidArgumentException
@willdurand

willdurand Mar 31, 2012

Owner

I suggest you to run the check_cs script. See the README for more information.

Le 31 mars 2012 à 00:33, Cristiano Cinottireply@reply.github.com a écrit :

+

  • <column name="id" required="true" primaryKey="true" autoIncrement="true" type="INTEGER" description="Author Id" />
    
  • <column name="first_name" required="true" type="VARCHAR" size="128" description="First Name" />
    
  • <behavior name="validate">
    
  •   <parameter name="rule1" value="{validator: NotNull}" />
    
  •   <parameter name="rule2" value="{column: first_name, validator: MaxLength, options: {limit: 128}}" />
    
  •  </behavior>
    

+EOF;

  •    QuickBuilder::buildSchema($schema);
    
  • }
  • /**
  • \* @expectedException  InvalidArgumentException
    

Fixed


Reply to this email directly or view it on GitHub:
https://github.com/propelorm/Propel2/pull/156/files#r628937

Owner

willdurand commented Mar 31, 2012

I think we are almost ready.

@cristianoc72 if you fixed all comments, then just exec the check_cs script (see the README), and tell me your mind about these questions:

  • Do we cover all existing validators provided by the Symfony2 Validator component?
  • And, all old validators in Propel 1.6?
  • Can we easily add a new validator aka custom validator?
Member

cristianoc72 commented Mar 31, 2012

I run check_cs script and it gives a list of files, without any error message: it's ok, isn't it?
Well, I'm working on I18n and concrete_inheritance behaviors, as @fzaninotto suggested. I 'ld be ready in a couple of days. If you prefere to merge the code as is, by now, I'll submit a new PR, when I'll finish this piece of work.

About the questions:

  1. By now, we cover all existing validators provided by the Symfony2 Validator component, except UniqueEntity, because it's written specifically for Doctrine. Date, DateTime and Time, when Propel is set to use Date objects, don't work as I expected. In fact, in that case, Propel creates a Date object before performing validation, so if an incorrect value is provided, Propel throws an exception before the validator can intervene.
  2. We cover all Propel 1 validators, except UniqueValidator (see previous point) as the following:
    MatchValidator --> Regex, with option match = true
    NotMatchValidator --> Regex, with option match = false
    MaxLengthValidator --> MaxLength
    MinLengthValidator --> MinLength
    MaxValueValidator --> Max
    MinValueValidator --> Min
    RequiredValidator --> NotBlank (if you want to consider also blank string as null) or NotNull
    ValidValuesValidator --> Choices
    TypeValidator --> Type
    Imho, next step should be creating a unique validator, working with Propel ActiveRecord, instead of Doctrine entity, then submit it to Symfony team.
  3. Creating a custom validator constraint is easy: see (http://symfony.com/doc/current/cookbook/validation/custom_constraint.html). Now that I think, we should provide a way to specify the path of custom validators, to correctly load it: via build.properties?
Owner

willdurand commented Apr 1, 2012

By now, we cover all existing validators provided by the Symfony2 Validator component, except UniqueEntity, because it's written specifically for Doctrine. Date, DateTime and Time, when Propel is set to use Date objects, don't work as I expected. In fact, in that case, Propel creates a Date object before performing validation, so if an incorrect value is provided, Propel throws an exception before the validator can intervene.

Ok, the UniqueEntity is another topic, not related to Propel2 itself.

For the date/datetime/time, I'm ok with the current behavior. Propel should be able to speak if something is wrong…
@fzaninotto agreed?

We cover all Propel 1 validators, except UniqueValidator (see previous point) as the following:
MatchValidator --> Regex, with option match = true
NotMatchValidator --> Regex, with option match = false
MaxLengthValidator --> MaxLength
MinLengthValidator --> MinLength
MaxValueValidator --> Max
MinValueValidator --> Min
RequiredValidator --> NotBlank (if you want to consider also blank string as null) or NotNull
ValidValuesValidator --> Choices
TypeValidator --> Type

Ok so the UniqueValidator could be the UniqueModel (or Entity) we will write later for the Validator component. Everything seems ok then, I saw you tried new validators too (Url for instance).

Imho, next step should be creating a unique validator, working with Propel ActiveRecord, instead of Doctrine entity, then submit it to Symfony team.

Agreed, we will see that later. It's not critical.

Creating a custom validator constraint is easy: see (http://symfony.com/doc/current/cookbook/validation/custom_constraint.html). Now that I think, we should provide a way to specify the path of custom validators, to correctly load it: via build.properties?

Nope, the build.properties won't be used anymore. Thanks to the autoloading component, we don't care about how to get things loaded. We just have to specify our new constraints somewhere in the autoloader config, and it will be available.
Don't focus on that, if we can write new constraints as easily as it's written in the Sf2 documentation, that's perfect.

So, once you've finished your current work:

  • clean your code using the check_cs fix command;
  • rebase your branch;
  • update your PR.

Thank you so much.

@themouette @mazenovi @jaugustin @fzaninotto @havvg heya folks, just to be sure. Are you ok with the outlines of this PR? Do you feel comfortable with how we define rules in the schema.xml?

<parameter name="rule3" value="{column: reader_id, validator: Type, options: {type: integer}}" />

Cheers!

Member

cristianoc72 commented Apr 1, 2012

Sorry but....wich command I must use to rebase my branch? git rebase validate-behavior ??

Owner

willdurand commented Apr 2, 2012

Nope, git rebase master but for now, it's useless You can forget this part for the moment.

Member

havvg commented Apr 2, 2012

Hey there,

I just took a look at the rule XML, it looks strange to me, that the XML structure contains a non-xml (JSON) structure in it.
The XML should express the validation in XML, too, shouldn't it? This also makes it easier, as the validations can be put into an XSD to aid creating the XML using sensitive editors.

Member

cristianoc72 commented Apr 2, 2012

Well, when I started my work, my first intention was to maintain the same syntax, as much as possible. I would have loved to write something like following:
<behavior name="validate">
<rule column="mycolumn" name="minLength" value="4" message="My message here" />
</behavior>
But I confess that, after a few nights spent on Propel code, I could not figure out how to do. Or better, I don't feel able to mess around too much in the refactoring of Propel builder system.
So, the second choice was trying to pass an array, representing the rule, in value attribute of <parameter> tag. The easiest way seemed to pass an array in Yaml format, because it's very readable and there's nothing to escape. That's the story.
Of course, If someone suggests me a way to return to the previous syntax, as described below, I'm still very happy to try to implemnt it!

Member

fzaninotto commented Apr 2, 2012

I don't care about the JSON in XML. In fact, XML is just one representation of the schema, and probably not the best. PHP or YAML schema representations don't care about having a JSON parameter. Also, the XML validation prevents extensibility for future validators.

I also think that a Unique validator should be in the Propel core, not in Symfony.

For the Date validator, what's the alternative? Converting in the background before validation? That sounds like a fair default.

Owner

willdurand commented Apr 3, 2012

XML is fine IMO but this is another discussion anyway.

We probably could provide a full XML syntax but... I like the current syntax. Even if it's weird to find some JSON stuffs in a XML schema, we have a short syntax, extensible, and readable.

Basically, this validate behavior embeds the Validator component, so a Unique validator is well located in it. No need to add more code to the core IMO.

For the Date validator, let's keeping the current behavior.

Member

cristianoc72 commented Apr 5, 2012

After a couple of days of meditation, I realized that we should provide Unique validator. Whether we like it or not, "Unique" is one of the most useful (and used) validators.
At the beginning of sf2 Validator component, UniqueEntity constraint was included in it. But a Unique validator submits a query to the database, to perform its action, so it's not "model agnostic" and it was moved to Doctrine Bridge.
So, it's our responsability to provide our own Unique constraint, for Propel users.

Now, the question is: where do we put our Unique constraint, and all the other constraints we'll decide to provide in the future?
I can see 2 solutions (may be there are many more):

  1. we can put them directly in `Propel\Runtime\Validator\Constraints` namespace
    
  2. we can create a separate package (i.e. PropelValidator), and install it in `vendor` path, via composer
    

The first solution seems easier, the second solution can make happy our great leader, because it keeps everything out of Propel core ;-)

Anyway, this week end I'll develop our Unique constraint.

Happy Easter to you all !

Member

fzaninotto commented Apr 5, 2012

+1 for solution 1

Owner

willdurand commented Apr 5, 2012

+1 for solution 1

Member

havvg commented Apr 5, 2012

+1 on 1 :)

Member

cristianoc72 commented Apr 10, 2012

Hi there,
Unique validator is done and all comments are fixed.
I'm reviewing the whole documentation, to correct any other nods to the previous validation system.
I ask you a hint: should I remove the chapter 05-validator or redirect it to the validate-behavior documentation?

Owner

willdurand commented Apr 10, 2012

sure you can rename what you want in the doc ;)

we will have to completely rewrite the doc anyway (when I say "rewrite", it's more reorganize it than rewrite the doc from scratch).

Owner

willdurand commented Apr 14, 2012

@cristianoc72 what's the plan now?

Member

cristianoc72 commented Apr 14, 2012

My work is almost finished. Unique validate is provided and documentation
review (to correct references to previous validation system) is done. I
think you should review the code and then merge it, if there's nothing to
fix again :-)
Il giorno 14/apr/2012 17.48, "William DURAND" <
reply@reply.github.com>
ha scritto:

@cristianoc72 what's the plan now?


Reply to this email directly or view it on GitHub:
#156 (comment)

Owner

willdurand commented Apr 14, 2012

Ok so first, we will merge the om-rename branch. Then, we will see if this PR needs to be rebased, or not. And the same time I will review it. That's the plan :)

Member

cristianoc72 commented Apr 15, 2012

awesome! let's go!

Owner

willdurand commented Apr 15, 2012

Ok so, a rebase is required. Can you follow these steps:

1 - Ensure you're on the master

git checkout master

2 - Ensure you are up to date

git fetch origin
git merge --ff-only origin/master

3 - Now go to your branch

git checkout validate-behavior

4 - Rebase

git rebase master

I think you will have to fix conflicts. Each time the rebase process stops, you can know which files have to be fixed:

git status

Just fix them, and then:

git add <file1> <file2> ...
git rebase --continue

5 - Run the test suite, and enjoy if it's green :)

6 - Push your changes:

git push origin validate-behavior -f

7 - That's all.

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

composer.lock
@@ -1,15 +1,14 @@
{
@willdurand

willdurand Apr 15, 2012

Owner

@cristianoc72 When you will rebase your branch, don't keep this file.

@cristianoc72

cristianoc72 Apr 16, 2012

Member

Should I include it in .gitignore?

@willdurand willdurand commented on the diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
@@ -0,0 +1,288 @@
+<?php
@willdurand

willdurand Apr 15, 2012

Owner

Can you add a blank line here.

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
@@ -0,0 +1,288 @@
+<?php
+/**
+ * This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @license MIT License
+ */
+
+namespace Propel\Generator\Behavior\Validate;
+
+use Propel\Generator\Model\Behavior;
@willdurand

willdurand Apr 15, 2012

Owner

Can you sort the use statements?

@willdurand willdurand commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
@@ -0,0 +1,288 @@
+<?php
+/**
@willdurand

willdurand Apr 15, 2012

Owner

blank line needed before the license header.

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ public function objectAttributes()
+ {
+ return $this->renderTemplate('objectAttributes');
+ }
+
+ /**
+ * Returns the parameters associated with a given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ * @return array The array of parameters associated to given column
+ */
+ public function getParametersFromColumnName($columnName = '')
+ {
+ if ('' == $columnName) {
+
@willdurand

willdurand Apr 15, 2012

Owner

no need for an extra space

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ *
+ * @return string The code to be added to model class
+ */
+ public function objectAttributes()
+ {
+ return $this->renderTemplate('objectAttributes');
+ }
+
+ /**
+ * Returns the parameters associated with a given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ * @return array The array of parameters associated to given column
+ */
+ public function getParametersFromColumnName($columnName = '')
@willdurand

willdurand Apr 15, 2012

Owner

you should use null instead of empty string, same for the condition below

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ if ($parameter['column'] == $columnName) {
+ $outArray[$key] = $parameter;
+ }
+ }
+
+ return $outArray;
+ }
+ }
+
+ /**
+ * Remove parameters associated with given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ */
+ public function removeParametersFromColumnName($columnName = '')

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ }
+ }
+
+ return $outArray;
+ }
+ }
+
+ /**
+ * Remove parameters associated with given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ */
+ public function removeParametersFromColumnName($columnName = '')
+ {
+ if ($columnName != '') {
@willdurand

willdurand Apr 15, 2012

Owner
if (null !== $columnName) {

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ $rule['auto_rule']['column'] = $pk[0]->getName();
+ $rule['auto_rule']['validator'] = 'Type';
+ $rule['auto_rule']['options'] = array('type' => $pk[0]->getPhpType());
+
+ $this->setParameters($rule);
+ }
+ }
+
+ /**
+ * Merge $paramArray array into parameters array.
+ * This method avoid that there are rules with the same name, when adding parameters programmatically.
+ * Useful for Concrete Inheritance behavior.
+ */
+ public function mergeParameters($paramsArray = null)
+ {
+ if (!is_null($paramsArray)) {
@willdurand

willdurand Apr 15, 2012

Owner
if (null !== $params) {

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ if (count($this->getParameters()) <= 0) {
+ $pk = $this->getTable()->getPrimaryKey();
+ $rule['auto_rule']['column'] = $pk[0]->getName();
+ $rule['auto_rule']['validator'] = 'Type';
+ $rule['auto_rule']['options'] = array('type' => $pk[0]->getPhpType());
+
+ $this->setParameters($rule);
+ }
+ }
+
+ /**
+ * Merge $paramArray array into parameters array.
+ * This method avoid that there are rules with the same name, when adding parameters programmatically.
+ * Useful for Concrete Inheritance behavior.
+ */
+ public function mergeParameters($paramsArray = null)
@willdurand

willdurand Apr 15, 2012

Owner

if you expect an array, then add the type in the signature, but don't use this kind of Hungarian notation.

@willdurand willdurand and 1 other commented on an outdated diff Apr 15, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ }
+ }
+ $this->setParameters($params);
+ }
+
+ /**
+ * Add loadValidatorMetadata() method
+ *
+ * @return string
+ */
+ protected function addLoadValidatorMetadataMethod()
+ {
+ $params = $this->getParameters();
+ $constraints = array();
+
+ foreach ($params as $key=>$properties)
@willdurand

willdurand Apr 15, 2012

Owner

add spaces between $key and $properties

Owner

willdurand commented May 1, 2012

@cristianoc72 what's the state of this PR?

Member

cristianoc72 commented May 1, 2012

@willdurand I'm fixing the last "red" test, then I'll push these fixes. It's a harder work then I supposed !

Owner

willdurand commented May 7, 2012

@cristianoc72 is it ok? Can you rebase (once again) your PR?

Member

cristianoc72 commented May 7, 2012

@willdurand it's ok. Of course I can. I'll rebase it tonight, after my little daughters will fall asleep :-)

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...r/ConcreteInheritance/ConcreteInheritanceBehavior.php
@@ -108,6 +102,11 @@ public function modifyTable()
if ($behavior->getName() == 'concrete_inheritance_parent' || $behavior->getName() == 'concrete_inheritance') {
continue;
}
+ //validate behavior. If validate behavior already exists, clone only rules from parent
+ if (($behavior->getName() == 'validate') && ($table->hasBehavior('validate'))) {
@willdurand

willdurand May 7, 2012

Owner

Please change to:

if ('validate' === $behavior->getName() && $table->hasBehavior('validate')) {

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...r/ConcreteInheritance/ConcreteInheritanceBehavior.php
@@ -108,6 +102,11 @@ public function modifyTable()
if ($behavior->getName() == 'concrete_inheritance_parent' || $behavior->getName() == 'concrete_inheritance') {
continue;
}
+ //validate behavior. If validate behavior already exists, clone only rules from parent
+ if (($behavior->getName() == 'validate') && ($table->hasBehavior('validate'))) {
+ $table->getBehavior('validate')->mergeParameters($behavior->getParameters());
+ continue;
@willdurand

willdurand May 7, 2012

Owner

can you add a blank line before continue;

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

src/Propel/Generator/Behavior/I18n/I18nBehavior.php
@@ -141,18 +143,33 @@ protected function moveI18nColumns()
}
$column = $table->getColumn($columnName);
// add the column
- $i18nTable->addColumn(clone $column);
- // add related validators
- if ($validator = $column->getValidator()) {
- $i18nTable->addValidator(clone $validator);
+ $i18nColumn = $i18nTable->addColumn(clone $column);
+
+ //validate behavior: move rules associated to the column
+ if ($table->hasBehavior('validate')) {
+ $vBehavior = $table->getBehavior('validate');
@willdurand

willdurand May 7, 2012

Owner

Can you rename the variable to validateBehavior, just to clarify :)

@cristianoc72

cristianoc72 May 7, 2012

Member

Fixed. Damn, my style should be improved!!

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ /**
+ * Add behavior methods to model class
+ *
+ * @return string
+ */
+ public function objectMethods($builder)
+ {
+ $array = $this->getParameters();
+ if (empty($array)) {
+ throw new InvalidArgumentException('Please, define your rules for validation.');
+ }
+ $this->cleanupParameters();
+
+ $this->builder = $builder;
+
+ $this->builder->declareClasses('Symfony\\Component\\Validator\\Mapping\\ClassMetadata', 'Symfony\\Component\\Validator\\Validator', 'Symfony\\Component\\Validator\\Mapping\\Loader\\StaticMethodLoader', 'Symfony\\Component\\Validator\\ConstraintValidatorFactory', 'Symfony\\Component\\Validator\\Mapping\\ClassMetadataFactory', 'Symfony\\Component\\Validator\\ConstraintViolationList');
@willdurand

willdurand May 7, 2012

Owner

You can use more than one line to declare these classes :)

@cristianoc72

cristianoc72 May 7, 2012

Member

Ops, Phpstorm has automatic wrapping.....

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ /**
+ * Returns the parameters associated with a given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ * @return array The array of parameters associated to given column
+ */
+ public function getParametersFromColumnName($columnName = null)
+ {
+ if (null == $columnName) {
+ return array();
+ } else {
+ $outArray = array();
+ $this->cleanupParameters();
+ $parameters = $this->getParameters();
+ foreach ($parameters as $key=>$parameter) {
@willdurand

willdurand May 7, 2012

Owner

can you add spaces before and after =>?

@cristianoc72

cristianoc72 May 7, 2012

Member

Fixed. And the same at line 100

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ */
+ public function objectAttributes()
+ {
+ return $this->renderTemplate('objectAttributes');
+ }
+
+ /**
+ * Returns the parameters associated with a given column.
+ * Useful for i18n behavior
+ *
+ * @param string The column name
+ * @return array The array of parameters associated to given column
+ */
+ public function getParametersFromColumnName($columnName = null)
+ {
+ if (null == $columnName) {
@willdurand

willdurand May 7, 2012

Owner

Maybe it's better to write:

$array = array();

if (null !== $columnName) {
    $this->cleanupParameters();
    foreach ($this->getParameters() as $key => $parameter) {
        if ($parameter['column'] === $columnName) {
            $array[$key] = $parameter;
        }
    }
}

return $array;
@cristianoc72

cristianoc72 May 7, 2012

Member

Not "maybe": sure it's better!

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ }
+ $this->setParameters($params);
+ }
+
+ /**
+ * Add loadValidatorMetadata() method
+ *
+ * @return string
+ */
+ protected function addLoadValidatorMetadataMethod()
+ {
+ $params = $this->getParameters();
+ $constraints = array();
+
+ foreach ($params as $key => $properties)
+ {
@willdurand

willdurand May 7, 2012

Owner

This should be on the same line than foreach

@willdurand willdurand commented on the diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ /**
+ * Adds the validate() method.
+ * @return string The code to be added to model class
+ */
+ protected function addValidateMethod()
+ {
+ $table = $this->getTable();
+ $foreignKeys = $table->getForeignKeys();
+ $hasForeignKeys = (count($foreignKeys) != 0);
+ $aVarNames = array();
+ $refFkVarNames = array();
+ $collVarNames = array();
+
+ if ($hasForeignKeys) {
+ foreach ($foreignKeys as $fk) {
+ $aVarNames[] = $this->builder->getFKVarName($fk);
@willdurand

willdurand May 7, 2012

Owner

what does aVarNames mean? :p
Change the variable name if you can :)

@cristianoc72

cristianoc72 May 7, 2012

Member

I followed the naming convention of foreign keys, inside model classes: $aSomething and $collSomethingElse are the arrays of 1-n and n-1 relations keys. I can't find a better name (maybe little imagination...)

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...opel/Generator/Behavior/Validate/ValidateBehavior.php
+ else {
+ $collVarNames[] = $this->builder->getRefFKCollVarName($refFK);
+ }
+ }
+
+ return $this->renderTemplate('objectValidate', array(
+ 'hasForeignKeys' => $hasForeignKeys,
+ 'aVarNames' => $aVarNames,
+ 'refFkVarNames' => $refFkVarNames,
+ 'collVarNames' => $collVarNames
+ ));
+ }
+
+ /**
+ * Adds the getValidationFailures() method.
+ * @param string &$script The script will be modified in this method.
@willdurand

willdurand May 7, 2012

Owner

there is no parameter here

@cristianoc72

cristianoc72 May 7, 2012

Member

Damn copy&paste !!
Fixed

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...erator/Behavior/Validate/templates/objectValidate.php
@@ -0,0 +1,62 @@
+
+/**
+ * Validates the object and all objects related to this table.
+ *
+ * @param object $validator A Validator class instance
+ * @return boolean Whether all objects pass validation.
+ * @see doValidate()
@willdurand

willdurand May 7, 2012

Owner

Maybe the @see should be after the description, and before the @param/@return statements

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...erator/Behavior/Validate/templates/objectValidate.php
@@ -0,0 +1,62 @@
+
+/**
+ * Validates the object and all objects related to this table.
+ *
+ * @param object $validator A Validator class instance
+ * @return boolean Whether all objects pass validation.
+ * @see doValidate()
+ * @see getValidationFailures()
+ */
+public function validate(Validator $validator = null)
+{
+ if (is_null($validator)) {
@willdurand

willdurand May 7, 2012

Owner
if (null === $validator) {
@cristianoc72

cristianoc72 May 7, 2012

Member

Fixed. And the same at line 44

@willdurand willdurand and 2 others commented on an outdated diff May 7, 2012

...erator/Behavior/Validate/templates/objectValidate.php
+ foreach ($this-><?php echo $collVarName; ?> as $referrerFK) {
+ if (method_exists($referrerFK, 'validate')) {
+ if (!$referrerFK->validate($validator)) {
+ $failureMap->addAll($referrerFK->getValidationFailures());
+ }
+ }
+ }
+ }
+<?php endforeach; ?>
+
+ $this->alreadyInValidation = false;
+ }
+
+ $this->validationFailures = $failureMap;
+
+ return (bool) (!(count($this->validationFailures) > 0));
@willdurand

willdurand May 7, 2012

Owner

I'm not sure bool is valid. We use to use Boolean

@hhamon

hhamon May 7, 2012

Member

(bool) is valid in PHP but we use (Boolean) to follow the Symfony coding standards

@cristianoc72

cristianoc72 May 7, 2012

Member

Fine.
Fixed

@willdurand willdurand and 2 others commented on an outdated diff May 7, 2012

src/Propel/Generator/Builder/Util/XmlToAppData.php
@@ -242,11 +240,7 @@ public function startElement($parser, $name, $attributes)
$this->currVendorObject = $this->currTable->addVendorInfo($attributes);
break;
- case 'validator':
- $this->currValidator = $this->currTable->addValidator($attributes);
- break;
-
- case 'id-method-parameter':
+ case "id-method-parameter":
@willdurand

willdurand May 7, 2012

Owner

Why these changes? And, I think this class no more exist (renamed to SchemaParser)

@hhamon

hhamon May 7, 2012

Member

It has been renamed SchemaReader

@cristianoc72

cristianoc72 May 7, 2012

Member

My intent was only to remove lines from 245 to 247. I removed also line 249 by mistake and then I added it again.
Should I modify this line, too? Imho it's not needed by now.

@willdurand

willdurand May 8, 2012

Owner

Rebase your PR, and it will fix this stuff I guess

@willdurand willdurand and 1 other commented on an outdated diff May 7, 2012

...pel/Runtime/Validator/Constraints/UniqueValidator.php
+ {
+ $object = $this->context->getRoot();
+ $className = get_class($object);
+ $peer = $object->getPeer();
+ $colName = $this->context->getCurrentProperty();
+ $colName = $peer->translateFieldName($colName, BasePeer::TYPE_STUDLYPHPNAME, BasePeer::TYPE_PHPNAME);
+ $query = call_user_func($className.'Query::create');
+ $filter = 'filterBy'.$colName;
+ $ret = $query->$filter($value)->count();
+ if ($ret >0) {
+ $this->setMessage($constraint->message);
+
+ return true;
+ } else {
+ return false;
+ }
@willdurand

willdurand May 7, 2012

Owner

Maybe it's better to write:

$object     = $this->context->getRoot();
$peer       = $object->getPeer();
$className  = get_class($object);
$queryClass = $className . 'Query';
$filter     = sprintf('filterBy%s', $peer->translateFieldName(
    $this->context->getCurrentProperty(), BasePeer::TYPE_STUDLYPHPNAME, BasePeer::TYPE_PHPNAME
));

if (0 < $queryClass::create()->$filter($value)->count()) {
    $this->setMessage($constraint->message);

    return true;
}

return false;
Owner

willdurand commented May 7, 2012

Ok, so I made my very last comments. Once, fixed/rebased, I'll merge this awesome work!

willdurand and others added some commits Dec 16, 2011

This pull request passes (merged bac1b58 into 3435113).

Member

cristianoc72 commented May 8, 2012

Rebased and pushed.
When it'll be merged, GitHub'll reward me for the "most commented ever" PR :-)))

The awesom work is yours, in discovering my mistakes!!

Owner

willdurand commented May 8, 2012

hehe :) Let's go!

@willdurand willdurand added a commit that referenced this pull request May 8, 2012

@willdurand willdurand Merge pull request #156 from cristianoc72/validate-behavior
Remove validation part from Propel core and add Validate behavior
e9d9875

@willdurand willdurand merged commit e9d9875 into propelorm:master May 8, 2012

Owner

willdurand commented May 8, 2012

Thank you for your work @cristianoc72!

Member

fzaninotto commented May 9, 2012

I just wanted to add one more comment: Awesome.

Member

hhamon commented May 9, 2012

Applause!

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