[ZF3] Paginator refactor

[GeeH]

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html

---

Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/5520
User: @snapshotpl
Created On: 2013-11-21T21:38:55Z
Updated At: 2014-09-28T00:34:45Z
Body
I think paginator needs changes. First of all I have been removed view dependency. We can also remove most static functions and properties.

And my questions:
I don't know whether we need Paginator\AdapterAggregateInterface ?
What are you think to bring scrolling style to view helper?

---

Comment

User: @Ocramius
Created On: 2013-11-22T00:39:40Z
Updated At: 2013-11-22T00:39:40Z
Body
@snapshotpl not sure about what you mean with AdapterAggregateInterface.

Very much agree on the changes so far though. Moving the scrolling style to view helpers is also a plus!

---

Comment

User: @froschdesign
Created On: 2013-11-22T10:58:07Z
Updated At: 2013-11-22T10:58:07Z
Body
The Paginator view helpers should also be moved to the Paginator namespace/component.

---

Comment

User: @bakura10
Created On: 2013-11-22T11:41:20Z
Updated At: 2013-11-22T11:41:20Z
Body
👍 on moving view helpers to Zend\Paginator!

---

Comment

User: @snapshotpl
Created On: 2013-11-22T12:29:24Z
Updated At: 2013-11-22T12:29:24Z
Body
AdapterAggregateInterface isn't implement in any class. Can you show any example of usage this interface? I think it's should be deleted.

---

Comment

User: @bakura10
Created On: 2013-11-22T13:03:53Z
Updated At: 2013-11-22T13:03:53Z
Body
@snapshotpl , the use case is for any class to implement this interface, and giving the class to the paginator. If the pagiantor detects that the class implements this class, it uses it to create an adapter.

To be honest I've never used it, I'm not sure if it's really useful.

---

Comment

User: @snapshotpl
Created On: 2013-11-22T13:36:31Z
Updated At: 2013-11-22T13:36:31Z
Body
@bakura10 That's right. But I can't imagine how it can be useful.

---

Comment

User: @bakura10
Created On: 2013-11-22T16:46:02Z
Updated At: 2013-11-22T16:46:02Z
Body
Well, I think I've thought about a use case: if you have an object that is responsible to create a paginator from an object, but you don't know which adapter to use because it depends on this object, then you can simply create the paginator from this object, and this object decides about which adapter to used.

---

Comment

User: @snapshotpl
Created On: 2013-11-22T16:58:56Z
Updated At: 2013-11-22T16:58:56Z
Body
That's some idea. @Ocramius what are you think?

---

Comment

User: @bakura10
Created On: 2013-11-24T11:30:39Z
Updated At: 2013-11-24T11:30:39Z
Body
@snapshotpl , you need to keep it. I have some ideas where it could be useful so this is not something we should remove ;-).

However, last time I check it, I think Paginator class could receive some cleaning:

• Removing the old conventions of prefixing protected/private methods by _ (example: https://github.com/zendframework/zf2/blob/master/library/Zend/Paginator/Paginator.php#L890)
• Remvoing useless variable (for instance: https://github.com/zendframework/zf2/blob/master/library/Zend/Paginator/Paginator.php#L82 why having a variable when we can simply check if $cache is null?)
• Reducing the amount of staticness. Paginator should work like any other components and by favoring proper injections. For instance the adapter plugin manager and the cache should be injected in the constructor.
• Scrolling styles seems nice and already quite clean, but you could quickly have a look :).
• Please rename the "Service" folder to "Factory": https://github.com/zendframework/zf2/tree/master/library/Zend/Paginator/Adapter/Service

Thanks for your help :).

Btw, for every things that may imply a BC (for instance, removing the _ in front of protected methods), please keep up to date this file: https://github.com/zendframework/zf2/wiki/ZF-3.0-Backwards-Compatibility-Breaks

---

Comment

User: @bakura10
Created On: 2013-11-26T10:33:04Z
Updated At: 2013-11-26T10:33:04Z
Body
Regarding removing staticness in order to favor factory usage, here is my thought:

1. Things like Cache, AdapterPluginManager... are injected into the Paginator:

class Paginator
{
    protected $adapterPluginManager; // No longer static

    protected $cache; // No longer static

    public function __construct(AdapterPluginManager $manager = null, CacheInterface $cache = null)
    {
        $this->adapterPluginManager = $manager ?: new AdapterPluginManager();
    }
}

Of course, we now need a built-in factory:

namespace Zend\Paginator;

class PaginatorFactory implements FactoryInterface
{
    public function createService($sl)
    {
        $pluginManager = $sl->get('Zend\Paginator\AdapterPluginManager');
        return new Paginator($pluginManager);
    }
}

And a factory for plugin manager that reads from config:

namespace Zend\Paginator;

class AdapterPluginManagerFactory implements FactoryInterface
{
    public function createService($sl)
    {
        $config = $sl->get('Config')['paginator_adapters'];
        return new AdapterPluginManager($config);
    }
}

This pattern should, I think, be used across the whole framework (this relates to this issue).

Of course it introduces a few caveats, because now everything should be created through service manager (and therefore, we would need to inject a service locator everywhere we want to use a Paginator).

What do you think @Ocramius @weierophinney ? Should we remove at no price the staticness or does it makes sense in some components in your opinion?

---

Comment

User: @Ocramius
Created On: 2013-11-26T11:25:09Z
Updated At: 2013-11-26T11:25:09Z
Body
Wow, I wasn't even aware that the cache was static in there. A couple of things:

• why do we need a plugin manager for adapters? Adapters are usually constructed with a data source (often parametrized)
• can we avoid having a nullable cache? I'd rather use a null cache implementation (memory, with ttl = 0) than having a nullable cache property

---

Comment

User: @bakura10
Created On: 2013-11-26T11:29:05Z
Updated At: 2013-11-26T11:29:05Z
Body
The Paginator indeed needed some cleaning :D. It makes sense for the plugin manager. Actually, I can't see where the adapter plugin manager is used.... :/

---

Comment

User: @marc-mabe
Created On: 2013-12-02T20:46:17Z
Updated At: 2013-12-02T20:46:17Z
Body
Please don't forget to move caching to adapters - see #3311 #3190

---

Comment

User: @weierophinney
Created On: 2014-03-03T17:06:52Z
Updated At: 2014-03-03T17:06:52Z
Body
These changes are backwards incompatible, so I'm marking this for the 3.0.0 milestone.

---

[weierophinney]

This repository has been closed and moved to laminas/laminas-paginator; a new issue has been opened at laminas/laminas-paginator#4.