[Doctrine Bridge] Added parameter ignoreNull to accept a nullable value on field #5015
Conversation
|
This is wrong. RDBMS allow several null values in a unique column and this change will break it. |
|
@stof seems weird indeed it would return if any of the values are null. Makes sense to do a query where the field |
|
@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. |
|
In this case i suspect that he wants to achieve a |
|
I suggest to make this configurable as the handling of NULL values in UNIQUE columns differs between SQL implementations. |
|
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 |
|
yep, this would be good (except for the naming which seems weird) |
|
No problem to change this. I don't find a good name for this ? |
|
what about |
|
Why multiple ? This option is for one or many. what about |
|
@Garfield-fr the current behavior allows having multiple null values without failing to the unique constraint |
|
ok. I make It's ok with that: https://gist.github.com/cae8d43780c45a5011ed Thanks |
|
What about |
|
no it is not. You have an issue in the validator. You have an extra negation. Please update your PR |
|
@stof |
|
yes |
| @@ -75,7 +75,7 @@ public function validate($entity, Constraint $constraint) | |||
|
|
|||
| $criteria[$fieldName] = $class->reflFields[$fieldName]->getValue($entity); | |||
|
|
|||
| if (null === $criteria[$fieldName]) { | |||
| if ($ignoreNull && null === $criteria[$fieldName]) { | |||
There was a problem hiding this comment.
This cannot work. You need to access $constraint->ignoreNull instead.
Please add a test that proves the implemented functionality.
|
Is it mergeable now? Is yes, @Garfield-fr Can you squash your commits? |
…low a nullable value on field. Added Test
|
@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 |
|
What's the right solution for resolve this ? |
|
@Garfield-fr reset your local branch to the squashed commit and force the push |
|
@stof Thanks for your help |
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).
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.
It's possible to add this PR on Bridge.
Thanks
Bertrand