#2877 closed defect (bug) (fixed)
A Slash too much @ get_pagenum_link()
Reported by: | Owned by: | costdev | |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 2.0 |
Component: | Posts, Post Types | Keywords: | has-patch has-unit-tests has-testing-info commit |
Focuses: | template | Cc: |
Description
- File: template-functions-links.php
- Function: get_pagenum_link()
The following code is at the end of the function:
// showing /page/1/ or ?paged=1 is redundant if ( 1 === $pagenum ) { $qstr = str_replace('page/1/', '', $qstr); // for mod_rewrite style $qstr = remove_query_arg('paged', $qstr); // for query style }
If you work with the mod_rewrite style, you will get this URL for the first page:
http://www.example.com/wordpress/index.php/
This wont work (/ at the end).
Correx:
// showing /page/1/ or ?paged=1 is redundant if ( 1 === $pagenum ) { $qstr = str_replace('/page/1/', '', $qstr); // for mod_rewrite style $qstr = remove_query_arg('paged', $qstr); // for query style }
Notes:
- sry for my bad english...
- i've downloaded a german translation of wordpress 2.0.3, so maybe its just in this version... ?!
Attachments (6)
Change History (50)
#2
@
17 years ago
- Resolution set to wontfix
- Status changed from new to closed
No traction in almost a year, so closing as wontfix.
Feel free to re-open it if you have additional information/suggestions/patches/...
#3
@
12 years ago
- Keywords needs-patch added
- Milestone set to Future Release
- Resolution wontfix deleted
- Status changed from closed to reopened
Only enhancements or feature requests can be closed when there's no traction.
#5
@
12 years ago
Note that the problem is with first-page links, not with /page/2, /page/3/ etc.
Original thread: http://wordpress.org/support/topic/plugin-wp-pagenavi-trailing-slash-at-first-page-url-regardless-of-defined-permalink-structure
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
14 months ago
#13
@
14 months ago
- Keywords needs-refresh added
- Owner set to petitphp
- Status changed from reopened to assigned
We discussed this issue during this week Old Tickets Triage Session.
The patch needs to be refreshed. Assigning @petitphp who's willing to add a new patch/unit tests.
Thanks to @SergeyBiryukov, @maigret, @petitphp for discussing this issue :)
This ticket was mentioned in PR #3931 on WordPress/wordpress-develop by @petitphp.
14 months ago
#14
- Keywords has-unit-tests added; needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/2877
#15
@
14 months ago
PR with refreshed patch added.
I've made a change from the original patch to ensure query_args don't force a trailing slash, and I updated the tests accordingly.
I've tested multiple cases :
- single site install with permalink structure
/%category%/%postname%/
(trailing slash)- link to first page : https://example.com/category/nam-voluptas/
- link to second page : https://example.com/category/nam-voluptas/page/2/
- single site install with permalink structure
/%category%/%postname%
(no trailing slash)- link to first page : https://example.com/category/officiis-incidunt
- link to second page : https://example.com/category/officiis-incidunt/page/2
- multisite subdirectory install with permalink structure
/%category%/%postname%
- link to first page : https://example.com/site-one/category/quia-voluptatem
- link to second page : https://example.com/site-one/category/quia-voluptatem/page/2
- multisite subdomain install with permalink structure
/%category%/%postname%
- link to first page : https://site-one.example.com/category/quasi
- link to second page : https://site-one.example.com/category/quasi/page/2
In every case, get_pagenum_link
correctly matched the permalink structure and only added a trailing slash if present.
This should be ready for testing on more exotic WordPress install.
#16
@
13 months ago
- Milestone changed from Future Release to 6.3
Thanks for the PR @petitphp!
Moving for 6.3 consideration.
This ticket was mentioned in Slack in #core by oglekler. View the logs.
10 months ago
#18
follow-up:
↓ 23
@
10 months ago
- Keywords has-testing-info added
This ticket was discussed in the recent bug scrub.
The PR looks good and requires manual testing. Could someone from the testing team, such as @ironprogrammer or @webtechpooja, please review it?
Additional props to @oglekler
#19
@
9 months ago
Unit tests need documentation to methods to clarify what will be tested, the purpose of this tests.
Besides, it still needs to be tested.
This ticket was mentioned in PR #4801 on WordPress/wordpress-develop by @costdev.
9 months ago
#21
Previously, a trailing slash was appended to the link returned from this function. If the permalink structure didn't contain a trailing slash, this link could fail.
This change removes trailing slashes and only appends one if the site is set for adding trailing slashes.
---
This PR is an update of #3931 which makes various test improvements, including:
- Documentation updates.
- Backing up some
$_SERVER
variables before any tests run, and restoring them after each test has run. - Using a data provider to group related datasets.
- Adding datasets that account for
index.php
in the permalink structure.
#22
@
9 months ago
- Keywords changes-requested removed
@oglekler PR 4801 updates the previous PR with the requested documentation updates, as well as other improvements to the tests which are listed in the PR's description. The tests now cover 100% of the lines and branches touched by the patch.
#23
in reply to:
↑ 18
@
8 months ago
Replying to mukesh27:
This ticket was discussed in the recent bug scrub.
The PR looks good and requires manual testing. Could someone from the testing team, such as @ironprogrammer or @webtechpooja, please review it?
Additional props to @oglekler
@mukesh27 Thanks for your comment. May I ask what is the exact reproducible steps here from the user's perspective?
#24
@
8 months ago
I tried to reproduced the issue as described in the ticket. But couldn't make it.
The above mentioned links worked for me without using patch, so not sure about this patch.
Could you please guide me the steps to reproduce this issue. Thanks
#25
@
8 months ago
Moving for 6.4 consideration as this still requires testing.
(@costdev feel free to ping me if you're confident this can ship before RC1)
#27
@
7 months ago
Hey @webtechpooja
I tried to reproduced the issue as described in the ticket. But couldn't make it.
The above mentioned links worked for me without using patch, so not sure about this patch.
Sorry, my first report wasn't clear on what and how to test this.
The original issue is that the get_pagenum_link
function always add a slash at the end of the link to the first page of a post type or taxonomy archive even if the permalink structure doesn't contain one.
Could you please guide me the steps to reproduce this issue. Thanks
Here the steps to reproduce the issue and the expected result.
Given a WordPress install with TwentyTwentyThree theme :
- Set the permalink structure to
/%postname%
(no trailing slash) - Create multiple posts (enough to have at least two pages)
- On the frontend go to the year archive page (
https://example.com/2023
) - The
Older Posts ->
link should behttps://example.com/2023/page/2
(no trailing slash, work as expected) - Go to the second page (
https://example.com/2023/page/2
) - The
<- Newer Posts
link should behttps://example.com/2023
but the actual link ishttps://example.com/2023/
with a trailing slash (not working as expected).
The patch should fix the issue from the last step and remove the trailing slash of the first page link.
Let me know if you need additional information.
This ticket was mentioned in Slack in #core by oglekler. View the logs.
6 months ago
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
6 months ago
#30
@
6 months ago
Test Report
Environment
- WordPress - 6.3.1
- Chrome Version - Version 117.0.5938.92
- OS - macOS Ventura Version: 13.2.1
- Theme: Twenty Twenty-Three Version: 1.2
- PHP - 8.1.9
- Web Server - nginx
- Database - MySQL 8.0.16
Test Results
I setup a WordPress with twenty twenty-three theme, set permalink without trailing slash and added 20 posts to regenerate the issue and get the Older Post and Newer Post as Pagination.
When I clicked on the Newer Post to Older Post to Newer Post, didn’t get the slash at the end of the URL as mentioned steps to reproduced by @petitphp.
So, the conclusion is the issue is not reproduced.
Video Demonstration:
https://www.awesomescreenshot.com/video/21188869?key=8bf1963ab5a9424aed2e33d84c5955cc
#31
@
6 months ago
Thank you, @petitphp for providing the detailed instructions for reproducing the issue.
Test Report
Environment
- WordPress - 6.4 Beta 1
- Chrome Version - Version 117.0.5938.92
- OS - macOS Ventura Version: 13.0
- Theme: Twenty Twenty-Three Version: 1.2
- PHP - 8.1.9
- Web Server - nginx
- Database - MySQL 8.0.16
Actual Results
I created a test setup using WordPress 6.4 Beta 1 and used the Twenty Twenty-Three theme, Here I followed the exact steps that were mentioned above by @petitphp:
- Set the permalink structure to /%postname% (no trailing slash)
- Created 20 posts
- Then checked on frontend on both pages Older and newer pages.
- I didn’t get the slash at the end of the URL.
Conclusion
❌ The issue is not recreated at my end. So I didn't apply the patch.
#32
@
5 months ago
@petitphp it seems that the testers cannot reproduce the issue. Perhaps we can close this ticket. Could you take a look?
#33
@
5 months ago
@nicolefurlan thanks for the ping, I didn't had time to look back at this ticket after the new test report, will do this shortly.
#34
@
5 months ago
@tejwanihemant, @webtechpooja, thanks for the test report.
The reason you didn't get the trailing slash in the URL in your testing is because WordPress has redirected you to the correct URL.
This is the purpose of this patch,
Looking at the network tab in the browser devtools you should see a redirect without the patch and no redirect with the patch.
Before, going back to the first page trigger a redirect
200 : localhost/2023 <-- on the first page 200 : localhost/2023/2 <-- going to the second page 301 : localhost/2023/ <-- going back the first page 200 : localhost/2023 <-- user is redirected to the correct URL without trailing slash
After, going back to the first page directly
200 : localhost/2023 <-- on the first page 200 : localhost/2023/2 <-- going to the second page 200 : localhost/2023 <-- going back the first page (no redirect)
See the attached screenshot for an example.
@nicolefurlan I can still reproduce the issue on trunk, and patch fix the issue.
@
5 months ago
Screenshot of the network inspector in Firefox showing the issue with trailing slashed before and after applying the proposed patch.
@
5 months ago
Screenshot of the network inspector showing the issue with trailing slashed before applying the patch.
@
5 months ago
Screenshot of the network inspector showing the issue resolved after applying the patch.
#35
@
5 months ago
Thank you @petitphp for the detailed description regarding testing. Now I am able to reproduce the issue. I have also added screenshot of the issue, before applying the patch and after applying the patch. This patch is perfectly working fine with the issue. And here is the updated test report,
Test Report
This report validates that the indicated patch addresses the issue.
Patch tested: https://github.com/WordPress/wordpress-develop/pull/4801
Environment
- OS: macOS 14.0
- Web Server: nginx/1.23.3
- PHP: 7.4.33
- WordPress: 6.3-beta3-56130-src
- Browser: Chrome 117.0.5938.149
- Theme: Twenty Twenty-Three
Actual Results
- ❌ Before Patch
- ✅ After Patch
#38
@
5 months ago
- Keywords commit added
Adding commit
! Of course, if committers don't feel this is ready, please feel free to remove that keyword and we can look at this ticket closer.
#39
@
5 months ago
- Keywords needs-testing removed
Thank you everyone for your test reports :)
@costdev the PR/patch has multiple test reports showing it does indeed resolve the issue. Is it ready for commit? If yes, then:
- Are you planning to commit it ahead of a tentatively scheduled 6.4 Beta 5 (tomorrow Oct 16th) or RC1 (the next day Oct 17th)?
- Or if you don't have time, another committer (maybe me) can pick it up?
#40
@
5 months ago
- Owner changed from petitphp to costdev
- Status changed from assigned to reviewing
@hellofromTonya Are you planning to commit it ahead of a tentatively scheduled 6.4 Beta 5 (tomorrow Oct 16th) or RC1 (the next day Oct 17th)?
Yep! Self-assigning for final review and commit.
This ticket was mentioned in Slack in #core-committers by costdev. View the logs.
5 months ago
5 months ago
#44
Closed in favour of https://github.com/WordPress/wordpress-develop/pull/4801.
No, it's that way in the english version. I don't think that would fix it and there would be a problem with that. Putting the slash in front like you did would make the server redirect to root. So if you have wordpress in a directory it would screw up like so:
You would want this:
http://site.com/wordpress/testing/1/
But you would get this:
http://site.com/testing/1/