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.x] Fix paginated page links for com_finder and com_content archive #43953

Merged
merged 11 commits into from
Aug 22, 2024

Conversation

SniperSister
Copy link
Contributor

@SniperSister SniperSister commented Aug 20, 2024

Summary of Changes

The 4.4.7 and 5.1.3 security release broke the pagination in com_finder (thanks Phil for reporting) and the com_content archive view.

Testing Instructions

Create a test site with a sufficient amount of articles to get a paginated result for the com_finder search results. Click on the pagination links.

Furthermore create an archive view with a sufficient amount of articles that filters for a specific category.

Actual result BEFORE applying this Pull Request

  • finder: Search parameters and search word are lost.
  • archive: catid parameter is lost

Expected result AFTER applying this Pull Request

  • finder: Search parameters and search word are appended to the URL.
  • archive: catid parameter is appended

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

@richard67 richard67 added the bug label Aug 20, 2024
@richard67
Copy link
Member

I have tested this item ✅ successfully on c6b8a24


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

@richard67
Copy link
Member

Hint for other testers: The issue can be easily reproduced and the fix be tested when installing Blog sample data on a clean, current 4.4.7, adding a finder menu item and in the advanced settings for that menu item set the items per page to 5. Then call the page in frontend and search for "Joomla". Then check the links of the 2nd, 3rd, next and last page in the pagination.

@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on c6b8a24

Patch Works also in Joomla 5.1.3


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

Co-authored-by: Quy <quy@nomonkeybiz.com>
@richard67
Copy link
Member

I've restored the previous human test results in the issue tracker as the change which invalidated the test count was just a code style change.

=> RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 20, 2024
@dautrich
Copy link

I have tested this item ✅ successfully on c9aaf5b

Tested on 5.1.3


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

@peteruoi
Copy link

Is there gonna be an immediate new release and wait for it or its gonna be days?

@bembelimen
Copy link
Contributor

No release today

@dautrich
Copy link

@peteruoi
You shouldn't wait with your update to J5.1.3, because the update is a security update. If necessary, you can apply the patch from this PR manually. It is an easy task, because only one file is involved; you only need to add some lines of code in one place.

@SniperSister SniperSister changed the title [4.x] Fix paginated page links for com_finder [4.x] Fix paginated page links for com_finder and com_content archive Aug 21, 2024
@SniperSister
Copy link
Contributor Author

@dautrich @richard67 @coolcat-creations I updated the PR to also cover a second bug in the com_content archive view, a test for that scenario would be much appreciated!

@coolcat-creations
Copy link
Contributor

@SniperSister Test not successful ... See here with applied patch: https://playground.designedwithlove.de/de/archiv?month=0&year=2024&catid[0]=&start=5 and search for "About" - the title filter is reset on pagination change

@SniperSister
Copy link
Contributor Author

@coolcat-creations excellent catch! Fixed!

@coolcat-creations
Copy link
Contributor

@SniperSister what did you fix before because I could not catch an error before the fix there? :-)

@SniperSister
Copy link
Contributor Author

@coolcat-creations i fixed the title filter that you mentioned

@coolcat-creations
Copy link
Contributor

Ok thanks

@coolcat-creations
Copy link
Contributor

Test successful

@jjnxpct
Copy link

jjnxpct commented Aug 21, 2024

@peteruoi You shouldn't wait with your update to J5.1.3, because the update is a security update. If necessary, you can apply the patch from this PR manually. It is an easy task, because only one file is involved; you only need to add some lines of code in one place.

If you have many sites to manage (100+) manually fixing stuff will take a lot more time. So an update / fix through the Joomla updater would be very much appreciated.

@richard67
Copy link
Member

Back to pending as there were made changes.


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

@richard67
Copy link
Member

@peteruoi You shouldn't wait with your update to J5.1.3, because the update is a security update. If necessary, you can apply the patch from this PR manually. It is an easy task, because only one file is involved; you only need to add some lines of code in one place.

If you have many sites to manage (100+) manually fixing stuff will take a lot more time. So an update / fix through the Joomla updater would be very much appreciated.

@jjnxpct If your sites are all on 5.1.3, you have to do that only one time, and for the others you can just copy the 2 modified files. For sites which are on 4.4.7 you can directly use the modified files from this PR

@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on 8245571


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

@JohnVesey
Copy link

Notes from a user:

  1. Fix works on J5.1.3 on PHP 8.2.15
  2. I needed to re-index Smart Search in /administrator as blank results were given when searching on the frontend.

Thank you for this fix 👍

@hans2103
Copy link
Contributor

I have tested this item ✅ successfully on 8245571


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

@richard67 richard67 added the RTC This Pull Request is Ready To Commit label Aug 21, 2024
@richard67
Copy link
Member

RTC


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

@MacJoom MacJoom removed the RTC This Pull Request is Ready To Commit label Aug 21, 2024
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 22, 2024
@SniperSister
Copy link
Contributor Author

I've merged the aditional Parameters that @Fedik suggested in #43954 to get one combined PR fixing the regressions

@richard67
Copy link
Member

Back to pending as there were made changes.


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

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 22, 2024
@richard67
Copy link
Member

@coolcat-creations @dautrich @hans2103 Could you test again? Thanks in advance.

@hans2103
Copy link
Contributor

I have tested this item ✅ successfully on 20d41c3


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

1 similar comment
@dautrich
Copy link

I have tested this item ✅ successfully on 20d41c3


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

@dautrich
Copy link

Hopefully, this is the last time this PR goes RTC.

@alikon
Copy link
Contributor

alikon commented Aug 22, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 22, 2024
@MacJoom MacJoom self-assigned this Aug 22, 2024
@MacJoom MacJoom added this to the Joomla! 4.4.8 milestone Aug 22, 2024
@MacJoom MacJoom merged commit 2d707c0 into joomla:4.4-dev Aug 22, 2024
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 22, 2024
@MacJoom
Copy link
Contributor

MacJoom commented Aug 22, 2024

Thank you for the quick fix!

@fontanil
Copy link

I have tested this item ✅ successfully on 20d41c3

Tested successfully on a 5.1.4-RC-1 version


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

@sakiss
Copy link
Contributor

sakiss commented Aug 23, 2024

@SniperSister What about the "o" and "od" variables?
They stand for ordering and ordering direction.
Enable the "Show Sort Fields" setting in the Smart Search menu item (under Advanced).
Then use the ordering fields to change the order, in a result set that has several pages and try to change page.
The ordering field and direction are vanished.

@SniperSister
Copy link
Contributor Author

@sakiss good catch, see #43967

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.