Skip to content

Random Redirects: random post type archives. - #15838

Open
peterwilsoncc wants to merge 23 commits into
WordPress:trunkfrom
peterwilsoncc:test/random-post-type-priority
Open

peterwilsoncc wants to merge 23 commits into
WordPress:trunkfrom
peterwilsoncc:test/random-post-type-priority

Conversation

@peterwilsoncc

@peterwilsoncc peterwilsoncc commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Ensures rewrite rules added to the top, including post type and taxonomy rules, take precedence over random content rules using the same regex.

  • Random content rules with the same regex as a rule in extra_rules_top are omitted when the rules are generated.
  • Previously array_merge() kept the top rule's position but replaced its query with the random query, so a post type archive at /random/ redirected to random content.
  • Rules using a different regex for the same path (eg ^random/?$) and bottom rules behave as they do on trunk.
  • Documents that a post type or taxonomy with the rewrite slug random may match random archive URLs. The workaround is changing the random base on the Permalinks screen.

Demonstrating the tests

Removed since earlier reviews

  • Regex-based detection of existing rules taking the random base, which gave false positives and negatives.
  • The $existing_rules parameter of WP_Rewrite::random_rewrite_rules().
  • The wp_rewrite_random_base_is_taken filter. The existing random_rewrite_rules filter can remove the root rule.
  • Tests for the above.

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

Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Opus 5.5 Medium
Used for: Code, tests, running code on test box. Reviewed and manually tested by me prior to marking this PR ready for review.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@peterwilsoncc
peterwilsoncc force-pushed the test/random-post-type-priority branch from a06ea9d to ee8cbec Compare November 8, 2026 22:04
@peterwilsoncc
peterwilsoncc force-pushed the test/random-post-type-priority branch from ee8cbec to d434e33 Compare November 9, 2026 02:45
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@peterwilsoncc
peterwilsoncc force-pushed the test/random-post-type-priority branch from 31f654a to 2f3742d Compare November 9, 2026 03:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved rewrite-rule precedence and conflict-detection issues can misroute /random/ requests or cause 404s, while missing post-type cleanup makes redirect tests order-dependent.

3 open issues
What changed in this PR

This PR updates random-content rewrite handling to detect conflicts with post type archives and custom routes at /random/.

Changes:

  • Checks existing rewrite rules for base conflicts and adds a filter to override detection.
  • Adds rewrite and redirect tests for conflicting routes, custom bases, and permalink variants.
  • Restores rewrite state during test cleanup and aligns data-provider names with their tests.
File Description
tests/​phpunit/​tests/​wp/​handleRandom.php Renames data providers to match test methods.
tests/​phpunit/​tests/​rewrite/​randomRewriteRules.php Adds conflict-detection coverage and rewrite-state cleanup.
tests/​phpunit/​tests/​query/​randomContentRedirect.php Adds request-level conflict tests and rewrite-state cleanup.
src/​wp-includes/​class-wp-rewrite.php Adds existing-rule conflict detection, an override filter, and documentation.

Review effort: Balanced

Comment thread src/wp-includes/class-wp-rewrite.php Resolved
Comment thread src/wp-includes/class-wp-rewrite.php Resolved
Comment thread src/wp-includes/class-wp-rewrite.php Resolved
@peterwilsoncc

peterwilsoncc commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Review by Claude Opus 5.5 Medium.

Code

  1. Bug: bottom rules are passed to the detection. class-wp-rewrite.php:1481 and the default at line 1612 include $this->extra_rules. An identical-regex bottom rule worked on trunk and now 404s, and other bottom rules for the base also turn /random/ into a 404. (Mine, extended by Copilot Comment 1.) Fix: leave out extra_rules; trunk's merge then lets an identical-regex bottom rule win again. Add a request-level regression test.
  2. The approach is heavier than the bug needs. The real collision was a top rule with the identical regex, random/?$, having its value overwritten. Rules written differently already won by order. Checking for the same key, or merging so the existing rule's value wins, would fix it. Detection only changes routing for rules between the random and page/post rules (root, comments, search, author, date). (Mine, refined by Copilot comment 1.)
  3. The catch-all test is unreliable.
    • Missed: a rule that only handles URLs ending in the slug, like (.*)random/?$, is treated as a catch-all. (Copilot comment 2.)
    • Wrongly flagged: a length-limited rule like ([a-z]{6})/?$ is treated as taking the base. (Mine.)
    • Fix: test against a path without the slug, and add a request-level test for the suffix case.
  4. The $existing_rules default is documented wrongly. The docblock at class-wp-rewrite.php:1568-1572 says the default includes add_permastruct() rules. It only does after rewrite_rules() has run.
  5. The rule list is copied by hand. Line 1481 repeats the final merge list minus three groups; build it once.
  6. Minor:
    • An invalid regex in any rule now warns on every flush.
    • $query is unused in the loop.
    • Adding a filter and a method parameter during 7.2-beta4 probably needs release lead sign-off and a dev note.

Tests

  1. Teardown leak. Fix as described in Copilot comment 3 above. (Copilot comment 3.)
  2. A test that can't fail. randomRewriteRules.php:1077 passes by construction, because $wp_rewrite->rules is never read.
  3. Copy error in a data provider. In randomRewriteRules.php:1318, the "archive slug random" case also sets the rewrite slug, which makes it the same test as "rewrite slug random".
  4. The data provider at randomRewriteRules.php:1221 is too large. It builds about 228 cases; two or three rules across the permalink structures would cover the same ground.
  5. Duplicates between the two test files:
    • rule takes the base: query test at line 2879 and the rewrite test's 17-regex version
    • rule doesn't take the base: query test at line 2932 and the 228-case rewrite test
    • the "filtered as taken" query tests at lines 2979 and 2991, and the rewrite test at line 1477
    • the query test's "requires a sub path" case and the rewrite test at line 1266
    • the two post type archive data providers
  6. Missing tests:
    • a bottom rule that takes the base (see 1)
    • a rule matching only URLs that end in the slug (see 3)
    • calling the method with an explicit $existing_rules, such as array()
    • a rule added through the root, author or date rule filters that takes the base

Already known

  1. A post type named random still takes over random archive URLs through its attachment rules. b6a012b documents this.

Comment thread src/wp-includes/class-wp-rewrite.php Resolved
Comment thread src/wp-includes/class-wp-rewrite.php Resolved
@peterwilsoncc
peterwilsoncc marked this pull request as ready for review November 10, 2026 23:19
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Core Committers: Use this line as a base for the props when committing in SVN:

Props peterwilsoncc, jorbin.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@peterwilsoncc

Copy link
Copy Markdown
Contributor Author

@aaronjorbin I've simplified the approach, so some of your earlier review comments are now outdated.

The PR description has the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants