Merge pull request thecodingmachine#82 from homersimpsons/fix/express…

…ion-null-subtree Expression: Handle null `$subTree` Between: Refactor bypass logic GitHub Actions: Wait for MySQL to start
rodion-k · Sep 23, 2021 · 7efa677 · 7efa677
2 parents bae0f92 + 9d90054
commit 7efa677
Show file tree

Hide file tree

Showing 7 changed files with 61 additions and 52 deletions.
diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml
@@ -80,6 +80,12 @@ jobs:
       - name: "Install dependencies with composer"
         run: "composer update --no-interaction --no-progress --no-suggest --prefer-lowest"
 
+      - name: Wait for MySQL
+        run: |
+          while ! mysqladmin ping --host=127.0.0.1 --silent; do
+            sleep 1
+          done
+
       - name: "Run PHPUnit"
         run: "vendor/bin/phpunit --no-coverage"
 
@@ -124,6 +130,12 @@ jobs:
       - name: "Install dependencies with composer"
         run: "composer install --no-interaction --no-progress --no-suggest"
 
+      - name: Wait for MySQL
+        run: |
+          while ! mysqladmin ping --host=127.0.0.1 --silent; do
+            sleep 1
+          done
+
       - name: "Run PHPUnit"
         run: "vendor/bin/phpunit"
 
@@ -170,5 +182,11 @@ jobs:
       - name: "Install dependencies with composer"
         run: "composer install --no-interaction --no-progress --no-suggest"
 
+      - name: Wait for MySQL
+        run: |
+          while ! mysqladmin ping --host=127.0.0.1 --silent; do
+            sleep 1
+          done
+
       - name: "Run PHPUnit"
         run: "vendor/bin/phpunit --no-coverage"
diff --git a/composer.json b/composer.json
@@ -13,7 +13,7 @@
         }
     ],
     "require": {
-    	"php": ">=7.4.0 || ^8.0",
+    	"php": "^7.4 || ^8.0",
     	"mouf/utils.common.conditioninterface": "~2.0",
     	"mouf/utils.value.value-interface": "~1.0",
     	"mouf/utils.common.paginable-interface": "~1.0",

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -95,11 +95,6 @@ parameters:
 			count: 1
 			path: src/SQLParser/Node/NodeFactory.php
 
-		-
-			message: "#^Parameter \\#1 \\$var of function count expects array\\|Countable, array\\<SQLParser\\\\Node\\\\NodeInterface\\>\\|SQLParser\\\\Node\\\\NodeInterface given\\.$#"
-			count: 1
-			path: src/SQLParser/Node/NodeFactory.php
-
 		-
 			message: "#^Parameter \\#2 \\.\\.\\.\\$args of function array_merge expects array, array\\<SQLParser\\\\Node\\\\NodeInterface\\>\\|SQLParser\\\\Node\\\\NodeInterface given\\.$#"
 			count: 1

diff --git a/src/SQLParser/Node/Between.php b/src/SQLParser/Node/Between.php
@@ -148,7 +148,7 @@ public function toInstanceDescriptor(MoufManager $moufManager)
     }
 
     /**
-     * Renders the object as a SQL string.
+     * Renders the object as an SQL string.
      *
      * @param array $parameters
      * @param AbstractPlatform $platform
@@ -160,52 +160,45 @@ public function toInstanceDescriptor(MoufManager $moufManager)
      */
     public function toSql(array $parameters, AbstractPlatform $platform, int $indent = 0, $conditionsMode = self::CONDITION_APPLY, bool $extrapolateParameters = true): ?string
     {
-        $minBypass = false;
-        $maxBypass = false;
-
-        if ($conditionsMode == self::CONDITION_GUESS) {
-            if ($this->minValueOperand instanceof Parameter) {
-                if ($this->minValueOperand->isDiscardedOnNull() && !isset($parameters[$this->minValueOperand->getName()])) {
-                    $minBypass = true;
-                }
-            }
+        switch ($conditionsMode) {
+            case self::CONDITION_APPLY:
+                $minBypass = $this->minValueCondition && !$this->minValueCondition->isOk($parameters);
+                $maxBypass = $this->maxValueCondition && !$this->maxValueCondition->isOk($parameters);
+                break;
+            case self::CONDITION_GUESS:
+                $minBypass = $this->minValueOperand instanceof Parameter && $this->minValueOperand->isDiscardedOnNull() && !isset($parameters[$this->minValueOperand->getName()]);
+                $maxBypass = $this->maxValueOperand instanceof Parameter && $this->maxValueOperand->isDiscardedOnNull() && !isset($parameters[$this->maxValueOperand->getName()]);
+                break;
+            case self::CONDITION_IGNORE:
+                $minBypass = false;
+                $maxBypass = false;
+                break;
+            default:
+                throw new \InvalidArgumentException('Invalid `$conditionsMode`: "' . $conditionsMode. '"');
+        }
 
-            if ($this->maxValueOperand instanceof Parameter) {
-                if ($this->maxValueOperand->isDiscardedOnNull() && !isset($parameters[$this->maxValueOperand->getName()])) {
-                    $maxBypass = true;
-                }
-            }
-        } elseif ($conditionsMode == self::CONDITION_IGNORE) {
-            $minBypass = false;
-            $maxBypass = false;
-        } else {
-            if ($this->minValueCondition && !$this->minValueCondition->isOk($parameters)) {
-                $minBypass = true;
-            }
-            if ($this->maxValueCondition && !$this->maxValueCondition->isOk($parameters)) {
-                $maxBypass = true;
-            }
+        if ($maxBypass && $minBypass) {
+            return null;
+        }
+
+        if ($minBypass) {
+            return sprintf('%s <= %s',
+                NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters),
+                NodeFactory::toSql($this->maxValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters)
+            );
         }
 
-        if (!$minBypass && !$maxBypass) {
-            $sql = NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-            $sql .= ' BETWEEN ';
-            $sql .= NodeFactory::toSql($this->minValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-            $sql .= ' AND ';
-            $sql .= NodeFactory::toSql($this->maxValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-        } elseif (!$minBypass && $maxBypass) {
-            $sql = NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-            $sql .= ' >= ';
-            $sql .= NodeFactory::toSql($this->minValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-        } elseif ($minBypass && !$maxBypass) {
-            $sql = NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-            $sql .= ' <= ';
-            $sql .= NodeFactory::toSql($this->maxValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters);
-        } else {
-            $sql = null;
+        if ($maxBypass) {
+            return sprintf('%s >= %s',
+                NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters),
+                NodeFactory::toSql($this->minValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters));
         }
 
-        return $sql;
+        return sprintf('%s BETWEEN %s AND %s',
+            NodeFactory::toSql($this->leftOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters),
+            NodeFactory::toSql($this->minValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters),
+            NodeFactory::toSql($this->maxValueOperand, $platform, $parameters, ' ', false, $indent, $conditionsMode, $extrapolateParameters)
+        );
     }
 
     /**

diff --git a/src/SQLParser/Node/Expression.php b/src/SQLParser/Node/Expression.php
@@ -69,11 +69,11 @@ public function setBaseExpression($baseExpression): void
         $this->baseExpression = $baseExpression;
     }
 
-    /** @var NodeInterface[]|NodeInterface */
+    /** @var NodeInterface[]|NodeInterface|null */
     private $subTree;
 
     /**
-     * @return NodeInterface|NodeInterface[]
+     * @return NodeInterface|NodeInterface[]|null
      */
     public function getSubTree()
     {
@@ -263,7 +263,7 @@ public function walk(VisitorInterface $visitor)
                         $this->subTree[$key] = $result2;
                     }
                 }
-            } else {
+            } elseif ($this->subTree instanceof NodeInterface) {
                 $result2 = $this->subTree->walk($visitor);
                 if ($result2 === NodeTraverser::REMOVE_NODE) {
                     $this->subTree = [];

diff --git a/src/SQLParser/Node/NodeFactory.php b/src/SQLParser/Node/NodeFactory.php
@@ -652,7 +652,7 @@ public static function simplify($nodes)
                 if ($operand instanceof Expression) {
                     if (empty($operand->getBaseExpression())) {
                         $subTree = $operand->getSubTree();
-                        if (count($subTree) === 1) {
+                        if (is_array($subTree) && count($subTree) === 1) {
                             $newNodes = array_merge($newNodes, self::simplify($subTree));
                         } else {
                             $newNodes[] = $operand;

diff --git a/tests/Mouf/Database/MagicQueryTest.php b/tests/Mouf/Database/MagicQueryTest.php
@@ -208,6 +208,9 @@ public function testStandardSelect()
 
         $sql = 'SELECT COUNT(*) AS cnt FROM (SELECT id FROM country) subquery';
         $this->assertEquals($sql, self::simplifySql($magicQuery->build($sql)));
+
+        $sql = "SELECT YEAR(register_date) AS year FROM users GROUP BY year";
+        $this->assertEquals("SELECT YEAR(register_date) AS year FROM users GROUP BY year", self::simplifySql($magicQuery->build($sql)));
     }
 
     public function testInNullException() {