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

Pagination doesn't redirect to error page, shows last page instead. #1887

Closed
manuelmoreale opened this issue Jun 28, 2019 · 3 comments

Comments

@manuelmoreale
Copy link

@manuelmoreale manuelmoreale commented Jun 28, 2019

Describe the bug
The current pagination, instead of returning an error page when trying to visit a page number that doesn't exist (for example visiting page:50 in a collection that has only 10 pages) it's returning the last page of the pagination.

To Reproduce
Steps to reproduce the behavior:

  1. In the starterkit homepage template, add a paginate() in the foreach loop. For example foreach (page('photography')->children()->listed()->paginate(2)
  2. Try open a page with a stupidly high page number

Expected behavior
You should be redirected to the error page.

Screenshots
screenshot_2019-06-28_at_12 27 45

Kirby Version
This is happening in the current 3.2.0 but @texnixe has tested this and told me it goes back to 3.1.0

@bastianallgeier bastianallgeier added this to the 3.2.1 milestone Jul 1, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.2.1, 3.2.2, 3.3.0 Jul 1, 2019
@bastianallgeier bastianallgeier modified the milestones: 3.3.0, 3.2.4 Aug 6, 2019
@bastianallgeier bastianallgeier removed this from the 3.2.4 milestone Aug 22, 2019
@lukasbestle lukasbestle added this to the 3.2.5 milestone Sep 14, 2019
lukasbestle added a commit that referenced this issue Sep 14, 2019
@lukasbestle lukasbestle self-assigned this Sep 14, 2019
@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Sep 14, 2019

I'm not sure if it should redirect to the error page by default – depending on the use-case, the behavior should be configurable. Maybe the dev wants to show a custom error message instead or redirect to the URL of the last existing page.

I have implemented a new $pagination->hasCurrentPage() method that can be used in the controller to check if an overflow happened and to react accordingly.

lukasbestle added a commit that referenced this issue Sep 15, 2019
Otherwise the $pagination->hasCurrentPage() method won’t work.

Caching the result here does not have a huge benefit anyway as the calculations aren’t hard.
@manuelmoreale

This comment has been minimized.

Copy link
Author

@manuelmoreale manuelmoreale commented Sep 16, 2019

@lukasbestle the reason why I flagged this at first it's because the old K2 behaviour was to redirect to the 404 page.

Which make sense in my opinion since you're trying to access a page that doesn't exist.

Returning the last page also breaks every infinite scroll implementation and that's how I noticed the error in the first place.

IMO, the default behaviour should be to return a 404 and then maybe add the possibility to overwrite that if someone wants to handle this specific situation in a different way.

@lukasbestle lukasbestle modified the milestones: 3.2.5, 3.3.0 Sep 16, 2019
lukasbestle added a commit that referenced this issue Sep 29, 2019
lukasbestle added a commit that referenced this issue Sep 29, 2019
lukasbestle added a commit that referenced this issue Sep 29, 2019
- Migrate different setters to a common setProperties() method
- Throw ErrorPageException if passed page is out of bounds
lukasbestle added a commit that referenced this issue Sep 29, 2019
Otherwise the ErrorPageException would get thrown if the pagination does not contain elements and therefore does not have any pages; the default page is now automatically determined if not passed.
lukasbestle added a commit that referenced this issue Sep 29, 2019
- Migrate different setters to a common setProperties() method
- Throw ErrorPageException if passed page is out of bounds
lukasbestle added a commit that referenced this issue Sep 29, 2019
Otherwise the ErrorPageException would get thrown if the pagination does not contain elements and therefore does not have any pages; the default page is now automatically determined if not passed.
lukasbestle added a commit that referenced this issue Sep 29, 2019
lukasbestle added a commit that referenced this issue Oct 3, 2019
lukasbestle added a commit that referenced this issue Oct 3, 2019
lukasbestle added a commit that referenced this issue Oct 3, 2019
- Migrate different setters to a common setProperties() method
- Throw ErrorPageException if passed page is out of bounds
lukasbestle added a commit that referenced this issue Oct 3, 2019
Otherwise the ErrorPageException would get thrown if the pagination does not contain elements and therefore does not have any pages; the default page is now automatically determined if not passed.
lukasbestle added a commit that referenced this issue Oct 3, 2019
bastianallgeier added a commit that referenced this issue Oct 7, 2019
bastianallgeier added a commit that referenced this issue Oct 7, 2019
bastianallgeier added a commit that referenced this issue Oct 7, 2019
bastianallgeier added a commit that referenced this issue Oct 7, 2019
- Migrate different setters to a common setProperties() method
- Throw ErrorPageException if passed page is out of bounds
bastianallgeier added a commit that referenced this issue Oct 7, 2019
Otherwise the ErrorPageException would get thrown if the pagination does not contain elements and therefore does not have any pages; the default page is now automatically determined if not passed.
bastianallgeier added a commit that referenced this issue Oct 7, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Oct 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.