CheckNull option for IsUnique #8621

JayPHP · 2016-04-09T21:07:35Z

This is a suggestion for issue #8620

PHPUnit tests;

Time: 1.54 minutes, Memory: 164.00Mb

OK, but incomplete, skipped, or risky tests!
Tests: 6883, Assertions: 21233, Skipped: 144.

thinkingmedia · 2016-04-09T22:27:51Z

No, this is bad. null should not be treated as a key value for an index. This leads to other issues such as using null as a primary key, or what position null has in an ordered list.

I think at the data storage level this is a problem. MySQL for example does not support null for unique indexes. It just ignores it. If a column allows null and has a unique index it can contain many nulls. You don't gain any index performance by using null either.

What ever problem it was that you were originally trying to solve. There was probably was a better way to solve it than testing for unique null.

JayPHP · 2016-04-09T23:58:00Z

@thinkingmedia I'm struggling to understand your argument.

This leads to other issues such as using null as a primary key

AFAIK the SQL standard says that you cannot have null as a primary key?

or what position null has in an ordered list.

What does this have to do with this option? How can allowing an option for null values to be checked with IsUnique affect the positioning of null in an ordered list? Surely if the data is related to an ordered list, the user would prevent null values anyway? But still, I don't see how this is relevant?

MySQL for example does not support null for unique indexes.

True but again, I don't see how this is relevant? This is adding an option to the validation stage before data insertion, so why can we not offer an option that MySQL does not? There are multiple threads calling for MySQL to support this, so it's clearly a wanted feature but still, I don't see how it should affect what Cake offers? If Cake only offered validation options that were available in MySQL, then surely there would be no need for the Cake Validation stage in the first place?

What ever problem it was that you were originally trying to solve. There was probably was a better way to solve it than testing for unique null.

I'm all up for suggestions. AFAIK, this is the best option for someone who wants IsUnique to support null values, but if there's a better option, I'm all for it.

thinkingmedia · 2016-04-10T00:16:07Z

@JayPHP first, I'm apologizing if you found my comment was to critical of your work. I understand that these pull requests take a lot of time to make, and I didn't mean to sound like I was slamming the door on the PR. I don't have authority here to reject PRs and was just stating an opinion, and maybe not a very good one at that.

I'm of the opinion that null is an anti-value. It can represent a state but not a value. You can say that a record belongsTo ABC or when null it belongsTo nothing. From a data stand point that makes sense, but it's difficult to say that a record can belongsTo nothing only once.

We can count things in the dataset with IS NULL or IS NOT NULL. So for validation I think that will work, but I think its an indication of a problem in the database schema rather than a limitation on the validation rules.

So I worry that this encourages the usage of null as a value where underlying data storage will not.

Again, this is just my opinion. It does change often ;)

JayPHP · 2016-04-10T00:40:10Z

@thinkingmedia This is my first contribution so I appreciate any feedback I can get, really.
I was just trying to understand your comment, that's all :P

I'm of the opinion that null is an anti-value. It can represent a state but not a value.

Now I completely understand what you mean;

It looks like PHP is more on the value side? http://php.net/manual/en/language.types.null.php however, SQL (like you say)...

SQL null is a state (unknown) and not a value. This usage is quite different from most programming languages, where null means not assigned to a particular instance.

So I guess I'm torn but leaning towards your opinion more as this method is fairly db based

ndm2 · 2016-04-10T01:03:51Z

As the one suggesting that possible "solution" for the given "problem", without putting the root of the problem to question, and possibly offering solutions that don't require such controversial constraints, let me add my tree fiddy...

AFAIR, MySQLs BDB engine supported NULL as a unique value, however that engine is gone since 5.1, so it's not relevant for CakePHP anymore. SQL Server on the other hand supports it (it always did if I'm not mistaken, and you have to use filters to change this behavior), which some people like, and some don't, as it contradicts the ANSI standard.

With respect to that historical behavior, being able to represent this in the form of an application rule might come handy for some SQL Server peeps, whether this justifies core feature relevance however is of debatable nature. Rethinking it, I think I'd now possibly tend more towards, nah, might not be an overly good idea, enforcing standards on that level of the abstraction layer might be better. Making the rule more extensible may be a compromise, so that people could easily modify the conditions without having to reimplement the whole rules logic.

markstory · 2016-04-10T01:10:25Z

src/ORM/Rule/IsUnique.php

@@ -63,6 +64,15 @@ public function __invoke(EntityInterface $entity, array $options)
            }
        }

+        if ($checkNull) {
+            foreach ($conditions as $key => $value) {


Couldn't this be done in the _alias() method that is called on line 57?

@markstory So add another parameter to the alias method? (Currently the only parameter is $alias)

JayPHP · 2016-04-10T18:21:52Z

Agreed with @lorenzo that this isn't required. A simple note in the docs to say that IsUnique will not include null values in its checks should suffice.

chinpei215 · 2016-04-10T18:43:52Z

@JayPHP Why did you close this PR? I don't think this PR doesn't make sense though. @lorenzo also seems not to say such things.

JayPHP · 2016-04-10T19:49:00Z

@chinpei215 I spoke to @lorenzo in slack. Sorry it doesn't make sense, i'll try and explain a bit better below;

So, while using IsUnique in order to prevent duplicate entries in my db, I came across the below issue;
My rule;
$rules->add($rules->IsUnique(['date', 'time']));

Now, lets say I have the below entry in my database;

Date = 22/07/2015
time = null

If I try to save the above entry again, the IsUnique rule will not stop me, because null is not seen as a value. (Better explanation here).

So the checkNull option that I suggested, is an option that when true, will consider null values and thus would prevent a save such as the above.

However from speaking to Lorenzo in slack + the above discussions, I don't think there's much support for it, that's why I closed :)

chinpei215 · 2016-04-10T22:34:29Z

@JayPHP Thank you for explaining your issue. But I knew that when this PR opened. My English might be bad. I wanted to say "I think this PR is not bad". Personally, I wouldn't use this feature. But I think that some people would need this feature, as I have seen some applications using a datetime field for soft delete.

And thank you for letting me know about the discussion on slack. Although I am not good at English, but if you closed this PR for the reason of naming, what about permitMultipleNulls? The MySQL Reference Manual seems to explain the behavior by using such words.

A UNIQUE index creates a constraint such that all values in the index must be distinct. An error occurs if you try to add a new row with a key value that matches an existing row. For all engines, a UNIQUE index permits multiple NULL values for columns that can contain NULL.

JayPHP · 2016-04-11T12:13:02Z

@chinpei215 Ah apologies, I misunderstood your comment. Alright I'll re-open and improve the request via @markstory comments.

ADmad · 2016-04-11T18:47:21Z

src/ORM/Rule/IsUnique.php

@@ -53,6 +53,11 @@ public function __invoke(EntityInterface $entity, array $options)
            return true;
        }

+        $permitMultipleNulls = true;
+        if (isset($options['permitMultipleNulls'])) {


Naming the option allowMultipleNulls would be better for consistency since "allowFoo" is used at other places in the framework.

@ADmad Sounds good and reasonable to me.
@JayPHP I'm sorry to trouble you. 🙇

JayPHP · 2016-04-12T07:43:33Z

@ADmad @chinpei215 No problem :) Updated.

dereuromark · 2016-04-13T08:56:14Z

src/ORM/Rule/IsUnique.php

@@ -53,6 +53,11 @@ public function __invoke(EntityInterface $entity, array $options)
            return true;
        }

+        $allowMultipleNulls = true;
+        if (isset($options['allowMultipleNulls'])) {


I think those strings should not be magic strings but class constants.
This way they can be used from outside without the risk of typos (IsUnique::ALLOW_MULTIPLE_NULLS etc).
We should be doing this more often in general with those option keys available per class IMO.
Would do others think?

@dereuromark I guess it's bad practise to presume that the user won't accidentally enable this, so +1 from me. If others agree, I don't mind trying to change it to a constant.

@dereuromark Theoretically, I think you are right. But as you know, we are using string literals in many places. For example:

$validator ->add('title', 'notEmpty', [ 'rule' => 'notEmpty', 'message' => __('You need to provide a title'), ]);

@chinpei215 It is still never too late to start somewhere :)

@dereuromark Sure. But it would be better to discuss it apart from this PR. The above line also can be changed after discussion :)

I am fine with that :)

lorenzo · 2016-04-13T20:53:00Z

Thanks!

saeideng · 2016-05-08T09:38:39Z

cake 3.2.X need to these changes :)
why only 3.next?

markstory · 2016-05-08T12:19:51Z

@saeideng Its a new feature, we often put new features into 3.next.

saeideng · 2016-05-08T12:37:57Z

thanks mark.
but i think that is a bug instead of a new features
like this

SELECT 
  1 AS `existing` 
FROM 
  categories Categories 
WHERE 
  (
    Categories.name = 'sample' 
    AND Categories.parent_id = NULL //this line wronge
    AND NOT (Categories.id = 23)
  ) 
LIMIT 
  1

saeideng · 2016-05-08T12:46:01Z

@markstory i dont talk about allowMultipleNulls option

lorenzo · 2016-05-08T17:32:08Z

Can you open a ticket for what you have found to be a bug? please make sure to explain how to reproduce it

markstory · 2016-05-08T18:36:00Z

@saeideng Well both the original issue and pull request positioned these changes as a new features.

JayPHP and others added 6 commits April 8, 2016 23:17

Added option to check for NULL values on IsUnique

9989334

added tests

466ecf9

added tests + corrections

3a6372e

Removed note

621c410

Removed whitespace

b8ab699

Added whitespace

36c7d1c

JayPHP mentioned this pull request Apr 9, 2016

Option to check NULL values for IsUnique #8620

Closed

1 task

Corrected uppercase and formatting issues

0aced1a

markstory added this to the 3.3.0 milestone Apr 10, 2016

markstory added the enhancement label Apr 10, 2016

markstory reviewed Apr 10, 2016
View reviewed changes

JayPHP closed this Apr 10, 2016

JayPHP reopened this Apr 11, 2016

james.byrne added 3 commits April 11, 2016 15:24

Changed option name

7a59f20

Using pre-existing fixture + CS Fix

c453418

Added back tags fixture

6fb1641

ADmad reviewed Apr 11, 2016
View reviewed changes

markstory added the validation label Apr 11, 2016

Changed name to allow instead of permit

7974693

dereuromark reviewed Apr 13, 2016
View reviewed changes

lorenzo merged commit 88138af into cakephp:3.next Apr 13, 2016

ionas mentioned this pull request May 8, 2016

3.x Bug/Enhance: ExistsIn Application Rule and NULL values #8749

Closed

2 tasks

saeideng mentioned this pull request May 9, 2016

"= null" in query #8793

Closed

1 task

markstory mentioned this pull request May 27, 2016

ExistsIn() should allow composite foreign keys having NULL allowed in the schema #8897

Closed

ADmad mentioned this pull request Aug 8, 2019

4.x: IS NULL exception fix #13214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CheckNull option for IsUnique #8621

CheckNull option for IsUnique #8621

JayPHP commented Apr 9, 2016

thinkingmedia commented Apr 9, 2016

JayPHP commented Apr 9, 2016 •

edited

thinkingmedia commented Apr 10, 2016

JayPHP commented Apr 10, 2016 •

edited

ndm2 commented Apr 10, 2016

markstory Apr 10, 2016

JayPHP Apr 10, 2016

JayPHP commented Apr 10, 2016

chinpei215 commented Apr 10, 2016

JayPHP commented Apr 10, 2016

chinpei215 commented Apr 10, 2016

JayPHP commented Apr 11, 2016

ADmad Apr 11, 2016

chinpei215 Apr 12, 2016

JayPHP commented Apr 12, 2016

dereuromark Apr 13, 2016

JayPHP Apr 13, 2016

chinpei215 Apr 13, 2016

dereuromark Apr 13, 2016

chinpei215 Apr 13, 2016

dereuromark Apr 13, 2016

lorenzo commented Apr 13, 2016

saeideng commented May 8, 2016

markstory commented May 8, 2016

saeideng commented May 8, 2016 •

edited

saeideng commented May 8, 2016

lorenzo commented May 8, 2016

markstory commented May 8, 2016

CheckNull option for IsUnique #8621

CheckNull option for IsUnique #8621

Conversation

JayPHP commented Apr 9, 2016

thinkingmedia commented Apr 9, 2016

JayPHP commented Apr 9, 2016 • edited

thinkingmedia commented Apr 10, 2016

JayPHP commented Apr 10, 2016 • edited

ndm2 commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayPHP commented Apr 10, 2016

chinpei215 commented Apr 10, 2016

JayPHP commented Apr 10, 2016

chinpei215 commented Apr 10, 2016

JayPHP commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayPHP commented Apr 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzo commented Apr 13, 2016

saeideng commented May 8, 2016

markstory commented May 8, 2016

saeideng commented May 8, 2016 • edited

saeideng commented May 8, 2016

lorenzo commented May 8, 2016

markstory commented May 8, 2016

JayPHP commented Apr 9, 2016 •

edited

JayPHP commented Apr 10, 2016 •

edited

saeideng commented May 8, 2016 •

edited