Skip to content

Pager makeLinks param fix#2723

Merged
MGatner merged 2 commits intocodeigniter4:developfrom
TysiacSzescset:PagerFix
Mar 24, 2020
Merged

Pager makeLinks param fix#2723
MGatner merged 2 commits intocodeigniter4:developfrom
TysiacSzescset:PagerFix

Conversation

@TysiacSzescset
Copy link
Copy Markdown
Contributor

@TysiacSzescset TysiacSzescset commented Mar 19, 2020

Description
If $group=null then group name and query string param name is made of timestamp (change every call) making it useless. Setting default $group to "default" string literal solves the problem and generates default "page" query param name.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 22, 2020

Could you please add a test case to tests/system/Pager/PagerTest.php to demonstrate the issue that this fixes? See testMakeLinks() for likely examples.

@TysiacSzescset
Copy link
Copy Markdown
Contributor Author

TysiacSzescset commented Mar 23, 2020

I'v modified makeLinks() a little more.
Timestamp assign is not needed anymore, either $group checks in $this->store() and $this->displayLinks() calls.
Now makeLinks() generates 2 types of results based on two possible types of groups:

  • default group (no group or '' empty group) -> page query parameter
  • custom group -> page_groupName query parameter.

Also added some tests and user guide update.

@TysiacSzescset
Copy link
Copy Markdown
Contributor Author

Ok, after some struggle with git, i hope it's my final commit for this PR :)

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 23, 2020

i hope it's my final commit for this PR

I really hope those CI test errors are related to your PR or we have bigger issues!

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 23, 2020

Actually it looks like a bunch of files had their mode changed. Sorry to do this to you, but let's back out the changes and reapply just what you need. Looks like that will fix the CI issues because your branch is missing a .env file. There's a lot of ways to do that, but you might try something like this:

  1. Backup the following files outside your repo:
  • system/Pager/Pager.php
  • tests/system/Pager/PagerTest.php
  • user_guide_src/source/libraries/pagination.rst
  1. Reset your branch to before the issue: git reset c307fec459eb6b70a0bab2dd77b1c82c5c8a2e28
  2. Your modified files should still all be in place, so re-apply them:
  • git add system/Pager/Pager.php
  • git add tests/system/Pager/PagerTest.php
  • git add user_guide_src/source/libraries/pagination.rst
  1. Make a new commit: git commit -m "Pager fix, Pager test added, Guide update"
  2. Force origin to the new state: git push -f

I have a backup of your repo state local in case anything goes wrong. Good luck! :)

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 24, 2020

Much better, thank you!

@MGatner MGatner merged commit 29310e2 into codeigniter4:develop Mar 24, 2020
@TysiacSzescset TysiacSzescset deleted the PagerFix branch March 24, 2020 13:54
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.

2 participants