From 7c8733c4c8d7179d55cc071be10f9e92cf31b726 Mon Sep 17 00:00:00 2001 From: Nate Swanson Date: Mon, 8 Dec 2025 18:55:41 -0600 Subject: [PATCH 1/4] Build query string array from url stripped of marketing params instead of original request uri. --- .../Framework/App/PageCache/Identifier.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/internal/Magento/Framework/App/PageCache/Identifier.php b/lib/internal/Magento/Framework/App/PageCache/Identifier.php index b10681654683d..232797f13036d 100644 --- a/lib/internal/Magento/Framework/App/PageCache/Identifier.php +++ b/lib/internal/Magento/Framework/App/PageCache/Identifier.php @@ -106,14 +106,22 @@ private function reconstructUrl(string $url): array if (empty($url)) { return [$url, '']; } + $baseUrl = strtok($url, '?'); - $query = $this->request->getUri()->getQueryAsArray(); - if (!empty($query)) { - ksort($query); - $query = http_build_query($query); + $queryString = parse_url($url, PHP_URL_QUERY) ?? ''; + + $queryArray = []; + if ($queryString !== '') { + parse_str($queryString, $queryArray); + } + + if (!empty($queryArray)) { + ksort($queryArray); + $query = http_build_query($queryArray); } else { $query = ''; } + return [$baseUrl, $query]; } } From bf7f6c19f0ba4fe0be3c050f4a55020a3722a479 Mon Sep 17 00:00:00 2001 From: Nate Swanson Date: Tue, 10 Mar 2026 09:17:05 -0500 Subject: [PATCH 2/4] Handle invalid parsed query values in page cache identifier and added unit test for invalid url. --- .../Framework/App/PageCache/Identifier.php | 2 +- .../Test/Unit/PageCache/IdentifierTest.php | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/App/PageCache/Identifier.php b/lib/internal/Magento/Framework/App/PageCache/Identifier.php index 232797f13036d..8ae1c20bac305 100644 --- a/lib/internal/Magento/Framework/App/PageCache/Identifier.php +++ b/lib/internal/Magento/Framework/App/PageCache/Identifier.php @@ -108,7 +108,7 @@ private function reconstructUrl(string $url): array } $baseUrl = strtok($url, '?'); - $queryString = parse_url($url, PHP_URL_QUERY) ?? ''; + $queryString = parse_url($url, PHP_URL_QUERY) ?: ''; $queryArray = []; if ($queryString !== '') { diff --git a/lib/internal/Magento/Framework/App/Test/Unit/PageCache/IdentifierTest.php b/lib/internal/Magento/Framework/App/Test/Unit/PageCache/IdentifierTest.php index 6b7aaafaab28d..2e48efb9f78f3 100644 --- a/lib/internal/Magento/Framework/App/Test/Unit/PageCache/IdentifierTest.php +++ b/lib/internal/Magento/Framework/App/Test/Unit/PageCache/IdentifierTest.php @@ -339,4 +339,44 @@ public function testGetValueWithMarketingParameters(): void $this->model->getValue() ); } + + /** + * Test get identifier value with invalid URL. + * + * @return void + */ + public function testGetValueWithInvalidUrl(): void + { + $this->requestMock->expects($this->any()) + ->method('isSecure') + ->willReturn(true); + + $this->requestMock->expects($this->any()) + ->method('getUriString') + ->willReturn('http://example.com:invalid_port/path1/?a=1'); + + $this->contextMock->expects($this->any()) + ->method('getVaryString') + ->willReturn(self::VARY); + + $uri = $this->createMock(HttpUri::class); + $uri->expects($this->any())->method('getQueryAsArray')->willReturn([]); + $this->requestMock->expects($this->any()) + ->method('getUri') + ->willReturn($uri); + + $this->assertEquals( + sha1( + json_encode( + [ + true, + 'http://example.com:invalid_port/path1/', + '', + self::VARY + ] + ) + ), + $this->model->getValue() + ); + } } From ba6348a879effb9caca3261d0cec2ab6b9ef505a Mon Sep 17 00:00:00 2001 From: Nate Swanson Date: Tue, 31 Mar 2026 13:30:37 -0500 Subject: [PATCH 3/4] Moved URL reconstruction logic into the cache identifier for save class (\Magento\PageCache\Model\App\Request\Http\IdentifierForSave). --- .../App/Request/Http/IdentifierForSave.php | 24 +------------------ .../Framework/App/PageCache/Identifier.php | 2 +- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/app/code/Magento/PageCache/Model/App/Request/Http/IdentifierForSave.php b/app/code/Magento/PageCache/Model/App/Request/Http/IdentifierForSave.php index db7cc7f9b7074..8f284ad7e0b27 100644 --- a/app/code/Magento/PageCache/Model/App/Request/Http/IdentifierForSave.php +++ b/app/code/Magento/PageCache/Model/App/Request/Http/IdentifierForSave.php @@ -47,7 +47,7 @@ public function getValue() $pattern = $this->identifier->getMarketingParameterPatterns(); $replace = array_fill(0, count($pattern), ''); $url = preg_replace($pattern, $replace, (string)$this->request->getUriString()); - list($baseUrl, $query) = $this->reconstructUrl($url); + list($baseUrl, $query) = $this->identifier->reconstructUrl($url); $data = [ $this->request->isSecure(), $baseUrl, @@ -59,26 +59,4 @@ public function getValue() $data = $this->identifierStoreReader->getPageTagsWithStoreCacheTags($data); return sha1($this->serializer->serialize($data)); } - - /** - * Reconstruct url and sort query - * - * @param string $url - * @return array - */ - private function reconstructUrl(string $url): array - { - if (empty($url)) { - return [$url, '']; - } - $baseUrl = strtok($url, '?'); - $query = $this->request->getUri()->getQueryAsArray(); - if (!empty($query)) { - ksort($query); - $query = http_build_query($query); - } else { - $query = ''; - } - return [$baseUrl, $query]; - } } diff --git a/lib/internal/Magento/Framework/App/PageCache/Identifier.php b/lib/internal/Magento/Framework/App/PageCache/Identifier.php index 8ae1c20bac305..7ee85020c7661 100644 --- a/lib/internal/Magento/Framework/App/PageCache/Identifier.php +++ b/lib/internal/Magento/Framework/App/PageCache/Identifier.php @@ -101,7 +101,7 @@ public function getMarketingParameterPatterns(): array * @param string $url * @return array */ - private function reconstructUrl(string $url): array + public function reconstructUrl(string $url): array { if (empty($url)) { return [$url, '']; From dea16c4bea2a2279c9592b767f79363a92f81883 Mon Sep 17 00:00:00 2001 From: Nate Swanson Date: Tue, 31 Mar 2026 14:04:50 -0500 Subject: [PATCH 4/4] Updated IdentifierForSaveTest unit tests to work with new logic in \Magento\PageCache\Model\App\Request\Http\IdentifierForSave. --- .../Request/Http/IdentifierForSaveTest.php | 74 +++++-------------- 1 file changed, 19 insertions(+), 55 deletions(-) diff --git a/app/code/Magento/PageCache/Test/Unit/Model/App/Request/Http/IdentifierForSaveTest.php b/app/code/Magento/PageCache/Test/Unit/Model/App/Request/Http/IdentifierForSaveTest.php index 7c8dc4976fe64..62928a42a9749 100644 --- a/app/code/Magento/PageCache/Test/Unit/Model/App/Request/Http/IdentifierForSaveTest.php +++ b/app/code/Magento/PageCache/Test/Unit/Model/App/Request/Http/IdentifierForSaveTest.php @@ -7,8 +7,6 @@ namespace Magento\PageCache\Test\Unit\Model\App\Request\Http; -use Laminas\Stdlib\Parameters; -use Laminas\Uri\Http as HttpUri; use Magento\Framework\App\Http\Context; use Magento\Framework\App\ObjectManager; use Magento\Framework\App\PageCache\Identifier; @@ -56,9 +54,6 @@ class IdentifierForSaveTest extends TestCase */ private $identifierStoreReader; - /** @var Parameters|MockObject */ - private $fileParams; - /** * @var Identifier */ @@ -86,8 +81,6 @@ function ($value) { return json_encode($value); } ); - $this->fileParams = $this->createMock(Parameters::class); - $this->identifierStoreReader = $this->getMockBuilder(IdentifierStoreReader::class) ->onlyMethods(['getPageTagsWithStoreCacheTags']) ->disableOriginalConstructor() @@ -128,25 +121,15 @@ public function testGetValue(): void $this->requestMock->expects($this->any()) ->method('getUriString') ->willReturn('http://example.com/path1/'); - - $this->requestMock->expects($this->any()) - ->method('getQuery') - ->willReturn($this->fileParams); - - $this->fileParams->expects($this->any()) - ->method('toArray') - ->willReturn([]); + $this->identifierMock->expects($this->once()) + ->method('reconstructUrl') + ->with('http://example.com/path1/') + ->willReturn(['http://example.com/path1/', '']); $this->contextMock->expects($this->any()) ->method('getVaryString') ->willReturn(self::VARY); - $uri = $this->createMock(HttpUri::class); - $uri->expects($this->any())->method('getQueryAsArray')->willReturn(''); - $this->requestMock->expects($this->any()) - ->method('getUri') - ->willReturn($uri); - $this->identifierStoreReader->method('getPageTagsWithStoreCacheTags')->willReturnCallback( function ($value) { return $value; @@ -175,6 +158,9 @@ function ($value) { */ public function testGetValueWithQuery(): void { + $this->identifierMock->expects($this->once()) + ->method('getMarketingParameterPatterns') + ->willReturn([]); $this->requestMock->expects($this->any()) ->method('isSecure') ->willReturn(true); @@ -182,31 +168,15 @@ public function testGetValueWithQuery(): void $this->requestMock->expects($this->any()) ->method('getUriString') ->willReturn('http://example.com/path1/?b=2&a=1'); - - $this->requestMock->expects($this->any()) - ->method('getQuery') - ->willReturn($this->fileParams); - - $this->fileParams->expects($this->any()) - ->method('toArray') - ->willReturn([ - 'b' => 2, - 'a' => 1, - ]); + $this->identifierMock->expects($this->once()) + ->method('reconstructUrl') + ->with('http://example.com/path1/?b=2&a=1') + ->willReturn(['http://example.com/path1/', 'a=1&b=2']); $this->contextMock->expects($this->any()) ->method('getVaryString') ->willReturn(self::VARY); - $uri = $this->createMock(HttpUri::class); - $uri->expects($this->any())->method('getQueryAsArray')->willReturn([ - 'b' => 2, - 'a' => 1, - ]); - $this->requestMock->expects($this->any()) - ->method('getUri') - ->willReturn($uri); - $this->identifierStoreReader->method('getPageTagsWithStoreCacheTags')->willReturnCallback( function ($value) { return $value; @@ -246,17 +216,15 @@ public function testGetValueWithMarketingParameters(): void $this->requestMock->expects($this->any()) ->method('getUriString') ->willReturn('http://example.com/path1/?abc=123&gclid=456&utm_source=abc'); + $this->identifierMock->expects($this->once()) + ->method('reconstructUrl') + ->with('http://example.com/path1/?abc=123') + ->willReturn(['http://example.com/path1/', 'abc=123']); $this->contextMock->expects($this->any()) ->method('getVaryString') ->willReturn(self::VARY); - $uri = $this->createMock(HttpUri::class); - $uri->expects($this->any())->method('getQueryAsArray')->willReturn(['abc' => '123']); - $this->requestMock->expects($this->any()) - ->method('getUri') - ->willReturn($uri); - $this->identifierStoreReader->method('getPageTagsWithStoreCacheTags')->willReturnCallback( function ($value) { return $value; @@ -303,6 +271,10 @@ public function testGetValueVaryStringResolution( $this->requestMock->expects($this->once()) ->method('getUriString') ->willReturn('http://example.com/path1/'); + $this->identifierMock->expects($this->once()) + ->method('reconstructUrl') + ->with('http://example.com/path1/') + ->willReturn(['http://example.com/path1/', '']); $this->requestMock->expects($this->once()) ->method('get') ->with(Http::COOKIE_VARY_STRING) @@ -311,14 +283,6 @@ public function testGetValueVaryStringResolution( $this->contextMock->expects($expectContextCall ? $this->once() : $this->never()) ->method('getVaryString') ->willReturn($contextVaryString); - - $uri = $this->createMock(HttpUri::class); - $uri->expects($this->once()) - ->method('getQueryAsArray') - ->willReturn([]); - $this->requestMock->expects($this->once()) - ->method('getUri') - ->willReturn($uri); $this->identifierStoreReader->expects($this->once()) ->method('getPageTagsWithStoreCacheTags') ->willReturnArgument(0);