Make WordPress Core

Opened 18 years ago

Closed 9 months ago

Last modified 9 months ago

#2877 closed defect (bug) (fixed)

A Slash too much @ get_pagenum_link()

Reported by: davemad-davenet's profile dave@… Owned by: costdev's profile 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)

2877.diff (1.4 KB) - added by scribu 13 years ago.
2877.2.diff (1.4 KB) - added by jesin 9 years ago.
Refresh 2877.diff
2877-test.diff (891 bytes) - added by jesin 9 years ago.
ticket_2877_before_after_patch.png (139.0 KB) - added by petitphp 9 months ago.
Screenshot of the network inspector in Firefox showing the issue with trailing slashed before and after applying the proposed patch.
Before-patch.png (42.4 KB) - added by tejwanihemant 9 months ago.
Screenshot of the network inspector showing the issue with trailing slashed before applying the patch.
After-patch.png (30.2 KB) - added by tejwanihemant 9 months ago.
Screenshot of the network inspector showing the issue resolved after applying the patch.

Download all attachments as: .zip

Change History (50)

#1 @darkfate
18 years ago

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/

#2 @Nazgul
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 @scribu
13 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.

@scribu
13 years ago

#4 @scribu
13 years ago

  • Keywords has-patch added; needs-patch removed

#5 @scribu
13 years ago

Note that the problem is with first-page links, not with /page/2, /page/3/ etc.

Version 0, edited 13 years ago by scribu (next)

#6 @nacin
10 years ago

  • Component changed from General to Template

Interesting patch.

#7 @nacin
10 years ago

  • Keywords needs-unit-tests added

#8 @nacin
10 years ago

  • Component changed from Template to Posts, Post Types
  • Focuses template added

#9 @obenland
10 years ago

  • Keywords needs-refresh added

#10 @chriscct7
9 years ago

  • Version changed from 2.0.3 to 2.0

@jesin
9 years ago

Refresh 2877.diff

@jesin
9 years ago

#11 @mattheweppelsheimer
6 years ago

  • Keywords needs-testing added; needs-unit-tests needs-refresh removed

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


18 months ago

#13 @audrasjb
18 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.


18 months ago
#14

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

#15 @petitphp
18 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 :

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 @audrasjb
17 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.


14 months ago

#18 follow-up: @mukesh27
14 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 @oglekler
13 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.

#20 @oglekler
13 months ago

  • Keywords changes-requested added

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


13 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 @costdev
13 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.

Last edited 13 months ago by costdev (previous) (diff)

#23 in reply to: ↑ 18 @mai21
12 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 @webtechpooja
12 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 @audrasjb
12 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)

#26 @audrasjb
12 months ago

  • Milestone changed from 6.3 to 6.4

#27 @petitphp
11 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 be https://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 be https://example.com/2023 but the actual link is https://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.


10 months ago

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


10 months ago

#30 @tejwanihemant
10 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

Last edited 10 months ago by tejwanihemant (previous) (diff)

#31 @webtechpooja
10 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 @nicolefurlan
9 months ago

@petitphp it seems that the testers cannot reproduce the issue. Perhaps we can close this ticket. Could you take a look?

#33 @petitphp
9 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 @petitphp
9 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.

@petitphp
9 months ago

Screenshot of the network inspector in Firefox showing the issue with trailing slashed before and after applying the proposed patch.

@tejwanihemant
9 months ago

Screenshot of the network inspector showing the issue with trailing slashed before applying the patch.

@tejwanihemant
9 months ago

Screenshot of the network inspector showing the issue resolved after applying the patch.

#35 @tejwanihemant
9 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

https://core.trac.wordpress.org/attachment/ticket/2877/Before-patch.png

  • ✅ After Patch

https://core.trac.wordpress.org/attachment/ticket/2877/After-patch.png

#36 @nicolefurlan
9 months ago

Looks like we have a passing test report! Does this seem ready for commit?

#37 @petitphp
9 months ago

@nicolefurlan Yes, I think we're good if committers are ok.

#38 @nicolefurlan
9 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 @hellofromTonya
9 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 @costdev
9 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.


9 months ago

#42 @costdev
9 months ago

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

In 56939:

Posts, Post Types: Don't force trailing slash in get_pagenum_link().

Previously, a trailing slash was appended to the link returned from get_pagenum_link(). 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 adds a new test file for the accompanying tests, tests/phpunit/tests/link/getPagenumLink.php, and moves an existing test for get_pagenum_link() to the same file.

Props davemad-davenet, darkfate, Nazgul, scribu, nacin, obenland, chriscct7, jesin, matthewppelsheimer, audrasjb, petitphp, mukesh27, oglekler, mai21, webtechpooja, tejwanihemant, nicolefurlan, hellofromTonya, costdev.
Fixes #2877.

@costdev commented on PR #4801:


9 months ago
#43

Committed in r56939.

Note: See TracTickets for help on using tickets.