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

Fix | v1 - Deprecate Invalid Page Property #10

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

Sammyjo20
Copy link
Member

@Sammyjo20 Sammyjo20 commented Dec 7, 2023

This PR introduces a new property to the abstract Paginator class: $this->currentPage. This property should be used as a replacement for the $this->page property especially when used inside of the isLastPage method.

I have deprecated the old page property and getPage method because they would often be one figure higher than they should be. The reason why is with the order of how iterators work in PHP.

  1. Rewind is called
  2. Valid is called
  3. Current + Key is called
  4. Next is called
  5. Go to step 1 again

The old page property started at 1 on rewind so what happened was:

  1. Rewind is called - $this->page = 1
  2. Valid is called - No response yet so isLastPage is not called
  3. Current + Key is called - Get first page
  4. Next is called - $this->page = 2
  5. Valid is called - This is the issue - you would expect $this->page to be 1 but it's actually 2

The new currentPage property is now being used internally in the code which starts at zero instead of one and all tests pass.

Copy link
Member Author

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Self review

@Sammyjo20 Sammyjo20 changed the title Fix | Deprecate Invalid Page Property Fix | v1 | Deprecate Invalid Page Property Dec 7, 2023
@Sammyjo20 Sammyjo20 changed the title Fix | v1 | Deprecate Invalid Page Property Fix | v1 - Deprecate Invalid Page Property Dec 7, 2023
@Sammyjo20 Sammyjo20 merged commit e15977c into v1 Dec 8, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant