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

[4.4] Check pagination parameters from request, another try #44023

Merged
merged 7 commits into from
Sep 23, 2024

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Sep 6, 2024

Summary of Changes

Existing approach still have a problem, it is always forcing the parameters defined in $defaultUrlParams, and make it impossible to remove.

I suggest a different approach, take the allowed parameters from the router, and filtering them before adding. Also this now happen in the pagination constructor, which allows for developers to unset unneeded params.

Testing Instructions

Code review. Test with your component.

Or, add following code, somewhere here

<?php
$pg = new \Joomla\CMS\Pagination\Pagination(100, 0, 20);
$pg->setAdditionalUrlParam('tmpl', null);
echo $pg->getPaginationLinks();
?>

Then open any page with ?tmpl=index in URL, and check pagination links.

Actual result BEFORE applying this Pull Request

Somehow works.

Pagination links contain ?tmpl=index .

Expected result AFTER applying this Pull Request

Works but better.

Pagination links without ?tmpl=index .

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org:
  • No documentation changes for manual.joomla.org needed

@SniperSister
Copy link
Contributor

This PR re-introduces the issue that was fixed in the security release.

@Fedik Fedik marked this pull request as draft September 7, 2024 17:29
@Fedik Fedik marked this pull request as ready for review September 8, 2024 08:24
@Fedik
Copy link
Member Author

Fedik commented Sep 8, 2024

I updated the PR, it now takes only allowed parameters.

@PhocaCz
Copy link
Contributor

PhocaCz commented Sep 10, 2024

Testing now, after getting more information, it seems to be working fine for me. I will do more tests to confirm this PR. Thank you.

@PhocaCz
Copy link
Contributor

PhocaCz commented Sep 12, 2024

I have tested this item ✅ successfully on 097ef2e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44023.

@n3t

This comment was marked as outdated.

@n3t

This comment was marked as outdated.

@Fedik
Copy link
Member Author

Fedik commented Sep 13, 2024

@n3t your test is unrelated to current PR.
For fixing start=0 there need a separated fix. Please test #44069

@Fedik Fedik changed the title Check pagination parameters from request, another try [4.4] Check pagination parameters from request, another try Sep 13, 2024
@n3t
Copy link
Contributor

n3t commented Sep 15, 2024

I have tested this item ✅ successfully on 399ab15

In combination with patch #44069 this works as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44023.

@Quy Quy changed the title [4.4] Check pagination parameters from request, another try [4.4] Check pagination parameters from request, another try Sep 15, 2024
@Quy
Copy link
Contributor

Quy commented Sep 15, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44023.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 15, 2024
@MacJoom MacJoom added this to the Joomla! 4.4.9 milestone Sep 23, 2024
@MacJoom MacJoom merged commit d4a1f41 into joomla:4.4-dev Sep 23, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 23, 2024
@MacJoom
Copy link
Contributor

MacJoom commented Sep 23, 2024

Thank you!

@Fedik Fedik deleted the pagination-fix-another branch September 23, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants