Skip to content

Commit

Permalink
Fix failing tests in IsUnique rule.
Browse files Browse the repository at this point in the history
A while back the default behavior was changed in 3.2.x to make IsUnique
strict about nulls. When that code was merged into `3.next` I deleted
the 'duplicate' code. Then that change was reverted in `master` leaving
these tests failing. This restores the behavior and fixes the failing
tests.
  • Loading branch information
markstory committed May 27, 2016
1 parent a71cc74 commit 86e24ef
Showing 1 changed file with 11 additions and 9 deletions.
20 changes: 11 additions & 9 deletions src/ORM/Rule/IsUnique.php
Expand Up @@ -52,17 +52,14 @@ public function __invoke(EntityInterface $entity, array $options)
if (!$entity->extract($this->_fields, true)) {
return true;
}

$allowMultipleNulls = true;
if (isset($options['allowMultipleNulls'])) {
$allowMultipleNulls = $options['allowMultipleNulls'] === true ? true : false;
}
$options += ['allowMultipleNulls' => true];
$allowMultipleNulls = $options['allowMultipleNulls'];

$alias = $options['repository']->alias();
$conditions = $this->_alias($alias, $entity->extract($this->_fields));
$conditions = $this->_alias($alias, $entity->extract($this->_fields), $allowMultipleNulls);
if ($entity->isNew() === false) {
$keys = (array)$options['repository']->primaryKey();
$keys = $this->_alias($alias, $entity->extract($keys));
$keys = $this->_alias($alias, $entity->extract($keys), $allowMultipleNulls);
if (array_filter($keys, 'strlen')) {
$conditions['NOT'] = $keys;
}
Expand All @@ -79,13 +76,18 @@ public function __invoke(EntityInterface $entity, array $options)
*
* @param string $alias The alias to add.
* @param array $conditions The conditions to alias.
* @param bool $multipleNulls Whether or not to allow multiple nulls.
* @return array
*/
protected function _alias($alias, $conditions)
protected function _alias($alias, $conditions, $multipleNulls)
{
$aliased = [];
foreach ($conditions as $key => $value) {
$aliased["$alias.$key"] = $value;
if ($multipleNulls) {
$aliased["$alias.$key"] = $value;
} else {
$aliased["$alias.$key IS"] = $value;
}
}
return $aliased;
}
Expand Down

5 comments on commit 86e24ef

@ionas
Copy link
Contributor

@ionas ionas commented on 86e24ef May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
So by default it will work like before (for BC sake) but you can set allowMultipleNulls to false?

@ionas
Copy link
Contributor

@ionas ionas commented on 86e24ef May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd do it the other way around and in 4.x change the default behavior possibly:
e.g.
default in 3.x: treatNullsAsUnique => false
default in 4.x (or 5.x): treatNullsAsUnique => true

Just a different perspective.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's an option. I think sticking with SQL equivalence as the default is probably going to surprise fewer people though.

@ionas
Copy link
Contributor

@ionas ionas commented on 86e24ef May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, however that's not BC, is it?

What I meant is mostly changing the names because the current option flag doesn't really "tell" me what to expect, e.g. treatNullsAsUnique or similar does what you expect... allowMultipleNulls I don't really understand by reading just it.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the code enforces SQL UNIQUE behavior (which allows multi-column indices to have duplicates if one column contains a null). The new option allows you to enforce no duplicates including nulls.

Please sign in to comment.