[ZF3] Paginator refactor

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 :+1: 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:

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.

zendframework / zend-paginator