Perform a canonical redirect when paginated states of the front page are not found

[dd32]

As a followup to #45337, [34492], and [47727].

When example.com/ has a static front page assigned and uses <!--nextpage--> accessing non-existing pages uses example.com/page/3/ and Canonical won't kick in.

This is because paged states of the front page use the paged query var, not the page query var.

Should also fix ​https://meta.trac.wordpress.org/ticket/5184 :)

See https://core.trac.wordpress.org/ticket/50163

[SergeyBiryukov]

Thanks for the ticket, I'm working on this as part of #28081 :)

[SergeyBiryukov]

Didn't get a chance to write the tests for this in time for 5.5 RC, moving to 5.6.

[mukesh27]

Adding the refresh keyword for the PR merge conflict.

[audrasjb]

Patch refresh

[audrasjb]

before patch: example.com/page/3 exists

[audrasjb]

After patch: example.com/page/3 redirects to homepage

[audrasjb]

50163.diff refreshes the previous patch. It appears to fix the issue.

[mukesh27]

Patch 50163.diff​ applied fine in trunk version

[SergeyBiryukov]

Thanks for the refresh! This still needs tests though, moving to the next release for now.

[audrasjb]

Just to be sure: do we need unit tests @SergeyBiryukov ?
Because in my testing the suggested patch fixes the issue.

[SergeyBiryukov]

Yes, sorry I was not clear enough :) Any functional changes to the Canonical component should really be accompanied by unit tests, to avoid unexpected regressions like comment:40:ticket:12456.

[audrasjb]

Ok thanks for clarifying. I'll make sure to get those unit tests for the next release as I already saw at least one example of SEO impact of this issue.

[lukecarbis]

Since we're in 5.7 beta 3, let's move this to the next release.

[chaion07]

We reviewed this ticket during a [recent bug-scrub]​https://wordpress.slack.com/archives/C02RQBWTW/p1622579264170500. Pinging @audrasjb for the [unit tests]https://core.trac.wordpress.org/ticket/50163#comment:9. Are these likely to be done before Beta 1 or this should be punted to the Future Releases? Thanks!
Props to @jeffpaul ​https://wordpress.slack.com/archives/C02RQBWTW/p1622579402171400

[hellofromTonya]

Today is 5.8 Beta 1. This one needs tests. Punting to 5.9. But also adding to my tests TODO list. And to ensure I don't forget, @SergeyBiryukov changing ownership to me for the tests.

[hellofromTonya]

With 5.9 Beta 1 tomorrow and this patch touching wpdb, moving to 6.0. I'll work on and finish the unit tests early in the 6.0 cycle to get it committed.

[Mte90]

Any updates for this? maybe for 6.0?

[chaion07]

Thanks @dd32 for reporting this. We reviewed this ticket during a ​recent bug-scrub session. The team is hopeful that @hellofromTonya can take look at her convenience for this ticket to be moving forward during the 6.0 cycle.

Props to @peterwilsoncc

Cheers!

[chaion07]

Updating milestone to Future Release as per discussion during today's bug-scrub session. We can always pull this ticket back if the tests happen to land before the beta 1 which gets into effect on April 12.

Props to @peterwilsoncc & @costdev

Cheers!

[hellofromTonya]

As of today, I'm no longer a sponsored contributor and am starting a month or more AFK break. To not stand in the way, I'm clearing me as the ticket owner, which opens the ticket for someone to step in to shepherd this ticket forward to resolution.

[Mte90]

Any hope for this?
There is a ton of spam on GSC for urls with /page/[number] that doesn't exists, and this issue once resolved should help in some cases.
I see that the PR is waiting a review from @SergeyBiryukov since 4 years on github.

Using the 'paged' var if the singular page is the front page with unit tests

Trac ticket: https://core.trac.wordpress.org/ticket/50163

[rajinsharwar]

Sent a new PR, and also added the unit tests. As this was agreed that the patch would work, and only was missing unit tests, marking this as a candidate for commit.

[hellofromTonya]

This ticket was previously marked as an early candidate. Though it's not touching wpdb, it is touching an area that likely needs a longer soak time to ensure it works properly in Site Editor, front-end, etc. Thinking this one will be better for early commit in 6.7-alpha, which should open in less than 2 weeks.

Moving it to 6.7.

Also removing the commit keyword, as the PR is not yet approved. And now that I'm sponsored again, if @SergeyBiryukov doesn't mind, I'll reassign ownership to myself.

Hey Sergey, please feel free to reassign to yourself and move it back into the milestone if you think it's ready.

[joemcgill]

This looks pretty good at first glance. @hellofromTonya do you still have plans to review this for 6.7?

[peterwilsoncc]

I've approved the ​linked pull request and think it looks good for commit. @hellofromTonya @dd32 a logic check from either of you would be welcome.

[hellofromTonya]

Test Report

Patches tested:

• ​https://github.com/WordPress/wordpress-develop/pull/6421 which includes ​PR 270 (for the source code changes) and unit tests.

Steps to Reproduce or Test

Set up steps:

1. Set the static Home page by:
   • Go to Settings > Reading Settings.
   • Select "A static page (see below)".
   • Set the Homepage to "Sample Page".
   • Click / trigger "Save Changes" button.
2. Add the <!--nextpage--> to the Sample Page by:
   • Go to Pages > Sample Page.
   • Add the "Page Break" block to the content. For example:
     • Click/trigger the "Add Block" + UI below the first quote, type "Page Break", click on the block to add it.
     • Or in the Code Editor version, copy and paste this code below the first wp:quote:

       <!-- wp:nextpage -->
       <!--nextpage-->
       <!-- /wp:nextpage -->
       

     • Click/trigger the Save button.

Testing Instructions (after completing the setup):

1. Go to the root of the front-end, i.e. the static homepage.
2. Click on the 2 in the pagination or add /page/2/ to the browser's URL.
3. Change the /page/2/ to /page/3/ in the browser's URL. 🐞 Bug occurs.

Expected Results

When testing a patch to validate it works as expected:

• ✅ Step 2: /page/2/ should show the rest of the Sample Page's content and set the pagination such that 1 is now linked.
• ✅ Step 3: /page/3/ (which does not exist) should redirect to the static homepage's root URL, e.g. http://localhost:8889, show the first part of the Sample Page's content, and set the pagination such that 2 is linked.

When reproducing a bug:

• ✅ Step 2: /page/2/ should show the rest of the Sample Page's content and set the pagination such that 1 is now linked.
• ❌ Step 3: /page/3/ (which does not exist) does not redirect, i.e. the error condition occurs.

Environment

• Localhost: npm run env setup (Docker)
• Browser: Firefox 130.0.1
• WordPress: 6.6.2 and 6.7-alpha-58576-src
• PHP: 8.2
• OS: macOS
• Theme: TT4
• Plugins: none

Actual Results

When reproducing a bug/defect:

• ✅ Step 2: /page/2/ did properly show the Sample Page's remaining content and the pagination was correct.
• ❌ Step 3: /page/3/ (which does not exist) did not redirect.

When testing the bugfix patch:

• ✅ Step 2: /page/2/ should show the rest of the Sample Page's content and set the pagination such that 1 is now linked.
• ✅ Step 3: /page/3/ (which does not exist) did redirect to root URL which rendered the first part of the Sample Page's content with the expected pagination.

[hellofromTonya]

Test Report with classic theme

Patches tested:

• ​https://github.com/WordPress/wordpress-develop/pull/6421 which includes ​PR 270 (for the source code changes) and unit tests.

Steps to Reproduce or Test

Used the same setup and testing instructions as my block theme test report, expect for 1 extra set up step:

1. Install and activate a classic theme such as Twenty Nineteen theme.

Expected Results

See comment:36.

Environment

• Localhost: npm run env setup (Docker)
• Browser: Firefox 130.0.1
• WordPress: 6.6.2 and 6.7-alpha-58576-src
• PHP: 8.2
• OS: macOS
• Theme: Twenty Nineteen
• Plugins: none

Actual Results

Same results as comment:36

• Can reproduce the bug.
• ✅ The patch(es) resolve the bug.

[hellofromTonya]

Thanks @peterwilsoncc and @joemcgill for ping reminders.

Patch: ​https://github.com/WordPress/wordpress-develop/pull/6421

With 2 PR approvals, ​one from @peterwilsoncc and ​one from me, marking this patch for commit and am preparing the commit.

[hellofromTonya]

In 59091:

Canonical: Redirect when front page's paginated states not found.

Perform a canonical redirect for an invalid pagination request of a static front page.

When a site has a static front page assigned and that page has a <!--nextpage--> within its content, previously accessing non-existing pages (e.g. example.com/page/3/) did not redirect or return a 404 or 301. This changeset resolves that issue by performing a canonical redirect.

Unit tests are also included for this specific use case and to ensure the fix does not affect a blog listing home page.

Follow-up to [47738], [47727], [34492].

Props dd32, audrasjb, chaion07, hellofromTonya, joemcgill, lukecarbis, Mte90, mukesh27, peterwilsoncc, rajinsharwar, SergeyBiryukov.
Fixes #50163.
See meta#5184.

Committed via https://core.trac.wordpress.org/changeset/59091.

Committed via https://core.trac.wordpress.org/changeset/59091.

[david.binda]

This might be a breaking change for some plugins. I've run into an issue with bbpress which is setup the way there is a [bbp-topic-index] shortcode used on a page which is set as a static homepage for a blog.

Under such setup, the pagination of the topics is broken, as the /page/2 is being redirected back to / (means the r59091 works as expected).

Reproduction steps:

1. install the bbpress plugin
2. have a page with [bbp-topic-index]
3. set is a static homepage
4. create at least 2 bbpress topics
5. set the number of topics in the bbpress's setting screen in Topics and Replies Per Page to 1
6. check that the homepage shows the list of topics
7. try to go to 2nd page of the listing

My understanding is that bbpress is using the ​`paged` query var in the ​rewrite rules it's creating.

It worked well until the r59091 landed, which changed the page check to paged for front pages.

[hellofromTonya]

Thanks @davidbinda for reopening and sharing your findings and concerns. I pinged @jjj to also have a look.

[hellofromTonya]

Sharing a summary of @jjj and my discussion and investigation:

• A static static frontpage:
  • may have a <!--nextpage-->.
  • may have a block or shortcode with its own pagination and query, which may use 'paged' query var (e.g. bbPress with [bbp-topic-index]).
  • and/or may have a combination of the above.

• When a static frontpage has [bbp-topic-index] and <!--nextpage-->, it's get more complicated when the current /page/2 does not include the [bbp-topic-index] content.

Should [59091] be reverted ahead of 6.7 Beta 1?

From @jjj:

Agree. This specific bbPress breakage weighs heavily on my decision, because it is a pretty common type of implementation.

I agree. This particular issue needs a lot more thinking and discussion.

I'll revert it shortly and reopen the PRs associated with it. cc @dd32 @peterwilsoncc

[hellofromTonya]

In 59133:

Canonical: Revert redirect when front page's paginated states not found.

r59091 introduced a backward compatibility (BC) break for a static homepage that includes a shortcode's or block's with paginated content that uses the 'paged' query var, e.g. bbPress.

In this use case, attempting to navigate the shortcode / block's pagination causes a canonical redirect, rather than navigating to the next page of content within that shortcode or block.

Follow-up to [59091].

Props davidbinda, jjj.
See #50163, ​#meta5184.

Reopening as r59091 was reverted by https://core.trac.wordpress.org/changeset/59133.

Why? It caused a BC break for a static homepage that includes a shortcode's or block's with paginated content that uses the 'paged' query var, e.g. bbPress (details are here in the Trac ticket).

Resetting the PR approvals.

Note: r59091 was reverted by https://core.trac.wordpress.org/changeset/59133.

Leaving this PR closed as work is ongoing in PR 6421.

[hellofromTonya]

Added changes-requested keyword to denote current patch(es) will need changes.

[peterwilsoncc]

As 6.7 has entered the beta phase and this ticket is marked as early, I think it's best to bump it to the next release for another attempt/further consideration as to how to account for plugins using pagination on the front-page.

I think the main complication is the one that has been identified: custom shortcodes and custom blocks may make use of pagination. The same could be the case for other code that hijacks the_content in some way without the use of either a block or a shortcode.

[audrasjb]

As per today's bug scrub:
As this ticket is marked early and since it didn't move in 2 weeks, let's punt it from milestone 6.8. Please feel free to re-milestone it for a next release when a new patch is ready for review.