Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

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

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

sasezaki
Copy link

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);

@marc-mabe
Copy link
Member

@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.

1 similar comment
@marc-mabe
Copy link
Member

@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.

@Ocramius
Copy link
Member

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)

@sasezaki
Copy link
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 ?

@marc-mabe
Copy link
Member

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

sasezaki added a commit to sasezaki/zend-paginator that referenced this pull request Jan 17, 2016
Don't add new method when Refactoring.
(oops, jsonSerialize() introduced with my misunderstood usage.)
@weierophinney weierophinney added this to the 3.0.0 milestone Apr 11, 2016
@weierophinney
Copy link
Member

@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!

protected function _getCacheInternalId()
{
return md5(serialize([
spl_object_hash($this->getAdapter()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cache internal id fixed by #33 , this may need copy function from current master branch.

@weierophinney
Copy link
Member

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

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-paginator. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-paginator to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-paginator.
  • In your clone of laminas/laminas-paginator, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants