Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v3] Refactor: Cut Redundant Dependency in order to use as Standalone #5

Closed
weierophinney opened this issue Dec 31, 2019 · 10 comments
Closed
Labels
Won't Fix This will not be worked on

Comments

@weierophinney
Copy link
Member

Refactor: Cut Redundant Dependency in order to use as Standalone.

When using Paginator class as Dependency, there is problem.

  • A lot of method is defined, regardless of whether or not to use.
    • json, filter, render, cache
  • ScrollingStyle is static dependecy.
    • it's not need when ScrollingStyle object is passed.
      • eg. $paginator->getPages(new Zend\Paginator\ScrollingStyle\All()))

Above thing is a headache when to use standalone.

So I request refactoring this component.

Introduced below interface, trait, class.

  • PaginatorInterface
  • JsonSerializeTrait, RenderTrait, FilterTrait, CachedTrait
  • SimplePaginator
  • GlobalPaginator

.. And Paginator class can implements like below, without BC break.

class Paginator extends GlobalPaginator
{
    use JsonSerializeTrait,
        RenderTrait,
        FilterTrait,
        CachedTrait;

Only ScrollingStyleInterface implentation must be changed.
BEFORE public function getPages(Paginator $paginator, $pageRange = null);
AFTER public function getPages(PaginatorInterface $paginator, $pageRange = null);


Originally posted by @sasezaki at zendframework/zend-paginator#9

@weierophinney
Copy link
Member Author

@sasezaki the caching should also be moved into the adapters because the adapter only know how to cache paginater results. I think the issues for that are in the zf2 repo.


Originally posted by @marc-mabe at zendframework/zend-paginator#9 (comment)

@weierophinney
Copy link
Member Author

@sasezaki the caching should also be moved into the adapters because the adapter only know how to cache paginater results. I think the issues for that are in the zf2 repo.


Originally posted by @marc-mabe at zendframework/zend-paginator#9 (comment)

@weierophinney
Copy link
Member Author

Caching could still exist, but on a separate instance. Agreed tho: it needs to die for now (the current approach is using static caching, that can lead to horrible SNAFU moments)


Originally posted by @Ocramius at zendframework/zend-paginator#9 (comment)

@weierophinney
Copy link
Member Author

To be clear, talking cache problem is current v2 Paginator implemntaions ?
I agree caching problem.

Firstly, this PR only tried cutting Redundant Dependency for current implementaion.
Should I continue update on this PR as complete update for v3 ?


Originally posted by @sasezaki at zendframework/zend-paginator#9 (comment)

@weierophinney
Copy link
Member Author

@sasezaki @Ocramius I agree that the caching issue is not part of this PR but it should be addressed in another PR before v3.


Originally posted by @marc-mabe at zendframework/zend-paginator#9 (comment)

@weierophinney
Copy link
Member Author

@sasezaki Could you provide some usage examples? These will be important for understanding the impact to end users, and will also shed more light on the design changes.

Thanks!


Originally posted by @weierophinney at zendframework/zend-paginator#9 (comment)

@Moln
Copy link

Moln commented Jan 7, 2020

JsonSerializeTrait, RenderTrait, FilterTrait, CachedTrait

FilterTrait, CachedTrait

I think CacheWrapperAdapter, FilterWrapperAdapter will be better then FilterTrait, CachedTrait.

class CacheWrapperAdapter implements AdapterInterface {
    public function __construct(AdapterInterface $adapter, CacheInterface $cache)
    { }
}

new Paginator(new CacheWrapperAdapter(new DbSelectAdapter(), $cache));
class FilterWrapperAdapter implements AdapterInterface {
    public function __construct(AdapterInterface $adapter, FilterInterface $filter)
    { }
}

new Paginator(new FilterWrapperAdapter(new DbSelectAdapter(), $filter));

JsonSerializeTrait & toJson() method

I think no need it in paginator . And it can be removed.

RenderTrait & render, __toString(), (g|s)etView() methods.

It think can be removed, too.

@froschdesign
Copy link
Member

The original pull request contains some good points, but the implementation is wrong and cannot be followed up:

  • the current code is split in classes and traits without further improvements of the API
  • BC breaks are introduced with the same code
  • traits using undefined methods

A new major version that breaks backwards compatibility should offer more improvements for the user, the code and component itself.

However, we will continue to use the suggestions of the pull request to create a RFC for the new major version.

Thank you for this contribution!

@froschdesign froschdesign added Won't Fix This will not be worked on and removed Awaiting Author Updates labels Jun 22, 2020
@froschdesign
Copy link
Member

@Moln

I think CacheWrapperAdapter, FilterWrapperAdapter will be better then FilterTrait, CachedTrait.

"Wrapper" is nameless and without any technical meaning, therefore it should be avoid.

@froschdesign
Copy link
Member

@sasezaki
Btw. the documentation contains now a description to use paginator as stand-alone:

https://docs.laminas.dev/laminas-paginator/application-integration/stand-alone/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Won't Fix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants