Merge pull request #4 from spaceonfire/paginable-criteria

refactor(pagination): fix some errors in PaginableCriteria
spaceonfire · Feb 25, 2021 · 0b29a0b · 0b29a0b
2 parents 93872b5 + d672f0a
commit 0b29a0b
Show file tree

Hide file tree

Showing 9 changed files with 182 additions and 89 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,12 @@ Updates should follow the [Keep a CHANGELOG](http://keepachangelog.com/) princip
 - Nothing
 -->
 
+## [1.2.0] - 2020-02-25
+
+### Added
+
+- Fix some errors with PaginableCriteria.
+
 ## [1.1.1] - 2020-02-16
 
 ### Added

diff --git a/composer.json b/composer.json
@@ -51,7 +51,7 @@
   },
   "extra": {
     "branch-alias": {
-      "dev-master": "1.1-dev"
+      "dev-master": "1.2-dev"
     }
   },
   "config": {

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -12,12 +12,6 @@ parameters:
 	paths:
 		- src/
 	ignoreErrors:
-		-
-			message: '/^Parameter \#1 \$function of function call_user_func_array expects callable/'
-			paths:
-				- src/AbstractCriteriaDecorator.php
-				- src/Expression/ExpressionFactory.php
-				- src/Expression/AbstractExpressionDecorator.php
 		- '/^Method (.*) should return static\((.*)\) but returns (.*)\.$/'
 		-
 			message: '/^If condition is always false\.$/'

diff --git a/src/AbstractCriteriaDecorator.php b/src/AbstractCriteriaDecorator.php
@@ -44,7 +44,15 @@ public function getInnerCriteria(bool $recursive = true): CriteriaInterface
      */
     final protected function proxyCall(string $methodName, array $arguments = [])
     {
-        return call_user_func_array([$this->criteria, $methodName], $arguments);
+        $callable = [$this->criteria, $methodName];
+
+        if (!is_callable($callable)) {
+            throw new \BadMethodCallException(
+                sprintf('Call to an undefined method %s::%s()', get_class($this->criteria), $methodName)
+            );
+        }
+
+        return call_user_func_array($callable, $arguments);
     }
 
     /**

diff --git a/src/Bridge/SpiralPagination/PaginableCriteria.php b/src/Bridge/SpiralPagination/PaginableCriteria.php
@@ -10,12 +10,11 @@
 use Spiral\Pagination\PaginableInterface;
 use Spiral\Pagination\Paginator;
 use Spiral\Pagination\PaginatorInterface;
-use Webmozart\Assert\Assert;
 
-class PaginableCriteria extends AbstractCriteriaDecorator implements PaginableInterface
+class PaginableCriteria extends AbstractCriteriaDecorator implements PaginableInterface, PaginatorInterface
 {
     /**
-     * @var PaginatorInterface|Paginator|null
+     * @var PaginatorInterface|Paginator
      */
     private $paginator;
 
@@ -28,43 +27,28 @@ public function __construct(?CriteriaInterface $criteria = null, ?PaginatorInter
     {
         parent::__construct($criteria ?? new Criteria());
 
-        if ($paginator !== null) {
+        if ($paginator === null) {
+            $this->resetPaginator();
+        } else {
             $this->paginator = $paginator;
             $paginator->paginate($this);
-        } else {
-            $this->resetPaginator();
         }
     }
 
     /**
-     * Getter for `paginator` property
-     * @return PaginatorInterface|Paginator
+     * Clone criteria.
      */
-    public function getPaginator(): PaginatorInterface
+    public function __clone()
     {
-        Assert::notNull($this->paginator);
-        return $this->paginator;
+        $this->paginator = clone $this->paginator;
     }
 
     /**
-     * @return PaginatorInterface|Paginator
+     * @inheritDoc
      */
-    private function makePaginator(): PaginatorInterface
-    {
-        $paginator = $this->paginator ?? new Paginator();
-
-        Assert::isInstanceOf($paginator, Paginator::class);
-
-        $limit = $this->getLimit() ?? 25;
-        $tmpCount = $limit + $this->getOffset();
-        $page = (int)($this->getOffset() / $limit) + 1;
-
-        return $paginator->withCount(max($tmpCount, $paginator->count()))->withLimit($limit)->withPage($page);
-    }
-
-    private function resetPaginator(): void
+    public function getLimit(): int
     {
-        $this->paginator = $this->makePaginator();
+        return parent::getLimit() ?? 25;
     }
 
     /**
@@ -88,12 +72,29 @@ public function offset(?int $offset): CriteriaInterface
     }
 
     /**
-     * Clone criteria
+     * @inheritDoc
      */
-    public function __clone()
+    public function paginate(PaginableInterface $target): PaginatorInterface
     {
-        if ($this->paginator !== null) {
-            $this->paginator = clone $this->paginator;
-        }
+        $this->paginator = $this->paginator->paginate($target);
+
+        return $this;
+    }
+
+    /**
+     * Getter for `paginator` property
+     * @return PaginatorInterface|Paginator
+     */
+    public function getPaginator(): PaginatorInterface
+    {
+        return $this->paginator;
+    }
+
+    private function resetPaginator(): void
+    {
+        $this->paginator = (new Paginator())
+            ->withLimit($this->getLimit())
+            ->withPage((int)($this->getOffset() / $this->getLimit()) + 1)
+            ->withCount($this->paginator instanceof \Countable ? $this->paginator->count() : 0);
     }
 }
diff --git a/src/Expression/AbstractExpressionDecorator.php b/src/Expression/AbstractExpressionDecorator.php
@@ -4,7 +4,6 @@
 
 namespace spaceonfire\Criteria\Expression;
 
-use BadMethodCallException;
 use spaceonfire\Criteria\Criteria;
 use Symfony\Component\PropertyAccess\PropertyPath;
 use Webmozart\Expression\Expression;
@@ -188,25 +187,37 @@ public function orX(Expression $expr): Expression
      * @see ExpressionFactory
      */
     public function __call(string $name, array $arguments = [])
+    {
+        if (null !== $expr = $this->magicLogicalExpression($name, $arguments)) {
+            return $expr;
+        }
+
+        throw new \BadMethodCallException(sprintf('Call to an undefined method %s::%s()', static::class, $name));
+    }
+
+    private function magicLogicalExpression(string $name, array $arguments): ?Expression
     {
         $isAnd = strpos($name, 'and') === 0;
         $isOr = strpos($name, 'or') === 0;
 
         if (!$isAnd && !$isOr) {
-            throw new BadMethodCallException('Call to an undefined method ' . static::class . '::' . $name . '()');
+            return null;
         }
 
         $factoryMethod = lcfirst(substr($name, $isAnd ? 3 : 2));
 
-        try {
-            if (stripos($factoryMethod, 'and') !== 0 && stripos($factoryMethod, 'or') !== 0) {
-                $expr = call_user_func_array([Criteria::expr(), $factoryMethod], $arguments);
-                return $isAnd ? $this->andX($expr) : $this->orX($expr);
-            }
-        } catch (BadMethodCallException $e) {
-            // skip BadMethodCallException from ExpressionFactory
+        if (stripos($factoryMethod, 'and') === 0 || stripos($factoryMethod, 'or') === 0) {
+            return null;
         }
 
-        throw new BadMethodCallException('Call to an undefined method ' . static::class . '::' . $name . '()');
+        $factory = [Criteria::expr(), $factoryMethod];
+
+        if (!is_callable($factory)) {
+            return null;
+        }
+
+        $expr = call_user_func_array($factory, $arguments);
+
+        return $isAnd ? $this->andX($expr) : $this->orX($expr);
     }
 }
diff --git a/src/Expression/ExpressionFactory.php b/src/Expression/ExpressionFactory.php
@@ -134,44 +134,59 @@ public function property($propertyName, Expression $expr): Selector
      */
     public function __call(string $name, array $arguments = [])
     {
-        $proxyMethods = array_flip([
-            'all',
-            'andX',
-            'atLeast',
-            'atMost',
-            'contains',
-            'count',
-            'endsWith',
-            'equals',
-            'exactly',
-            'false',
-            'greaterThan',
-            'greaterThanEqual',
-            'in',
-            'isEmpty',
-            'isInstanceOf',
-            'keyExists',
-            'keyNotExists',
-            'lessThan',
-            'lessThanEqual',
-            'matches',
-            'method',
-            'not',
-            'notEmpty',
-            'notEquals',
-            'notNull',
-            'notSame',
-            'null',
-            'orX',
-            'same',
-            'startsWith',
-            'true',
-        ]);
-
-        if (array_key_exists($name, $proxyMethods)) {
-            return call_user_func_array([Expr::class, $name], $arguments);
+        if (null !== $expr = $this->proxyCall($name, $arguments)) {
+            return $expr;
         }
 
         throw new BadMethodCallException('Call to an undefined method ' . static::class . '::' . $name . '()');
     }
+
+    private function proxyCall(string $name, array $arguments = []): ?Expression
+    {
+        $proxyMethods = [
+            'all' => true,
+            'andX' => true,
+            'atLeast' => true,
+            'atMost' => true,
+            'contains' => true,
+            'count' => true,
+            'endsWith' => true,
+            'equals' => true,
+            'exactly' => true,
+            'false' => true,
+            'greaterThan' => true,
+            'greaterThanEqual' => true,
+            'in' => true,
+            'isEmpty' => true,
+            'isInstanceOf' => true,
+            'keyExists' => true,
+            'keyNotExists' => true,
+            'lessThan' => true,
+            'lessThanEqual' => true,
+            'matches' => true,
+            'method' => true,
+            'not' => true,
+            'notEmpty' => true,
+            'notEquals' => true,
+            'notNull' => true,
+            'notSame' => true,
+            'null' => true,
+            'orX' => true,
+            'same' => true,
+            'startsWith' => true,
+            'true' => true,
+        ];
+
+        if (!array_key_exists($name, $proxyMethods)) {
+            return null;
+        }
+
+        $factory = [Expr::class, $name];
+
+        if (!is_callable($factory)) {
+            return null;
+        }
+
+        return call_user_func_array($factory, $arguments) ?: null;
+    }
 }
diff --git a/tests/AbstractCriteriaDecoratorTest.php b/tests/AbstractCriteriaDecoratorTest.php
@@ -9,6 +9,10 @@ class AbstractCriteriaDecoratorTest extends AbstractCriteriaTest
     private function factory(?CriteriaInterface $criteria = null): AbstractCriteriaDecorator
     {
         return new class($criteria ?? new Criteria()) extends AbstractCriteriaDecorator {
+            public function proxyCallUnknown(): void
+            {
+                $this->proxyCall('unknownMethodName');
+            }
         };
     }
 
@@ -26,4 +30,11 @@ public function testGetInnerCriteria(): void
         self::assertEquals($innerCriteria, $outerCriteriaAdapter->getInnerCriteria());
         self::assertInstanceOf(AbstractCriteriaDecorator::class, $outerCriteriaAdapter->getInnerCriteria(false));
     }
+
+    public function testProxyCallUnknownMethod(): void
+    {
+        $this->expectException(\BadMethodCallException::class);
+        $criteria = $this->factory();
+        $criteria->proxyCallUnknown();
+    }
 }