8000 merged branch Garfield-fr/master (PR #5015) · symfony/symfony@0ccda38 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 0ccda38

Browse files
fabpotfabpot
committed
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 into c20c1d1). --------------------------------------------------------------------------- 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) (merged f1c4b8b into 20d2e5a).
2 parents ca9f5e8 + f1c4b8b commit 0ccda38

File tree

4 files changed

+72
-3
lines changed

4 files changed

+72
-3
lines changed

‎src/Symfony/Bridge/Doctrine/Tests/Fixtures/DoubleIdentEntity.php

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bridge\Doctrine\Tests\Fixtures;
13+
14+
use Doctrine\ORM\Mapping\Id;
15+
use Doctrine\ORM\Mapping\Column;
16+
use Doctrine\ORM\Mapping\Entity;
17+
18+
/** @Entity */
19+
class DoubleIdentEntity
20+
{
21+
/** @Id @Column(type="integer") */
22+
protected $id;
23+
24+
/** @Column(type="string") */
25+
public $name;
26+
27+
/** @Column(type="string", nullable=true) */
28+
public $name2;
29+
30+
public function __construct($id, $name, $name2)
31+
{
32+
$this->id = $id;
33+
$this->name = $name;
34+
$this->name2 = $name2;
35+
}
36+
}

‎src/Symfony/Bridge/Doctrine/Tests/Validator/Constraints/UniqueValidatorTest.php

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Bridge\Doctrine\Tests\DoctrineOrmTestCase;
1515
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity;
16+
use Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity;
1617
use Symfony\Bridge\Doctrine\Tests\Fixtures\CompositeIdentEntity;
1718
use Symfony\Bridge\Doctrine\Tests\Fixtures\AssociationEntity;
1819
use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
@@ -114,7 +115,7 @@ protected function createValidatorFactory($uniqueValidator)
114115
return $validatorFactory;
115116
}
116117

117-
public function createValidator($entityManagerName, $em, $validateClass = null, $uniqueFields = null, $errorPath = null, $repositoryMethod = 'findBy')
118+
public function createValidator($entityManagerName, $em, $validateClass = null, $uniqueFields = null, $errorPath = null, $repositoryMethod = 'findBy', $ignoreNull = true)
118119
{
119120
if (!$validateClass) {
120121
$validateClass = 'Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity';
@@ -132,7 +133,8 @@ public function createValidator($entityManagerName, $em, $validateClass = null,
132133
'fields' => $uniqueFields,
133134
'em' => $entityManagerName,
134135
'errorPath' => $errorPath,
135-
'repositoryMethod' => $repositoryMethod
136+
'repositoryMethod' => $repositoryMethod,
137+
'ignoreNull' => $ignoreNull
136138
));
137139
$metadata->addConstraint($constraint);
138140

@@ -147,6 +149,7 @@ private function createSchema($em)
147149
$schemaTool = new SchemaTool($em);
148150
$schemaTool->createSchema(array(
149151
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIdentEntity'),
152+
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity'),
150153
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\CompositeIdentEntity'),
151154
$em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\AssociationEntity'),
152155
));
@@ -224,6 +227,35 @@ public function testValidateUniquenessWithNull()
224227
$this->assertEquals(0, $violationsList->count(), "No violations found on entity having a null value.");
225228
}
226229

230+
public function testValidateUniquenessWithIgnoreNull()
231+
{
232+
$entityManagerName = "foo";
233+
$validateClass = 'Symfony\Bridge\Doctrine\Tests\Fixtures\DoubleIdentEntity';
234+
$em = $this->createTestEntityManager();
235+
$this->createSchema($em);
236+
$validator = $this->createValidator($entityManagerName, $em, $validateClass, array('name', 'name2'), 'bar', 'findby', false);
237+
238+
$entity1 = new DoubleIdentEntity(1, 'Foo', null);
239+
$violationsList = $validator->validate($entity1);
240+
$this->assertEquals(0, $violationsList->count(), "No violations found on entity before it is saved to the database.");
241+
242+
$em->persist($entity1);
243+
$em->flush();
244+
245+
$violationsList = $validator->validate($entity1);
246+
$this->assertEquals(0, $violationsList->count(), "No violations found on entity after it was saved to the database.");
247+
248+
$entity2 = new DoubleIdentEntity(2, 'Foo', null);
249+
250+
$violationsList = $validator->validate($entity2);
251+
$this->assertEquals(1, $violationsList->count(), "Violation found on entity with conflicting entity existing in the database.");
252+
253+
$violation = $violationsList[0];
254+
$this->assertEquals('This value is already used.', $violation->getMessage());
255+
$this->assertEquals('bar', $violation->getPropertyPath());
256+
$this->assertEquals('Foo', $violation->getInvalidValue());
257+
}
258+
227259
public function testValidateUniquenessAfterConsideringMultipleQueryResults()
228260
{
229261
$entityManagerName = "foo";

‎src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntity.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ class UniqueEntity extends Constraint
2727
public $repositoryMethod = 'findBy';
2828
public $fields = array();
2929
public $errorPath = null;
30+
public $ignoreNull = true;
3031

3132
public function getRequiredOptions()
3233
{

‎src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public function validate($entity, Constraint $constraint)
7575

7676
$criteria[$fieldName] = $class->reflFields[$fieldName]->getValue($entity);
7777

78-
if (null === $criteria[$fieldName]) {
78+
if ($constraint->ignoreNull && null === $criteria[$fieldName]) {
7979
return;
8080
}
8181

0 commit comments

Comments
 (0)
0