[6.x] Fix: StaticWarm re-paginates already-paginated URLs, producing ?page=3?page=4

[steveparks]

StaticWarm has a different approach to pagination than StaticWarmJob, and introduces an issue as a result, re-paginating already-paginated URLs.

StaticWarmJob guards against following pagination on a URL that is itself a page, and builds its URLs with the right separator:

private function shouldWarmPaginatedPages(ResponseInterface $response): bool
{
    if (! $response->hasHeader('X-Statamic-Pagination')) {
        return false;
    }

    [$currentPage, $totalPages, $pageName] = $this->paginationHeader($response);

    return ! str_contains($this->request->getUri()->getQuery(), "{$pageName}=");
}

return implode('', [
    $url,
    str_contains($url, '?') ? '&' : '?',
    "{$pageName}={$page}",
]);

StaticWarm has neither. It appends unconditionally:

$url = "{$url}?{$pageName}={$page}";

so any URL reaching outputSuccessLine() that already carries ?page=N produces ?page=3?page=4, ?page=3?page=5, and so on.

This reaches real sites because additionalUris() is a public extension point (StaticWarm::hook('additional', …)), and feeding paginated URLs through it is a reasonable thing to do. Eg it is how a site re-warms ?page=N pages that fell out of the cache while their index page stayed in it, which the compiled URI list cannot reach on its own because pagination is only discoverable from a response header.

Once such a URL is in the main pass, it answers with a pagination header of its own and the double-query URLs follow. They render — so the pool reports them ✓ Cached — but they are not the canonical URL of anything and never enter the cache. Observed on my production site: a tracked-pagination set growing 130 → 243 entries in fifty minutes, all of it junk, plus the wasted renders. The work grows with the square of the pagination set.

The fix

Mirror the job: a named shouldWarmPaginatedPages() predicate that folds in the hasHeader check, called from outputSuccessLine().

     public function outputSuccessLine(Response $response, $index): void
     {
-        $this->components->twoColumnDetail($this->getRelativeUri($this->uris()->get($index)), '<info>✓ Cached</info>');
+        $url = $this->uris()->get($index);
+
+        $this->components->twoColumnDetail($this->getRelativeUri($url), '<info>✓ Cached</info>');

-        if ($response->hasHeader('X-Statamic-Pagination')) {
+        if ($this->shouldWarmPaginatedPages($response, $url)) {
             [$currentPage, $totalPages, $pageName] = $this->paginationHeader($response);

-            $this->warmPaginatedPages($this->uris()->get($index), $currentPage, $totalPages, $pageName);
+            $this->warmPaginatedPages($url, $currentPage, $totalPages, $pageName);
         }
     }

+    private function shouldWarmPaginatedPages(Response $response, string $url): bool
+    {
+        if (! $response->hasHeader('X-Statamic-Pagination')) {
+            return false;
+        }
+
+        [$currentPage, $totalPages, $pageName] = $this->paginationHeader($response);
+
+        return ! str_contains(parse_url($url, PHP_URL_QUERY) ?? '', "{$pageName}=");
+    }

and the separator, which also fixes a quieter case — any warmable URL with a pre-existing query string currently gets a malformed second ?:

         $urls = collect(range($currentPage, $totalPages))->map(function ($page) use ($url, $pageName) {
-            $url = "{$url}?{$pageName}={$page}";
+            $url = $url.(str_contains($url, '?') ? '&' : '?')."{$pageName}={$page}";

Alternative: put the predicate in the trait both classes already use

NormalizesPaginationHeader is shared by StaticWarm and StaticWarmJob and is already the home for pagination-header logic, so the guard arguably belongs there rather than being written twice:

trait NormalizesPaginationHeader
{
    protected function paginationHeader(ResponseInterface $response): array { /* unchanged */ }

    protected function shouldWarmPaginatedPages(ResponseInterface $response, string $query): bool
    {
        if (! $response->hasHeader('X-Statamic-Pagination')) {
            return false;
        }

        [, , $pageName] = $this->paginationHeader($response);

        return ! str_contains($query, "{$pageName}=");
    }
}

Each caller passes its own query string — parse_url($url, PHP_URL_QUERY) ?? '' in the command, $this->request->getUri()->getQuery() in the job — which is the only thing that genuinely differs between them. This fixes both classes at once and leaves one place to change next time.

I only mention this here as an idea, and haven't included it in the PR as it's a bigger decision.

[steveparks]

One other point on this....

The check above (copied from the job) matches on a substring, and $pageName is configurable: page_name on the paginating tag, defaulting to page (Tags/Concerns/GetsQueryResults.php).

So a query parameter whose name ends with the page name collides. With the default page, a URL carrying
?news_page=3 reads as already-paginated and its pagination is silently skipped. news_page is what the conventional workaround for two paginated collections on one page produces, so could happen.

Anchoring on the parameter boundary would avoid it:

return ! Str::startsWith($query, "{$pageName}=")
    && ! Str::contains($query, "&{$pageName}=");

I didn't include this in the PR as it's a bigger decision affecting both StaticWarm (after this PR) and StaticWarmJob (as it already is). Maybe this strengthens the case for the trait alternative approach?

[steveparks]

Dammit, looks like I have whitespace issues again. Sorry.
I'll really have to figure out how to contribute upstream PRs from my dev environment rather than trying to replicate through the Github UI!

[jasonvarga]

Do you see this above the comment box?

What happens if you click Lint code style issues – are you able to see the output?

[steveparks]

Yes, it shows me the output if I click that — which is where I saw what (I thought) said it was whitespace issues.

I'm obviously not doing things correctly in the GH editor in the UI.

[jasonvarga]

No worries. It's not really clear from that output since it doesn't actually show you the whitespace.
In the GitHub UI you could have seen it though. There was just some trailing whitespace after the brace.

I've fixed it for you: f3b438d