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

Rename property of pagination page object to be more intuitive #13492

Closed
scrnjakovic opened this Issue Sep 19, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@scrnjakovic
Copy link
Contributor

scrnjakovic commented Sep 19, 2018

I propose changing before property of page object to previous which is far more intuitive for given context, as before goes better with after, not next.

I know this is minor and breaks BC, but maybe we can do it in 4.0? Or for some time, allow both properties.

EDIT: I also think that having total_pages and last is a bit redundant. I propose removing last.

EDIT2: I'd also rename $paginator->getPaginate() to either $paginator->getPage() or $paginator->paginate() for the same reason - being more intuitive.

@niden

This comment has been minimized.

Copy link
Member

niden commented Sep 19, 2018

I agree with these changes, it will make things a bit more logical I think.

We can implement this right now for the next minor release but we will need to keep both versions available to keep backwards compatibility for 4.0 version as well. We can deprecate the old properties after 4.0.

In summary:

  • Introduce one more element called previous which will have the same value as before
  • Add a method called paginate() which will produce the same result as getPaginate()
  • Keep total_pages for now to be later removed since the same information is in last
  • Add @deprecated to getPaginate() to inform the developers that this will be removed after 4.0
@niden

This comment has been minimized.

Copy link
Member

niden commented Sep 19, 2018

@phalcon/core-team Thoughts?

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Sep 19, 2018

Overall i agree. If we are going to release next minor version then we can add new methods.

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit to niden/cphalcon that referenced this issue Sep 19, 2018

niden added a commit that referenced this issue Oct 10, 2018

Merge pull request #13494 from niden/4.0.x
[#13492] Changes to the paginator

niden added a commit that referenced this issue Oct 10, 2018

Merge pull request #13493 from niden/3.4.x
[#13492] Changes to the paginator
@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 10, 2018

Implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment