Add paginator factory & adapter plugin manager #2316

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

blanchonvincent commented Sep 8, 2012

Add paginator factory & adapter plugin manager and suite tests

blanchonvincent added some commits Sep 8, 2012

@blanchonvincent blanchonvincent Add paginator factory & paginator adapater plugin manager
Add paginator factory & paginator adapater plugin manager
f8d6046
@blanchonvincent blanchonvincent Add adapter plugin manager tests
Add adapter plugin manager tests
76ee4e7
@blanchonvincent blanchonvincent Add factory paginator tests
Add factory paginator tests
76f8c64
@blanchonvincent blanchonvincent Add test with AdapterAggregateInterface
Add test with AdapterAggregateInterface
b278ea4
@blanchonvincent blanchonvincent Fix unit tests error
Fix unit tests error
611e92a

@weierophinney weierophinney and 1 other commented on an outdated diff Sep 13, 2012

library/Zend/Paginator/Factory.php
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Paginator
+ */
+
+namespace Zend\Paginator;
+
+use Zend\Paginator\Adapter\AdapterInterface;
+
+/**
+ * @category Zend
+ * @package Zend_Paginator
+ */
+class Factory
@weierophinney

weierophinney Sep 13, 2012

Owner

For classes that are only offering static methods, we typically recommend making them abstract. They do not need the "Abstract" prefix, however.

@blanchonvincent

blanchonvincent Sep 13, 2012

Contributor

I keep the same that Zend\Serializer\Serializer to keep consistency.

Owner

weierophinney commented Sep 13, 2012

We actually removed factory functionality from Zend\Paginator prior to the stable release. What is the rationale you have for re-adding it?

The reason we removed it is because it's usually safer and easier to simply instantiate a Paginator and provide the adapter instance fully configured; this is particularly useful when you consider adapters that may require multiple options.

Can you bring this PR up on the ML to make your case and get feedback?

Contributor

blanchonvincent commented Sep 13, 2012

Oh ok, I did not know that it had been removed. I added because it's developer friendy. At the work we use paginator, and factory is easier to read.

Owner

weierophinney commented Sep 13, 2012

@blanchonvincent As noted, please bring it on the ML so we can discuss it. Show how it would be used, and indicate why you feel it's necessary. It'll be a 2.1 feature at the earliest.

weierophinney reopened this Sep 14, 2012

Contributor

blanchonvincent commented Sep 15, 2012

Ok not a problem :) But what is the "ML" ?

@blanchonvincent blanchonvincent Add class as abstract
Make class abstract, offer only static methods
1774c44
Owner

weierophinney commented Sep 15, 2012

Mailing list - zf-contributors at lists.zend.com.

On Saturday, September 15, 2012, Blanchon Vincent wrote:

Ok not a problem :) But what is the "ML" ?


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/2316#issuecomment-8582860.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@blanchonvincent blanchonvincent Add unit test and improve existing
Add unit test and improve existing
69818cb
Contributor

blanchonvincent commented Sep 20, 2012

Ok mail sent :)

Owner

weierophinney commented Sep 25, 2012

Typically with adapter factories we allow passing a single array/Traversable to the factory, and inside that array, hint the adapter type/class name. Can you make that addition, please? Having the adapter name as a second, optional argument can still be allowed, but if we want to make the factory configuration driven, it should definitely allow passing the adapter via the options.

Contributor

blanchonvincent commented Sep 27, 2012

Ok i commit that in few hours

Contributor

blanchonvincent commented Sep 27, 2012

Ok, PR updated and fix for PSR2.

blanchonvincent added some commits Sep 27, 2012

@blanchonvincent blanchonvincent Updte factory for one argument
Updte factory for one argument
3da4598
@blanchonvincent blanchonvincent Add tests for factory for one argument
Add tests for factory for one argument
dd24e43

@weierophinney weierophinney commented on an outdated diff Oct 1, 2012

library/Zend/Paginator/AdapterPluginManager.php
+ $invokable = $this->invokableClasses[$canonicalName];
+
+ if (null === $this->creationOptions
+ || (is_array($this->creationOptions) && empty($this->creationOptions))
+ ) {
+ $instance = new $invokable();
+ } else {
+ if($canonicalName == "dbselect" && is_array($this->creationOptions)) {
+ $class = new \ReflectionClass($invokable);
+ $instance = $class->newInstanceArgs($this->creationOptions);
+ } else {
+ $instance = new $invokable($this->creationOptions);
+ }
+ }
+
+ return $instance;
@weierophinney

weierophinney Oct 1, 2012

Owner

Instead of if/else and nesting conditionals, return as soon as you have an instance.

@weierophinney weierophinney commented on an outdated diff Oct 1, 2012

library/Zend/Paginator/Factory.php
+
+use Traversable;
+use Zend\Paginator\Adapter\AdapterInterface;
+use Zend\Stdlib\ArrayUtils;
+
+/**
+ * @category Zend
+ * @package Zend_Paginator
+ */
+abstract class Factory
+{
+ protected static $adapters;
+
+ public static function factory($items, $adapter = null)
+ {
+ if(null === $adapter) {
@weierophinney

weierophinney Oct 1, 2012

Owner

add a space between "if" and the opening paren

@weierophinney weierophinney commented on an outdated diff Oct 1, 2012

library/Zend/Paginator/Factory.php
+
+/**
+ * @category Zend
+ * @package Zend_Paginator
+ */
+abstract class Factory
+{
+ protected static $adapters;
+
+ public static function factory($items, $adapter = null)
+ {
+ if(null === $adapter) {
+ if ($items instanceof Traversable) {
+ $items = ArrayUtils::iteratorToArray($items);
+ }
+ if(!is_array($items)) {
@weierophinney

weierophinney Oct 1, 2012

Owner

Add space between if and opening paren. Same comment for lines 38 and 48, too.

Owner

weierophinney commented Oct 1, 2012

Besides the CS and logic flow issues noted above, I'd actually add a service listener for retrieving the paginator plugin manager similar to how view helpers, controllers, and controller plugins are done currently. This will make it easy to configure the plugin manager application-wide with any additional paginators you might use, and then inject that plugin manager where it needs to be used.

@blanchonvincent blanchonvincent CS space & returning instance
CS space & returning instance
9b1378c
Contributor

blanchonvincent commented Oct 8, 2012

Fix CS. I added a factory like view helpers, controllers, and controller plugins are done currently to retrieve plugin manager and add paginator plugins. Say me if it is what you wanted.

@weierophinney weierophinney commented on an outdated diff Oct 10, 2012

library/Zend/Paginator/Factory.php
+ throw new Exception\InvalidArgumentException(
+ 'The factory needs an associative array '
+ . 'or a Traversable object as an argument when '
+ . "it's used with one parameter"
+ );
+ }
+ if (!isset($items['adapter']) && !isset($items['items'])) {
+ throw new Exception\InvalidArgumentException(
+ 'The factory needs an associative array '
+ . 'or a Traversable object with keys '
+ . '"adapter" and "items"'
+ );
+ }
+ $adapter = $items['adapter'];
+ $items = $items['items'];
+ }
@weierophinney

weierophinney Oct 10, 2012

Owner

Extract the above into a separate method, and then call and return from that method:

protected static function createAdapterFromItems($items)
{
    // do the work from inside the conditional, and then:
    $adapter = static::getAdapterFromManager($adapter, $Items);
    return $adapter
}

protected static function getAdapterFromManager($adapter, $items)
{
    if ($adapter instanceof AdapterInterface || $adapter instanceof AdapterAggregateInterface) {
        return new Paginator($adapter);
    }
    $adapter = static::getAdapterPluginManager()->get($adapter, $items);
    return new Paginator($adapter);
}

This will help keep the method more readable. Also notice the use of "static" instead of "self"; this allows for better inheritance and extension.

@weierophinney weierophinney commented on an outdated diff Oct 10, 2012

library/Zend/Paginator/Factory.php
+ . "it's used with one parameter"
+ );
+ }
+ if (!isset($items['adapter']) && !isset($items['items'])) {
+ throw new Exception\InvalidArgumentException(
+ 'The factory needs an associative array '
+ . 'or a Traversable object with keys '
+ . '"adapter" and "items"'
+ );
+ }
+ $adapter = $items['adapter'];
+ $items = $items['items'];
+ }
+ if (!$adapter instanceof AdapterInterface && !$adapter instanceof AdapterAggregateInterface) {
+ $adapter = self::getAdapterPluginManager()->get($adapter, $items);
+ }
@weierophinney

weierophinney Oct 10, 2012

Owner

Replace the above with the "createAdapterFromManager()" method I detailed in a previous comment.

@weierophinney weierophinney commented on an outdated diff Oct 10, 2012

library/Zend/Paginator/Factory.php
+ if (!$adapter instanceof AdapterInterface && !$adapter instanceof AdapterAggregateInterface) {
+ $adapter = self::getAdapterPluginManager()->get($adapter, $items);
+ }
+
+ return new Paginator($adapter);
+ }
+
+ /**
+ * Change the adapter plugin manager
+ *
+ * @param AdapterPluginManager $adapters
+ * @return void
+ */
+ public static function setAdapterPluginManager(AdapterPluginManager $adapters)
+ {
+ self::$adapters = $adapters;
@weierophinney

weierophinney Oct 10, 2012

Owner

use "static", not "self".

@weierophinney weierophinney commented on an outdated diff Oct 10, 2012

library/Zend/Paginator/Factory.php
+ public static function setAdapterPluginManager(AdapterPluginManager $adapters)
+ {
+ self::$adapters = $adapters;
+ }
+
+ /**
+ * Get the adapter plugin manager
+ *
+ * @return AdapterPluginManager
+ */
+ public static function getAdapterPluginManager()
+ {
+ if (self::$adapters === null) {
+ self::$adapters = new AdapterPluginManager();
+ }
+ return self::$adapters;
@weierophinney

weierophinney Oct 10, 2012

Owner

Use "static", not "self".

Owner

weierophinney commented Oct 10, 2012

It may also be useful to have a PaginatorFactory for use with the service manager; it could then utilize the configured paginator adapter, using the adapter manager via the service factory you've already created.

Contributor

blanchonvincent commented Oct 10, 2012

Thanks for the advices ! Code is updated.
Just one question with self/static. Some parts of ZF2 code keep self like me, we should, with time, change with "static" to keep consistency ?

@blanchonvincent blanchonvincent Code refactory to make easier
Code refactory to make easier
8c81500
Owner

weierophinney commented Oct 16, 2012

@blanchonvincent Anytime the class could be extended and has static values, we should likely use LSB to make the code more adaptable.

@weierophinney weierophinney commented on an outdated diff Oct 16, 2012

library/Zend/Paginator/AdapterPluginManager.php
+ */
+ protected function createFromInvokable($canonicalName, $requestedName)
+ {
+ $invokable = $this->invokableClasses[$canonicalName];
+
+ if (null === $this->creationOptions
+ || (is_array($this->creationOptions) && empty($this->creationOptions))
+ ) {
+ return new $invokable();
+ } else {
+ if($canonicalName == "dbselect" && is_array($this->creationOptions)) {
+ $class = new \ReflectionClass($invokable);
+ return $class->newInstanceArgs($this->creationOptions);
+ } else {
+ return new $invokable($this->creationOptions);
+ }
@weierophinney

weierophinney Oct 16, 2012

Owner

Several things:

  • If you have a return in a conditional body, you don't need an else.
  • Invokables should all be treated the same, which leads to:
  • You should likely define a factory for the DbSelect adapter instead. Having conditional logic based on type is a recipe for problems; what if somebody substitutes an alternate DbSelect implementation that isn't compatible with this?

Alternately, modify the DbSelect adapter to allow accepting an array of arguments, just like the other adapters.

Owner

weierophinney commented Oct 16, 2012

Getting closer, @blanchonvincent !

Contributor

blanchonvincent commented Oct 18, 2012

@weierophinney, Yeah, it's really better with a factory for dbselect !
Thank you for the advice.

@weierophinney weierophinney added a commit to zendframework/zend-paginator that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/paginator-factory' into develop 7400cfb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment