Re-implementing UnaryExpression to make it more simple

Allowing isNull and isNotNull to accept ExpressionInterface

src/Database/Dialect/SqlserverDialectTrait.php

@@ -99,7 +99,7 @@ trait SqlserverDialectTrait {
 		$query = clone $original;
 		$order = $query->clause('order') ?: new OrderByExpression('NULL');
 		$query->select([
-				'_cake_page_rownum_' => new UnaryExpression($order, [], 'ROW_NUMBER() OVER')
+				'_cake_page_rownum_' => new UnaryExpression('ROW_NUMBER() OVER', $order)
 			])->limit(null)
 			->offset(null)
 			->order([], true);

src/Database/Expression/QueryExpression.php

@@ -15,6 +15,7 @@
 namespace Cake\Database\Expression;
 
 use Cake\Database\ExpressionInterface;
+use Cake\Database\Expression\IdentifierExpression;
 use Cake\Database\TypeMapTrait;
 use Cake\Database\ValueBinder;
 use \Countable;
@@ -201,11 +202,14 @@ class QueryExpression implements ExpressionInterface, Countable {
 /**
  * Adds a new condition to the expression object in the form "field IS NULL".
  *
- * @param string $field database field to be tested for null
+ * @param string|ExpressionInterface $field database field to be tested for null
  * @return QueryExpression
  */
 	public function isNull($field) {
-		return $this->add($field . ' IS NULL');
+		if (!($field instanceof ExpressionInterface)) {
+			$field = new IdentifierExpression($field);
+		}
+		return $this->add(new UnaryExpression('IS NULL', $field, UnaryExpression::POSTFIX));
 	}
 
 /**
@@ -215,7 +219,10 @@ class QueryExpression implements ExpressionInterface, Countable {
  * @return QueryExpression
  */
 	public function isNotNull($field) {
-		return $this->add($field . ' IS NOT NULL');
+		if (!($field instanceof ExpressionInterface)) {
+			$field = new IdentifierExpression($field);
+		}
+		return $this->add(new UnaryExpression('IS NOT NULL', $field, UnaryExpression::POSTFIX));
 	}
 
 /**
@@ -437,7 +444,7 @@ class QueryExpression implements ExpressionInterface, Countable {
 			}
 
 			if (strtolower($k) === 'not') {
-				$this->_conditions[] = new UnaryExpression(new self($c, $typeMap), [], 'NOT');
+				$this->_conditions[] = new UnaryExpression('NOT', new self($c, $typeMap));
 				continue;
 			}
 

src/Database/Expression/UnaryExpression.php

@@ -22,7 +22,53 @@ use Cake\Database\ValueBinder;
  *
  * @internal
  */
-class UnaryExpression extends QueryExpression {
+class UnaryExpression implements ExpressionInterface {
+
+/**
+ * Indicates that the operation is in pre-order
+ *
+ */
+	const PREFIX = 0;
+
+/**
+ * Indicates that the operation is in post-order
+ *
+ */
+	const POSTFIX = 1;
+
+/**
+ * The operator this unary expression represents
+ *
+ * @var string
+ */
+	protected $_operator;
+
+/**
+ * Holds the value which the unary expression operates
+ *
+ * @var mixed
+ */
+	protected $_value;
+
+/**
+ * Where to place the operator
+ *
+ * @var int
+ */
+	protected $_mode;
+
+/**
+ * Constructor
+ *
+ * @param string $operator The operator to used for the expression
+ * @param mixed $value the value to use as the operand for the expression
+ * @param int $mode either UnaryExpression::PREFIX or  UnaryExpression::POSTFIX
+ */
+	public function __construct($operator, $value, $mode = self::PREFIX) {
+		$this->_operator = $operator;
+		$this->_value = $value;
+		$this->_mode = $mode;
+	}
 
 /**
  * Converts the expression to its string representation
@@ -31,12 +77,25 @@ class UnaryExpression extends QueryExpression {
  * @return string
  */
 	public function sql(ValueBinder $generator) {
-		foreach ($this->_conditions as $condition) {
-			if ($condition instanceof ExpressionInterface) {
-				$condition = $condition->sql($generator);
-			}
-			// We only use the first (and only) condition
-			return $this->_conjunction . ' (' . $condition . ')';
+		$operand = $this->_value;
+		if ($operand instanceof ExpressionInterface) {
+			$operand = $operand->sql($generator);
+		}
+
+		if ($this->_mode === self::POSTFIX) {
+			return '(' . $operand . ') ' . $this->_operator;
+		}
+
+		return $this->_operator . ' (' . $operand . ')';
+	}
+
+/**
+ * {@inheritDoc}
+ *
+ */
+	public function traverse(callable $callable) {
+		if ($this->_value instanceof ExpressionInterface) {
+			$callable($this->_value);
 		}
 	}
 

tests/TestCase/Database/QueryTest.php

@@ -2485,6 +2485,37 @@ class QueryTest extends TestCase {
 	}
 
 /**
+ * Tests that it is possible to pass ExpressionInterface to isNull and isNotNull
+ *
+ * @return void
+ */
+	public function testIsNullWithExpressions() {
+		$query = new Query($this->connection);
+		$subquery = (new Query($this->connection))
+			->select(['id'])
+			->from('authors')
+			->where(['id' => 1]);
+
+		$result = $query
+			->select(['name'])
+			->from(['authors'])
+			->where(function($exp) use ($subquery) {
+				return $exp->isNotNull($subquery);
+			})
+		->execute();
+		$this->assertNotEmpty($result->fetchAll('assoc'));
+
+		$result = (new Query($this->connection))
+			->select(['name'])
+			->from(['authors'])
+			->where(function($exp) use ($subquery) {
+				return $exp->isNull($subquery);
+			})
+			->execute();
+		$this->assertEmpty($result->fetchAll('assoc'));
+	}
+
+/**
  * Assertion for comparing a table's contents with what is in it.
  *
  * @param string $table