Make WordPress Core

Opened 2 years ago

Closed 7 weeks ago

#61393 closed defect (bug) (fixed)

Trailing Slash Issue in Category Pagination URLs with the_posts_pagination()

Reported by: hmbashar's profile hmbashar Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Permalinks Keywords: has-screenshots has-test-info has-patch has-unit-tests commit
Focuses: Cc:

Description

Hello,

I've checked a few themes that use the the_posts_pagination() function for pagination, and I think this function has a small issue. Although it’s not a major problem, it should still be addressed. If I remove the extra/trailing slash (/) at the end of the URL from the permalink structure (like /%%postname%% instead of /%%postname%%/), it works perfectly for all category pages except the first page.

For example, if I have a category slug "uncategorized", the pagination for this category archive page works as follows:

However, if I hover over any other number in the pagination, it shows without the trailing slash (/). Check this screenshot: https://prnt.sc/GICqVaCdAD8e. But if I hover over page number 1, it shows with a trailing slash, like this: https://wpdevelop.test/category/uncategorized/. Check this screenshot: https://prnt.sc/n-eNn-P_Zr5x. After clicking on page number 1, it redirects to the URL without the slash: https://wpdevelop.test/category/uncategorized.

So, I think if it redirects to remove the slash, the URL should also remove the slash when hovering over page number 1.

I've checked several themes with this behavior:

  • Twenty Fifteen Version: 3.7
  • Twenty Fourteen Version: 3.9
  • Twenty Nineteen Version: 2.8
  • Twenty Seventeen Version: 3.6
  • Twenty Sixteen Version: 3.2
  • Twenty Twenty Version: 2.6
  • Twenty Twenty-One Version: 2.2
  • Twenty Twenty-Two Version: 1.7
  • Astra Version: 4.7.0

How to Check the Issue:

  1. Activate any of the themes mentioned above.
  2. Add categories.
  3. Add a few posts with those categories to enable pagination.
  4. Set permalinks with a custom structure (/%%postname%%), ensuring you don’t include a slash at the end.
  5. Visit your category archive, e.g., https://wpdevelop.test/category/uncategorized.
  6. Click pagination numbers and check the URLs.

Check the screenshots for reference:

Thank you!

Attachments (11)

Permalink structure set.png (186.6 KB) - added by hmbashar 2 years ago.
others pagination view.png (148.2 KB) - added by hmbashar 2 years ago.
First Page Pagination view.png (116.9 KB) - added by hmbashar 2 years ago.
Trailing Slash Issue in Category Pagination URLs with the_posts_pagination().mp4 (9.7 MB) - added by hmbashar 2 years ago.
I've added a video you may watch this from here for you can download from the attachment https://www.berrycast.com/conversations/88000375-7f49-574e-9ba0-e338ffc63702
61393.diff (645 bytes) - added by rahulsprajapati 23 months ago.
Use user_trailingslashit to fix trailing slash issue in pagination permalink.
61393-non-trailslash-permalink.gif (2.5 MB) - added by rahulsprajapati 23 months ago.
Test GIF: When removed trailingslash from permalink structure it uses all link without trailslash.
61393-with-trailslash-permalink.gif (1.5 MB) - added by rahulsprajapati 23 months ago.
Test GIF: When using trailingslash in permalink structure it uses all link with trailslash.
Huzaifa-20240613230120.gif (8.3 MB) - added by huzaifaalmesbah 23 months ago.
I test 61393.diff Working and looks good for me.
Screenshot at Jun 13 11-47-40 PM.png (161.7 KB) - added by hmbashar 23 months ago.
I've tested the patch 61393.diff, and it's working for me. The issue has been resolved.
61393-1.diff (1.2 KB) - added by rahulsprajapati 23 months ago.
Use user_trailingslashit to fix trailing slash issue in pagination permalink including "Newest Post" and "Older Post" links.
Screenshot at Jun 14 12-28-39 AM.png (189.1 KB) - added by hmbashar 23 months ago.
Perfect with the patch "61393-1.diff, I think now everything's is fine.

Change History (58)

#1 @huzaifaalmesbah
2 years ago

Thank you @hmbashar for pointing out this valid issue. I have been able to reproduce it.

In my tests with the default WordPress theme, as well as popular themes like Astra, GeneratePress, and Neve, I observed that they all encounter the same trailing slash issue when using the the_posts_pagination() function.


This ticket was mentioned in Slack in #accessibility by hmbashar. View the logs.


23 months ago

This ticket was mentioned in Slack in #core-test by mai21. View the logs.


23 months ago

#5 @mai21
23 months ago

@hmbashar thanks for reporting the issue.

Reproduction Report

Description

This report validates that the issue can be reproduced.

Steps

As mentioned on the main ticket

Environment

  • WordPress: 6.6-beta2-58396
  • PHP: 8.3.6
  • Server: nginx/1.24.0
  • Database: mysqli (Server: 10.5.24-MariaDB-1:10.5.24+maria~ubu2004-log / Client: mysqlnd 8.3.6)
  • Browser: Chrome 125.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty 2.6
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0
    • WordPress Beta Tester 3.5.5

Actual Results

🐞 Error condition occurs. Hovering the 1st-page link will have /, however, when clicked it opens the link without / (reproduced).

Expected Result

✅ Hovering any page (1st or nth) link shall match permalinks either it was with/without trailing slash

@rahulsprajapati
23 months ago

Use user_trailingslashit to fix trailing slash issue in pagination permalink.

@rahulsprajapati
23 months ago

Test GIF: When removed trailingslash from permalink structure it uses all link without trailslash.

@rahulsprajapati
23 months ago

Test GIF: When using trailingslash in permalink structure it uses all link with trailslash.

#6 @rahulsprajapati
23 months ago

  • Keywords has-patch added; needs-patch removed

Added patch & gifs for test permalink with and without trailingslash in pagination links.

@huzaifaalmesbah
23 months ago

I test 61393.diff Working and looks good for me.

#7 @huzaifaalmesbah
23 months ago

  • Keywords has-testing-info needs-testing added

according to the ticket status, I added two keywords has-testing-info and needs-testing

@hmbashar
23 months ago

I've tested the patch 61393.diff, and it's working for me. The issue has been resolved.

#8 @hmbashar
23 months ago

  • Keywords changes-requested added

Hello @rahulsprajapati,

I've tested your patch, and while it is working fine, I have noticed another issue. Perhaps your patch could address this as well. I checked several themes like Astra, Twenty Fifteen, Twenty Fourteen, and more. I observed that themes with the "Newest Post" and "Older Post" navigation feature have a trailing slash for the newest posts (see screenshot). However, other themes with the same features, such as Twenty Eleven and Twenty Ten, do not show a trailing slash with the newest posts (see screenshots).

Twenty Fourteen: https://prnt.sc/NrC_xTz4b63X
Astra: https://prnt.sc/i2H72Q6XLBjc
Twenty Fifteen: https://prnt.sc/bawBT2J-IhM7

Twenty Ten: https://prnt.sc/lTizkhfzP-Uz
Twenty Eleven: https://prnt.sc/iv169bWJkQSz

@rahulsprajapati
23 months ago

Use user_trailingslashit to fix trailing slash issue in pagination permalink including "Newest Post" and "Older Post" links.

#9 @rahulsprajapati
23 months ago

Hello @hmbashar,

Thank you for testing this and for the feedback. I just uploaded another patch with the next and prev link fix. Let me know if you see any issue with this.

@hmbashar
23 months ago

Perfect with the patch "61393-1.diff, I think now everything's is fine.

This ticket was mentioned in Slack in #core-test by hmbashar. View the logs.


23 months ago

#11 @martin.krcho
23 months ago

  • Keywords needs-unit-tests added

#12 @joedolson
23 months ago

  • Focuses accessibility removed

I don't see anything about this that impacts accessibility; removing the focus.

This ticket was mentioned in PR #8226 on WordPress/wordpress-develop by @ankitkumarshah.


15 months ago
#13

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: #61393

## Description
Adds a unit test to verify pagination links maintain consistent trailing slash behavior when the permalink structure has trailing slash as well as does not have trailing slash.

## Testing instructions

  1. Run the unit test:
    npm run test:php -- --filter=test_pagination_links_trailing_slash_consistency
    

#14 @ankitkumarshah
15 months ago

Hi @martinkrcho, @rahulsprajapati, @hmbashar, @huzaifaalmesbah,

I have added the unit test and also updated the implementation in my unit test PR to address the failing tests. Please review it at your convenience.

Thank You!

#15 @adamsilverstein
15 months ago

  • Keywords good-first-bug reporter-feedback added; changes-requested removed
  • Milestone changed from Awaiting Review to 6.8

#16 @adamsilverstein
15 months ago

@ankitkumarshah The user_trailingslashit function takes a second parameter that should probably be 'category' here, can you take a look at adding that? It is passed to the filter called in the function.

#17 @ankitkumarshah
15 months ago

Hi @adamsilverstein,

Thank you for pointing this out.
I found that the second parameter for user_trailingslashit( $link ) would be 'paged' as the function we are working with is paginate_links(). Technically, the function can be used to create a paginated link list for any area.

The same is done in line number 4516 in the paginate_links() function:

$format .= $wp_rewrite->using_permalinks() ? user_trailingslashit( $wp_rewrite->pagination_base . '/%#%', 'paged' ) : '?paged=%#%';

Reference - https://github.com/WordPress/wordpress-develop/blob/b8b72e62481d6fd37e5ec012d9af31722bb2a43f/src/wp-includes/general-template.php#L4516

I have updated my PR accordingly

#18 @adamsilverstein
15 months ago

I found that the second parameter for user_trailingslashit( $link ) would be 'paged'

Great, thanks for looking into that. Sounds right.

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


14 months ago

@sourabhjain commented on PR #8226:


14 months ago
#20

QA Report:

Issue is fixed.

Twenty Five
https://github.com/user-attachments/assets/17bee754-489d-4c5a-b9d2-b873d9064150

https://github.com/user-attachments/assets/d58f2a42-5fb3-4833-88a8-e96bb2706e41

Astra

https://github.com/user-attachments/assets/54adb5c9-bc18-40d9-a64c-f8785096c75b

https://github.com/user-attachments/assets/b6155865-425e-43ab-bad5-1a2cc9ed7af9

#21 @audrasjb
14 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for final review and commit.

#22 @audrasjb
14 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59966:

Permalinks: Ensure pagination links are consistent with permalink structure.

This changeset ensures that pagination links stay consistent with the chosen permalink structure. When the permalink structure uses a trailing slash, pagination permalinks contain one as well, but when the permalink structure doesn't use trailing slash, then pagination links should not use a trailing slash.

This makes use of user_trailingslashit() with a paged value for the type_of_url parameter.

Props hmbashar, huzaifaalmesbah, rejaulalomkhan, mai21, rahulsprajapati, martinkrcho, ankitkumarshah, adamsilverstein, sourabhjain.
Fixes #61393.

@ankitkumarshah commented on PR #8226:


14 months ago
#23

Merged in the changeset [59966]

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


13 months ago

#25 @peterwilsoncc
13 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch needs-testing has-unit-tests good-first-bug reporter-feedback removed
  • Milestone changed from 6.8 to 6.9
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this as the commit was reverted in [60088] due to a bug reported in #63123.

I think the change that is required is to modify the default base and format arguments to account for permalink settings.

If the permalinks do not include trailing slashes, the base should be of the form example.com/category/cateogory-slug%_% and the format include a leading slash.

If permalinks do include a trailing slash, the base should be of the form example.com/category/cateogory-slug/%_% and the format not include the trailing slash.

Modifying the defaults in this way will allow developers to pass a custom base and format and have them remain backward compatible.

The unit tests for the functions should use $this->goto() to allow for testing of the defaults.

This ticket was mentioned in PR #8591 on WordPress/wordpress-develop by @peterwilsoncc.


13 months ago
#26

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

#27 @peterwilsoncc
13 months ago

I've created a pull request taking a second pass at this.

Rather than modifying the links as they are generated, it takes in to consideration whether a site's permalinks uses trailing slashes and modifies the default values of $base and $format accordingly.

This will ensure the function doesn't add trailing slashes to links unexpectedly, see #63123, while avoiding the incorrect links and 301 redirects on sites that do not use trailing slashes.

I've included a bunch of tests but further suggestions are welcome.

#28 @wordpressdotorg
12 months ago

  • Keywords has-test-info added; has-testing-info removed

This ticket was mentioned in Slack in #core by welcher. View the logs.


6 months ago

#30 @welcher
6 months ago

  • Keywords needs-testing added

This was reviewed in the 6.9 Bug Scrub today. @peterwilsoncc new PR has failing unit tests and need to be reviewed before moving this forward.

This ticket was mentioned in Slack in #core-test by jon_bossenger. View the logs.


6 months ago

#32 @krupajnanda
6 months ago

Pinging @audrasjb and @peterwilsoncc to prioritize reviewing the failing tests on PR. If the failed tests cannot be stabilized and re-tested in the next 1 or 2 days, the safest course of action is to punt this ticket to the 6.9.1 minor release or next major 7.0 release.

Last edited 6 months ago by krupajnanda (previous) (diff)

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


6 months ago

#34 @peterwilsoncc
6 months ago

@welcher @krupajnanda The failing tests were network issues back when I first pushed. I've rebased and pushed and the tests are now passing.

#35 follow-up: @peterwilsoncc
6 months ago

  • Milestone changed from 6.9 to 7.0

I briefly discussed this with @westonruter and we decided to bump to 7.0 as WP tends to put trailing slashes on the permalinks when using the default options on Settings > Permalinks. It's likely low impact.

#36 @ozgursar
4 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8591

Environment

  • WordPress: 7.0-alpha-61215-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 143.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty 3.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.1

Actual Results

  1. ✅ Issue resolved with patch.

Screenshots

Twenty Twenty-Five

Before After
https://i.snipboard.io/S4Em86.jpg https://i.snipboard.io/udJgaT.jpg

Twenty Twenty

Before After
https://i.snipboard.io/Nc3lEd.jpg https://i.snipboard.io/xWncLC.jpg

#37 in reply to: ↑ 35 @peterwilsoncc
4 months ago

@ozgursar thank you for testing!

Replying to peterwilsoncc:

I briefly discussed this with @westonruter and we decided to bump to 7.0 as WP tends to put trailing slashes on the permalinks when using the default options on Settings > Permalinks. It's likely low impact.

@westonruter Presuming the tests pass after the rebase, are you happy for PR#8591 to go in to trunk?

#38 @westonruter
4 months ago

@peterwilsoncc I left some comments on the PR.

#39 @SirLouen
4 months ago

  • Keywords needs-testing removed

This ticket was mentioned in Slack in #core by juanmaguitar. View the logs.


7 weeks ago

@juanmaguitar commented on PR #8591:


7 weeks ago
#41

@peterwilsoncc thanks for working on this ticket!
There's some pending feedback to address. Do you think we can move forward with this PR in time for WP 7.0 RC1?

#42 @juanmaguitar
7 weeks ago

From today's bug scrub

There's some feedback in the related PR #8591 pending to be addressed. Pinged @peterwilsoncc about it in the PR

@peterwilsoncc commented on PR #8591:


7 weeks ago
#43

@westonruter Thank you! I've applied all of your suggestions, rebased and pushed.

@westonruter commented on PR #8591:


7 weeks ago
#44

# Testing

I removed the trailing slash from the permalink structure:

https://github.com/user-attachments/assets/6358e070-1b13-4309-866d-34c741107440

When the Twenty Twenty theme active, I went to the second page of results for the Uncategorized category archive:

<details><summary>Before</summary>

<div class="nav-links">
    <a
            class="prev page-numbers"
            href="http://localhost:8000/category/uncategorized/"
    ><span aria-hidden="true"></span>
        <span class="nav-prev-text"
        >Newer <span class="nav-short">Posts</span></span
        ></a
    >
    <a class="page-numbers" href="http://localhost:8000/category/uncategorized/"
    >1</a
    >
    <span aria-current="page" class="page-numbers current">2</span>
    <a
            class="page-numbers"
            href="http://localhost:8000/category/uncategorized/page/3"
    >3</a
    >
    <span class="page-numbers dots"></span>
    <a
            class="page-numbers"
            href="http://localhost:8000/category/uncategorized/page/7"
    >7</a
    >
    <a
            class="next page-numbers"
            href="http://localhost:8000/category/uncategorized/page/3"
    ><span class="nav-next-text"
    >Older <span class="nav-short">Posts</span></span
    >
        <span aria-hidden="true"></span></a
    >
</div>

</details>
<details><summary>After</summary>

<div class="nav-links">
    <a
            class="prev page-numbers"
            href="http://localhost:8000/category/uncategorized"
    ><span aria-hidden="true"></span>
        <span class="nav-prev-text"
        >Newer <span class="nav-short">Posts</span></span
        ></a
    >
    <a class="page-numbers" href="http://localhost:8000/category/uncategorized"
    >1</a
    >
    <span aria-current="page" class="page-numbers current">2</span>
    <a
            class="page-numbers"
            href="http://localhost:8000/category/uncategorized/page/3"
    >3</a
    >
    <span class="page-numbers dots"></span>
    <a
            class="page-numbers"
            href="http://localhost:8000/category/uncategorized/page/7"
    >7</a
    >
    <a
            class="next page-numbers"
            href="http://localhost:8000/category/uncategorized/page/3"
    ><span class="nav-next-text"
    >Older <span class="nav-short">Posts</span></span
    >
        <span aria-hidden="true"></span></a
    >
</div>

</details>

  • .html

    old new  
    11<div class="nav-links">
    22    <a
    33            class="prev page-numbers"
    4             href="http://localhost:8000/category/uncategorized/"
     4            href="http://localhost:8000/category/uncategorized"
    55    ><span aria-hidden="true">←</span>
    66        <span class="nav-prev-text"
    77        >Newer <span class="nav-short">Posts</span></span
    88        ></a
    99    >
    10     <a class="page-numbers" href="http://localhost:8000/category/uncategorized/"
     10    <a class="page-numbers" href="http://localhost:8000/category/uncategorized"
    1111    >1</a
    1212    >
    1313    <span aria-current="page" class="page-numbers current">2</span>

Clicking on previous page before (👎 has redirect):

https://github.com/user-attachments/assets/44606de6-93f3-4b3f-99ba-f2a39d14028c

Clicking previous page after (👍 no redirect):

https://github.com/user-attachments/assets/101a84ec-f285-4c31-b3ff-fe80ed68fa93

#45 follow-up: @audrasjb
7 weeks ago

  • Owner audrasjb deleted
  • Status changed from reopened to assigned

Removing myself from ticket ownership because I have a bit too much on my plate for 7.0. @peterwilsoncc @westonruter you've been really active on this ticket, do you think we can ship it before RC1 on Thursday?

#46 in reply to: ↑ 45 @peterwilsoncc
7 weeks ago

  • Owner set to peterwilsoncc
  • Status changed from assigned to accepted

Replying to audrasjb:

... do you think we can ship it before RC1 on Thursday?

I think we can, I pushed some minor test changes yesterday that are passing so I think it's good for committing but I'll get Weston to do a quick logic check before doing so.

#47 @westonruter
7 weeks ago

  • Keywords commit added

Good to go.

#48 @peterwilsoncc
7 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 62036:

Permalinks: Follow rewrite rules for trailing slashes in paginate_links().

Modifies the default settings of paginate_links() to only include trailing slashes, eg example.org/page/2/, when the permalink settings include a trailing slash. When the permalink settings do not include a slash, neither do the generated links, eg example.org/page/2.

This prevents the generated links from hitting a URL that subsequently redirects when the permalink structure does not include a trailing slash.

Props adamsilverstein, ankitkumarshah, audrasjb, hmbashar, huzaifaalmesbah, joedolson, juanmaguitar, krupajnanda, mai21, ozgursar, peterwilsoncc, rahulsprajapati, sirlouen, welcher, westonruter.
Fixes #61393.

Note: See TracTickets for help on using tickets.