Merge pull request #321 from garak/improve-code

improve code
KnpLabs · May 9, 2023 · 1f6560c · 1f6560c
2 parents 3a72377 + 98aa98b
commit 1f6560c
Show file tree

Hide file tree

Showing 25 changed files with 80 additions and 122 deletions.
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -1,6 +1,14 @@
 name: build
 
-on: [push, pull_request]
+on: 
+    push:
+        branches:
+            - '**'
+        tags:
+            - '!**'
+    pull_request:
+        branches:
+            - '**'
 
 jobs:
     phpstan:

diff --git a/composer.json b/composer.json
@@ -51,7 +51,7 @@
         "symfony/property-access": "to allow sorting arrays"
     },
     "conflict": {
-        "doctrine/dbal": "<2.10"
+        "doctrine/dbal": "<3.1"
     },
     "extra": {
         "branch-alias": {

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalker.php b/src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalker.php
@@ -49,7 +49,7 @@ public function walkSelectStatement(SelectStatement $AST): void
         $queriedValue = $query->getHint(self::HINT_PAGINATOR_FILTER_VALUE);
         $columns = $query->getHint(self::HINT_PAGINATOR_FILTER_COLUMNS);
         $filterCaseInsensitive = $query->getHint(self::HINT_PAGINATOR_FILTER_CASE_INSENSITIVE);
-        $components = $this->_getQueryComponents();
+        $components = $this->getQueryComponents();
         $filterExpressions = [];
         $expressions = [];
         foreach ($columns as $column) {
@@ -59,17 +59,17 @@ public function walkSelectStatement(SelectStatement $AST): void
             if (2 <= count($parts)) {
                 $alias = trim(reset($parts));
                 if (!array_key_exists($alias, $components)) {
-                    throw new InvalidValueException("There is no component aliased by [{$alias}] in the given Query");
+                    throw new InvalidValueException("There is no component aliased by [$alias] in the given Query");
                 }
                 $meta = $components[$alias];
                 if (!$meta['metadata']->hasField($field)) {
-                    throw new InvalidValueException("There is no such field [{$field}] in the given Query component, aliased by [$alias]");
+                    throw new InvalidValueException("There is no such field [$field] in the given Query component, aliased by [$alias]");
                 }
                 $pathExpression = new PathExpression(PathExpression::TYPE_STATE_FIELD, $alias, $field);
                 $pathExpression->type = PathExpression::TYPE_STATE_FIELD;
             } else {
                 if (!array_key_exists($field, $components)) {
-                    throw new InvalidValueException("There is no component field [{$field}] in the given Query");
+                    throw new InvalidValueException("There is no component field [$field] in the given Query");
                 }
                 $pathExpression = $components[$field]['resultVariable'];
             }
@@ -160,12 +160,13 @@ public function walkSelectStatement(SelectStatement $AST): void
      * @param  Node[]                $filterExpressions
      * @return bool
      */
-    private function expressionContainsFilter(ConditionalExpression $node, $filterExpressions): bool
+    private function expressionContainsFilter(ConditionalExpression $node, array $filterExpressions): bool
     {
         foreach ($node->conditionalTerms as $conditionalTerm) {
             if ($conditionalTerm instanceof ConditionalTerm && $this->termContainsFilter($conditionalTerm, $filterExpressions)) {
                 return true;
-            } elseif ($conditionalTerm instanceof ConditionalPrimary && $this->primaryContainsFilter($conditionalTerm, $filterExpressions)) {
+            }
+            if ($conditionalTerm instanceof ConditionalPrimary && $this->primaryContainsFilter($conditionalTerm, $filterExpressions)) {
                 return true;
             }
         }
@@ -178,7 +179,7 @@ private function expressionContainsFilter(ConditionalExpression $node, $filterEx
      * @param  Node[]          $filterExpressions
      * @return bool
      */
-    private function termContainsFilter(ConditionalTerm $node, $filterExpressions): bool
+    private function termContainsFilter(ConditionalTerm $node, array $filterExpressions): bool
     {
         foreach ($node->conditionalFactors as $conditionalFactor) {
             if ($conditionalFactor instanceof ConditionalFactor) {
@@ -200,7 +201,7 @@ private function termContainsFilter(ConditionalTerm $node, $filterExpressions):
      * @param  Node[]            $filterExpressions
      * @return bool
      */
-    private function factorContainsFilter(ConditionalFactor $node, $filterExpressions): bool
+    private function factorContainsFilter(ConditionalFactor $node, array $filterExpressions): bool
     {
         if ($node->conditionalPrimary instanceof ConditionalPrimary && $node->not === false) {
             return $this->primaryContainsFilter($node->conditionalPrimary, $filterExpressions);
@@ -214,9 +215,9 @@ private function factorContainsFilter(ConditionalFactor $node, $filterExpression
      * @param  Node[]             $filterExpressions
      * @return bool
      */
-    private function primaryContainsFilter(ConditionalPrimary $node, $filterExpressions): bool
+    private function primaryContainsFilter(ConditionalPrimary $node, array $filterExpressions): bool
     {
-        if ($node->isSimpleConditionalExpression() && ($node->simpleConditionalExpression instanceof LikeExpression || $node->simpleConditionalExpression instanceof ComparisonExpression)) {
+        if (($node->simpleConditionalExpression instanceof LikeExpression || $node->simpleConditionalExpression instanceof ComparisonExpression) && $node->isSimpleConditionalExpression()) {
             return $this->isExpressionInFilterExpressions($node->simpleConditionalExpression, $filterExpressions);
         }
         if ($node->isConditionalExpression()) {
@@ -231,7 +232,7 @@ private function primaryContainsFilter(ConditionalPrimary $node, $filterExpressi
      * @param  Node[] $filterExpressions
      * @return bool
      */
-    private function isExpressionInFilterExpressions(Node $node, $filterExpressions): bool
+    private function isExpressionInFilterExpressions(Node $node, array $filterExpressions): bool
     {
         foreach ($filterExpressions as $filterExpression) {
             if ((string) $filterExpression === (string) $node) {
@@ -242,11 +243,7 @@ private function isExpressionInFilterExpressions(Node $node, $filterExpressions)
         return false;
     }
 
-    /**
-     * @param  ConditionalPrimary|ConditionalExpression $node
-     * @return ConditionalPrimary
-     */
-    private function createPrimaryFromNode($node): ConditionalPrimary
+    private function createPrimaryFromNode(ConditionalPrimary|ConditionalExpression $node): ConditionalPrimary
     {
         if ($node instanceof ConditionalPrimary) {
             $conditionalPrimary = $node;

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/QuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/QuerySubscriber.php
@@ -47,14 +47,14 @@ public function items(ItemsEvent $event): void
         if (str_contains($value, '*')) {
             $value = str_replace('*', '%', $value);
         }
-        if (is_string($columns) && false !== strpos($columns, ',')) {
+        if (is_string($columns) && str_contains($columns, ',')) {
             $columns = explode(',', $columns);
         }
         $columns = (array) $columns;
         if (isset($event->options[PaginatorInterface::FILTER_FIELD_ALLOW_LIST])) {
             foreach ($columns as $column) {
                 if (!in_array($column, $event->options[PaginatorInterface::FILTER_FIELD_ALLOW_LIST])) {
-                    throw new InvalidValueException("Cannot filter by: [{$column}] this field is not in allow list");
+                    throw new InvalidValueException("Cannot filter by: [$column] this field is not in allow list");
                 }
             }
         }

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Filtration/PropelQuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Filtration/PropelQuerySubscriber.php
@@ -31,25 +31,25 @@ public function items(ItemsEvent $event): void
             } else {
                 return;
             }
-            if (is_string($columns) && false !== strpos($columns, ',')) {
+            if (is_string($columns) && str_contains($columns, ',')) {
                 $columns = explode(',', $columns);
             }
             $columns = (array) $columns;
             if (isset($event->options[PaginatorInterface::FILTER_FIELD_ALLOW_LIST])) {
                 foreach ($columns as $column) {
                     if (!in_array($column, $event->options[PaginatorInterface::FILTER_FIELD_ALLOW_LIST])) {
-                        throw new InvalidValueException("Cannot filter by: [{$column}] this field is not in allow list");
+                        throw new InvalidValueException("Cannot filter by: [$column] this field is not in allow list");
                     }
                 }
             }
             $value = $this->argumentAccess->get($event->options[PaginatorInterface::FILTER_VALUE_PARAMETER_NAME]);
             $criteria = \Criteria::EQUAL;
-            if (false !== strpos($value, '*')) {
+            if (str_contains($value, '*')) {
                 $value = str_replace('*', '%', $value);
                 $criteria = \Criteria::LIKE;
             }
             foreach ($columns as $column) {
-                if (false !== strpos($column, '.')) {
+                if (str_contains($column, '.')) {
                     $query->where($column.$criteria.'?', $value);
                 } else {
                     $query->{'filterBy'.$column}($value, $criteria);

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Paginate/ArraySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Paginate/ArraySubscriber.php
@@ -32,7 +32,7 @@ public function items(ItemsEvent $event): void
     public static function getSubscribedEvents(): array
     {
         return [
-            'knp_pager.items' => ['items', -1/* other data arrays should be analized first*/],
+            'knp_pager.items' => ['items', -1/* other data arrays should be analyzed first*/],
         ];
     }
 }
diff --git a/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/DBALQueryBuilderSubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/DBALQueryBuilderSubscriber.php
@@ -35,7 +35,7 @@ public function items(ItemsEvent $event): void
                 ->from('(' . $sql . ')', 'dbal_count_tbl')
             ;
 
-            $compat = $qb->execute();
+            $compat = $qb->executeQuery();
             $event->count = method_exists($compat, 'fetchColumn') ? $compat->fetchColumn(0) : $compat->fetchOne();
 
             // if there is results
@@ -48,8 +48,8 @@ public function items(ItemsEvent $event): void
                 ;
 
                 $event->items = $qb
-                    ->execute()
-                    ->fetchAll()
+                    ->executeQuery()
+                    ->fetchAllAssociative()
                 ;
             }
 

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/ODM/MongoDB/QuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/ODM/MongoDB/QuerySubscriber.php
@@ -18,7 +18,7 @@ public function items(ItemsEvent $event): void
             }
             static $reflectionProperty;
             if (is_null($reflectionProperty)) {
-                $reflectionClass = new \ReflectionClass('Doctrine\ODM\MongoDB\Query\Query');
+                $reflectionClass = new \ReflectionClass(Query::class);
                 $reflectionProperty = $reflectionClass->getProperty('query');
                 $reflectionProperty->setAccessible(true);
             }

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/ORM/QuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Paginate/Doctrine/ORM/QuerySubscriber.php
@@ -20,10 +20,7 @@ public function items(ItemsEvent $event): void
         }
         $event->stopPropagation();
 
-        $useOutputWalkers = false;
-        if (isset($event->options['wrap-queries'])) {
-            $useOutputWalkers = $event->options['wrap-queries'];
-        }
+        $useOutputWalkers = $event->options['wrap-queries'] ?? false;
 
         $event->target
             ->setFirstResult($event->getOffset())

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Paginate/PropelQuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Paginate/PropelQuerySubscriber.php
@@ -22,7 +22,7 @@ public function items(ItemsEvent $event): void
             if ($event->options[PaginatorInterface::DISTINCT]) {
                 $countQuery->distinct();
             }
-            $event->count = intval($countQuery->count());
+            $event->count = (int) $countQuery->count();
             // process items
             $result = null;
             if ($event->count) {

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ODM/MongoDB/QuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ODM/MongoDB/QuerySubscriber.php
@@ -34,10 +34,8 @@ public function items(ItemsEvent $event): void
                 $field = $this->argumentAccess->get($sortField);
                 $dir = null !== $sortDir && strtolower($this->argumentAccess->get($sortDir)) === 'asc' ? 1 : -1;
 
-                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                    if (!in_array($field, $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                        throw new InvalidValueException("Cannot sort by: [{$field}] this field is not in allow list.");
-                    }
+                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST]) && (!in_array($field, $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST]))) {
+                    throw new InvalidValueException("Cannot sort by: [$field] this field is not in allow list.");
                 }
                 static $reflectionProperty;
                 if (is_null($reflectionProperty)) {

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ORM/Query/OrderByWalker.php b/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ORM/Query/OrderByWalker.php
@@ -40,7 +40,7 @@ public function walkSelectStatement(SelectStatement $AST): string
         $fields = (array)$query->getHint(self::HINT_PAGINATOR_SORT_FIELD);
         $aliases = (array)$query->getHint(self::HINT_PAGINATOR_SORT_ALIAS);
 
-        $components = $this->_getQueryComponents();
+        $components = $this->getQueryComponents();
         foreach ($fields as $index => $field) {
             if (!$field) {
                 continue;
@@ -49,16 +49,14 @@ public function walkSelectStatement(SelectStatement $AST): string
             $alias = $aliases[$index];
             if ($alias !== false) {
                 if (!array_key_exists($alias, $components)) {
-                    throw new InvalidValueException("There is no component aliased by [{$alias}] in the given Query");
+                    throw new InvalidValueException("There is no component aliased by [$alias] in the given Query");
                 }
                 $meta = $components[$alias];
                 if (!$meta['metadata']->hasField($field)) {
-                    throw new InvalidValueException("There is no such field [{$field}] in the given Query component, aliased by [$alias]");
-                }
-            } else {
-                if (!array_key_exists($field, $components)) {
-                    throw new InvalidValueException("There is no component field [{$field}] in the given Query");
+                    throw new InvalidValueException("There is no such field [$field] in the given Query component, aliased by [$alias]");
                 }
+            } elseif (!array_key_exists($field, $components)) {
+                throw new InvalidValueException("There is no component field [$field] in the given Query");
             }
 
             $direction = $query->getHint(self::HINT_PAGINATOR_SORT_DIRECTION);
@@ -75,12 +73,14 @@ public function walkSelectStatement(SelectStatement $AST): string
             if ($AST->orderByClause) {
                 $set = false;
                 foreach ($AST->orderByClause->orderByItems as $item) {
-                    if ($item->expression instanceof PathExpression) {
-                        if ($item->expression->identificationVariable === $alias && $item->expression->field === $field) {
-                            $item->type = $direction;
-                            $set = true;
-                            break;
-                        }
+                    if (
+                        $item->expression instanceof PathExpression &&
+                        $item->expression->identificationVariable === $alias &&
+                        $item->expression->field === $field
+                    ) {
+                        $item->type = $direction;
+                        $set = true;
+                        break;
                     }
                 }
                 if (!$set) {

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ORM/QuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ORM/QuerySubscriber.php
@@ -35,10 +35,8 @@ public function items(ItemsEvent $event): void
             if (null !== $sortField && $this->argumentAccess->has($sortField)) {
                 $dir = null !== $sortDir && $this->argumentAccess->has($sortDir) && strtolower($this->argumentAccess->get($sortDir)) === 'asc' ? 'asc' : 'desc';
 
-                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                    if (!in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                        throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
-                    }
+                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST]) && !in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
+                    throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
                 }
 
                 $sortFieldParameterNames = $this->argumentAccess->get($sortField);
@@ -51,7 +49,7 @@ public function items(ItemsEvent $event): void
                 foreach (explode('+', $sortFieldParameterNames) as $sortFieldParameterName) {
                     $parts = explode('.', $sortFieldParameterName, 2);
 
-                    // We have to prepend the field. Otherwise OrderByWalker will add
+                    // We have to prepend the field. Otherwise, OrderByWalker will add
                     // the order-by items in the wrong order
                     array_unshift($fields, end($parts));
                     array_unshift($aliases, 2 <= count($parts) ? reset($parts) : false);

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Sortable/PropelQuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Sortable/PropelQuerySubscriber.php
@@ -35,10 +35,8 @@ public function items(ItemsEvent $event): void
                 $direction = (null !== $sortDir && $this->argumentAccess->has($sortDir) && strtolower($this->argumentAccess->get($sortDir)) === 'asc')
                                 ? 'asc' : 'desc';
 
-                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                    if (!in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                        throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
-                    }
+                if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST]) && !in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
+                    throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
                 }
 
                 $query->orderBy($part, $direction);

diff --git a/src/Knp/Component/Pager/Event/Subscriber/Sortable/SolariumQuerySubscriber.php b/src/Knp/Component/Pager/Event/Subscriber/Sortable/SolariumQuerySubscriber.php
@@ -38,10 +38,8 @@ public function items(ItemsEvent $event): void
                 $event->setCustomPaginationParameter('sorted', true);
                 $sortField = $event->options[PaginatorInterface::SORT_FIELD_PARAMETER_NAME];
                 if (null !== $sortField && $this->argumentAccess->has($sortField)) {
-                    if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                        if (!in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
-                            throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
-                        }
+                    if (isset($event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST]) && !in_array($this->argumentAccess->get($sortField), $event->options[PaginatorInterface::SORT_FIELD_ALLOW_LIST])) {
+                        throw new InvalidValueException("Cannot sort by: [{$this->argumentAccess->get($sortField)}] this field is not in allow list.");
                     }
 
                     $query->addSort($this->argumentAccess->get($sortField), $this->getSortDirection($event));

diff --git a/src/Knp/Component/Pager/Paginator.php b/src/Knp/Component/Pager/Paginator.php
@@ -12,7 +12,7 @@
 /**
  * Paginator uses event dispatcher to trigger pagination
  * lifecycle events. Subscribers are expected to paginate
- * wanted target and finally it generates pagination view
+ * wanted target, and finally it generates pagination view
  * which is only the result of paginator
  */
 final class Paginator implements PaginatorInterface

diff --git a/tests/Test/Fixture/Entity/Composite.php b/tests/Test/Fixture/Entity/Composite.php
@@ -26,33 +26,21 @@ class Composite
      */
     private ?string $uid = null;
 
-    /**
-     * Sets uid.
-     */
     public function setUid(?string $uid): void
     {
         $this->uid = $uid;
     }
 
-    /**
-     * Returns uid.
-     */
     public function getUid(): ?string
     {
         return $this->uid;
     }
 
-    /**
-     * Sets Id.
-     */
     public function setId(?int $id): void
     {
         $this->id = $id;
     }
 
-    /**
-     * Returns Id.
-     */
     public function getId(): ?int
     {
         return $this->id;