merged branch Garfield-fr/master (PR #5015)
Commits -------f1c4b8b[Doctrine Bridge] Added a parameter ignoreNull on Unique entity to allow a nullable value on field. Added Test Discussion ---------- [Doctrine Bridge] Added parameter ignoreNull to accept a nullable value on field In my last project, i use this syntax to test unicity on 2 fields, but it fail because the validator stop if value is null. I dropped the test on validator and my unicity work fine. ``` @UniqueEntity(fields={"username", "deletedAt"}) ``` It's possible to add this PR on Bridge. Thanks Bertrand --------------------------------------------------------------------------- by stof at 2012-07-23T08:14:19Z This is wrong. RDBMS allow several null values in a unique column and this change will break it. --------------------------------------------------------------------------- by henrikbjorn at 2012-07-23T08:17:08Z @stof seems weird indeed it would return if any of the values are null. Makes sense to do a query where the field `IS NULL` or whatever the find method does. --------------------------------------------------------------------------- by stof at 2012-07-23T08:18:50Z @henrikbjorn if you do a query with IS NULL, the validator would force to have only 1 entity with a null field whereas it is not the behavior of the DB-level constraint. --------------------------------------------------------------------------- by henrikbjorn at 2012-07-23T08:20:41Z In this case i suspect that he wants to achieve a `WHERE username = "henrikbjorn" AND deletedAt IS NULL` which would be valid right? Currently it just returns if any of the fields are null and the validation is never done. --------------------------------------------------------------------------- by bschussek at 2012-07-23T08:27:24Z I suggest to make this configurable as the handling of NULL values in UNIQUE columns [differs between SQL implementations](http://forums.mysql.com/read.php?22,53591,53591#msg-53591). --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T08:52:53Z @stof What the correct solution to test my unicity with deletedAt == null ? I use this definition: @ORM\Column(name="deleted_at", type="datetime", nullable=true) --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T20:28:44Z In my local repository, i added a new parameter "$authorizedNullField" on UniqueEntity.php and tested this on UniqueEntityValidator.php: Code: https://gist.github.com/4122efbe569e3c2c95c0 What about that ? Thanks for your help --------------------------------------------------------------------------- by stof at 2012-07-23T20:45:30Z yep, this would be good (except for the naming which seems weird) --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T20:46:44Z No problem to change this. I don't find a good name for this ? --------------------------------------------------------------------------- by stof at 2012-07-23T20:47:57Z what about ``allowMultipleNull`` (defaulting to ``true`` for BC) ? --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T20:51:30Z Why multiple ? This option is for one or many. what about `allowNullable` ? --------------------------------------------------------------------------- by stof at 2012-07-23T20:52:44Z @Garfield-fr the current behavior allows having multiple null values without failing to the unique constraint --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T20:56:07Z ok. I make `allowMultipleNull`. It's ok with that: https://gist.github.com/cae8d43780c45a5011ed Thanks --------------------------------------------------------------------------- by bschussek at 2012-07-23T20:58:12Z What about `uniqueNull` (`false` by default)? `ignoreNull` (`true` by default)? I find `allowMultipleNull` a bit cumbersome. --------------------------------------------------------------------------- by stof at 2012-07-23T20:58:26Z no it is not. You have an issue in the validator. You have an extra negation. Please update your PR --------------------------------------------------------------------------- by Garfield-fr at 2012-07-23T21:01:59Z @stof `ignoreNull` is ok for you ? --------------------------------------------------------------------------- by stof at 2012-07-23T21:10:24Z yes --------------------------------------------------------------------------- by fabpot at 2012-08-05T07:48:03Z Is it mergeable now? Is yes, @Garfield-fr Can you squash your commits? --------------------------------------------------------------------------- by travisbot at 2012-08-05T08:43:23Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2039523) (merged 19ae3cf9 intoc20c1d18). --------------------------------------------------------------------------- by stof at 2012-08-05T12:09:02Z @Garfield-fr when squashing the commits, you need to force the push as you are rewriting the history. You should not have merged with your remote branch --------------------------------------------------------------------------- by Garfield-fr at 2012-08-05T12:10:15Z What's the right solution for resolve this ? --------------------------------------------------------------------------- by stof at 2012-08-05T12:11:09Z @Garfield-fr reset your local branch to the squashed commit and force the push --------------------------------------------------------------------------- by Garfield-fr at 2012-08-05T12:14:09Z @stof Thanks for your help --------------------------------------------------------------------------- by travisbot at 2012-08-05T12:19:06Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2040210) (mergedf1c4b8b4into20d2e5a1).
This commit is contained in:
Fabien Potencier
commit
0ccda38775
|
|
@ -0,0 +1,36 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* This file is part of the Symfony package.
|
||||
*
|
||||
* (c) Fabien Potencier <fabien@symfony.com>
|
||||
*
|
||||
* For the full copyright and license information, please view the LICENSE
|
||||
* file that was distributed with this source code.
|
||||
*/
|
||||
|
||||
namespace Symfony\Bridge\Doctrine\Tests\Fixtures;
|
||||
|
||||
use Doctrine\ORM\Mapping\Id;
|
||||
use Doctrine\ORM\Mapping\Column;
|
||||
use Doctrine\ORM\Mapping\Entity;
|
||||
|
||||
/** @Entity */
|
||||
class DoubleIdentEntity
|
||||
{
|
||||
/** @Id @Column(type="integer") */
|
||||
protected $id;
|
||||
|
||||
/** @Column(type="string") */
|
||||
public $name;
|
||||
|
||||
/** @Column(type="string", nullable=true) */
|
||||
public $name2;
|
||||
|
||||
public function __construct($id, $name, $name2)
|
||||
{
|
||||
$this->id = $id;
|
||||
$this->name = $name;
|
||||
$this->name2 = $name2;
|
||||
}
|
||||
}
|
||||
|
|
@ -13,6 +13,7 @@ namespace Symfony\Bridge\Doctrine\Tests\Validator\Constraints;
|
|||
|
||||
use Symfony\Bridge\Doctrine\Tests\DoctrineOrmTestCase;
|
||||
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity;
|
||||
use Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity;
|
||||
use Symfony\Bridge\Doctrine\Tests\Fixtures\CompositeIdentEntity;
|
||||
use Symfony\Bridge\Doctrine\Tests\Fixtures\AssociationEntity;
|
||||
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
|
||||
|
|
@ -114,7 +115,7 @@ class UniqueValidatorTest extends DoctrineOrmTestCase
|
|||
return $validatorFactory;
|
||||
}
|
||||
|
||||
public function createValidator($entityManagerName, $em, $validateClass = null, $uniqueFields = null, $errorPath = null, $repositoryMethod = 'findBy')
|
||||
public function createValidator($entityManagerName, $em, $validateClass = null, $uniqueFields = null, $errorPath = null, $repositoryMethod = 'findBy', $ignoreNull = true)
|
||||
{
|
||||
if (!$validateClass) {
|
||||
$validateClass = 'Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity';
|
||||
|
|
@ -132,7 +133,8 @@ class UniqueValidatorTest extends DoctrineOrmTestCase
|
|||
'fields' => $uniqueFields,
|
||||
'em' => $entityManagerName,
|
||||
'errorPath' => $errorPath,
|
||||
'repositoryMethod' => $repositoryMethod
|
||||
'repositoryMethod' => $repositoryMethod,
|
||||
'ignoreNull' => $ignoreNull
|
||||
));
|
||||
$metadata->addConstraint($constraint);
|
||||
|
||||
|
|
@ -147,6 +149,7 @@ class UniqueValidatorTest extends DoctrineOrmTestCase
|
|||
$schemaTool = new SchemaTool($em);
|
||||
$schemaTool->createSchema(array(
|
||||
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity'),
|
||||
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity'),
|
||||
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\CompositeIdentEntity'),
|
||||
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\AssociationEntity'),
|
||||
));
|
||||
|
|
@ -224,6 +227,35 @@ class UniqueValidatorTest extends DoctrineOrmTestCase
|
|||
$this->assertEquals(0, $violationsList->count(), "No violations found on entity having a null value.");
|
||||
}
|
||||
|
||||
public function testValidateUniquenessWithIgnoreNull()
|
||||
{
|
||||
$entityManagerName = "foo";
|
||||
$validateClass = 'Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity';
|
||||
$em = $this->createTestEntityManager();
|
||||
$this->createSchema($em);
|
||||
$validator = $this->createValidator($entityManagerName, $em, $validateClass, array('name', 'name2'), 'bar', 'findby', false);
|
||||
|
||||
$entity1 = new DoubleIdentEntity(1, 'Foo', null);
|
||||
$violationsList = $validator->validate($entity1);
|
||||
$this->assertEquals(0, $violationsList->count(), "No violations found on entity before it is saved to the database.");
|
||||
|
||||
$em->persist($entity1);
|
||||
$em->flush();
|
||||
|
||||
$violationsList = $validator->validate($entity1);
|
||||
$this->assertEquals(0, $violationsList->count(), "No violations found on entity after it was saved to the database.");
|
||||
|
||||
$entity2 = new DoubleIdentEntity(2, 'Foo', null);
|
||||
|
||||
$violationsList = $validator->validate($entity2);
|
||||
$this->assertEquals(1, $violationsList->count(), "Violation found on entity with conflicting entity existing in the database.");
|
||||
|
||||
$violation = $violationsList[0];
|
||||
$this->assertEquals('This value is already used.', $violation->getMessage());
|
||||
$this->assertEquals('bar', $violation->getPropertyPath());
|
||||
$this->assertEquals('Foo', $violation->getInvalidValue());
|
||||
}
|
||||
|
||||
public function testValidateUniquenessAfterConsideringMultipleQueryResults()
|
||||
{
|
||||
$entityManagerName = "foo";
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ class UniqueEntity extends Constraint
|
|||
public $repositoryMethod = 'findBy';
|
||||
public $fields = array();
|
||||
public $errorPath = null;
|
||||
public $ignoreNull = true;
|
||||
|
||||
public function getRequiredOptions()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -75,7 +75,7 @@ class UniqueEntityValidator extends ConstraintValidator
|
|||
|
||||
$criteria[$fieldName] = $class->reflFields[$fieldName]->getValue($entity);
|
||||
|
||||
if (null === $criteria[$fieldName]) {
|
||||
if ($constraint->ignoreNull && null === $criteria[$fieldName]) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
|
|||
Reference in New Issue