From 0693cc2a75a1646080501176e24d3fd41a72a6b5 Mon Sep 17 00:00:00 2001 From: Alexey Arendarenko Date: Thu, 14 Nov 2019 15:08:45 +0200 Subject: [PATCH 1/4] UrlRewrite module fixes add ability to save query param from request during redirect to target path --- app/code/Magento/UrlRewrite/Controller/Router.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/Magento/UrlRewrite/Controller/Router.php b/app/code/Magento/UrlRewrite/Controller/Router.php index dd26f49b8efa4..0525621b6a20e 100644 --- a/app/code/Magento/UrlRewrite/Controller/Router.php +++ b/app/code/Magento/UrlRewrite/Controller/Router.php @@ -118,7 +118,7 @@ protected function processRedirect($request, $rewrite) if ($rewrite->getEntityType() !== Rewrite::ENTITY_TYPE_CUSTOM || ($prefix = substr($target, 0, 6)) !== 'http:/' && $prefix !== 'https:' ) { - $target = $this->url->getUrl('', ['_direct' => $target]); + $target = $this->url->getUrl('', ['_direct' => $target, '_query' => $request->getParams()]); } return $this->redirect($request, $target, $rewrite->getRedirectType()); } From 19b7bdac3a33eb12395b9f362a7c03e013228cc8 Mon Sep 17 00:00:00 2001 From: Alexey Arendarenko Date: Thu, 14 Nov 2019 15:08:55 +0200 Subject: [PATCH 2/4] UrlRewrite module fixes update unit test --- .../Test/Unit/Controller/RouterTest.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/UrlRewrite/Test/Unit/Controller/RouterTest.php b/app/code/Magento/UrlRewrite/Test/Unit/Controller/RouterTest.php index c935b3c7ec4cb..c67f3f400b007 100644 --- a/app/code/Magento/UrlRewrite/Test/Unit/Controller/RouterTest.php +++ b/app/code/Magento/UrlRewrite/Test/Unit/Controller/RouterTest.php @@ -260,6 +260,7 @@ public function testNoRewriteAfterStoreSwitcherWhenOldRewriteEqualsToNewOne() */ public function testMatchWithRedirect() { + $queryParams = []; $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store)); $urlRewrite = $this->getMockBuilder(UrlRewrite::class) ->disableOriginalConstructor()->getMock(); @@ -268,7 +269,11 @@ public function testMatchWithRedirect() $this->urlFinder->expects($this->any())->method('findOneByData')->will($this->returnValue($urlRewrite)); $this->response->expects($this->once())->method('setRedirect') ->with('new-target-path', 'redirect-code'); - $this->url->expects($this->once())->method('getUrl')->with('', ['_direct' => 'target-path']) + $this->request->expects($this->once())->method('getParams')->willReturn($queryParams); + $this->url->expects($this->once())->method('getUrl')->with( + '', + ['_direct' => 'target-path', '_query' => $queryParams] + ) ->will($this->returnValue('new-target-path')); $this->request->expects($this->once())->method('setDispatched')->with(true); $this->actionFactory->expects($this->once())->method('create') @@ -282,6 +287,7 @@ public function testMatchWithRedirect() */ public function testMatchWithCustomInternalRedirect() { + $queryParams = []; $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store)); $urlRewrite = $this->getMockBuilder(UrlRewrite::class) ->disableOriginalConstructor()->getMock(); @@ -289,8 +295,12 @@ public function testMatchWithCustomInternalRedirect() $urlRewrite->expects($this->any())->method('getRedirectType')->will($this->returnValue('redirect-code')); $urlRewrite->expects($this->any())->method('getTargetPath')->will($this->returnValue('target-path')); $this->urlFinder->expects($this->any())->method('findOneByData')->will($this->returnValue($urlRewrite)); + $this->request->expects($this->any())->method('getParams')->willReturn($queryParams); $this->response->expects($this->once())->method('setRedirect')->with('a', 'redirect-code'); - $this->url->expects($this->once())->method('getUrl')->with('', ['_direct' => 'target-path'])->willReturn('a'); + $this->url->expects($this->once())->method('getUrl')->with( + '', + ['_direct' => 'target-path', '_query' => $queryParams] + )->willReturn('a'); $this->request->expects($this->once())->method('setDispatched')->with(true); $this->actionFactory->expects($this->once())->method('create') ->with(\Magento\Framework\App\Action\Redirect::class); @@ -312,6 +322,7 @@ public function testMatchWithCustomExternalRedirect($targetPath) $urlRewrite->expects($this->any())->method('getTargetPath')->will($this->returnValue($targetPath)); $this->urlFinder->expects($this->any())->method('findOneByData')->will($this->returnValue($urlRewrite)); $this->response->expects($this->once())->method('setRedirect')->with($targetPath, 'redirect-code'); + $this->request->expects($this->never())->method('getParams'); $this->url->expects($this->never())->method('getUrl'); $this->request->expects($this->once())->method('setDispatched')->with(true); $this->actionFactory->expects($this->once())->method('create') From f63475c37e37dde8eef9f404f518eb415c7557b0 Mon Sep 17 00:00:00 2001 From: Alexey Arendarenko Date: Thu, 14 Nov 2019 15:10:37 +0200 Subject: [PATCH 3/4] UrlRewrite module fixes update fixture update integration test --- .../UrlRewrite/Controller/UrlRewriteTest.php | 30 +++++++++++++ .../Magento/UrlRewrite/_files/url_rewrite.php | 42 ++++++++++++++++++- .../_files/url_rewrite_rollback.php | 4 +- 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php index 5f8adc5d65113..b6a551cc6ad50 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php @@ -80,6 +80,36 @@ public function requestDataProvider(): array 'request' => '/page-similar/', 'redirect' => '/page-b', ], + 'Use Case #7: Rewrite: page-similar --(301)--> page-a; ' + . 'Request: page-similar?param=1 --(301)--> page-a?param=1' => [ + 'request' => '/page-similar?param=1', + 'redirect' => '/page-a?param=1', + ], + 'Use Case #8: Rewrite: page-similar/ --(301)--> page-b; ' + . 'Request: page-similar/?param=1 --(301)--> page-b?param=1' => [ + 'request' => '/page-similar/?param=1', + 'redirect' => '/page-b?param=1', + ], + 'Use Case #9: Rewrite: page-similar-query-param --(301)--> page-d?param1=1;' + . 'Request: page-similar-query-param --(301)--> page-d?param1=1' => [ + 'request' => '/page-similar-query-param', + 'redirect' => '/page-d?param1=1', + ], + 'Use Case #10: Rewrite: page-similar-query-param --(301)--> page-d?param1=1; ' + . 'Request: page-similar-query-param?param2=1 --(301)--> page-d?param1=1¶m2=1' => [ + 'request' => '/page-similar-query-param?param2=1', + 'redirect' => '/page-d?param1=1¶m2=1', + ], + 'Use Case #11: Rewrite: page-similar-query-param/ --(301)--> page-e?param1=1; ' + . 'Request: page-similar-query-param/ --(301)--> page-e?param1=1' => [ + 'request' => '/page-similar-query-param/', + 'redirect' => '/page-e?param1=1', + ], + 'Use Case #12: Rewrite: page-similar-query-param/ --(301)--> page-e?param1=1;' + . 'Request: page-similar-query-param/?param2=1 --(301)--> page-e?param1=1¶m2=1' => [ + 'request' => '/page-similar-query-param/?param2=1', + 'redirect' => '/page-e?param1=1¶m2=1', + ], ]; } } diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php index 8e82aa853d4a4..dc3f5490f5a53 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php @@ -64,6 +64,26 @@ ->setStores([$storeID, $secondStoreId]); $pageResource->save($page); +$page = $objectManager->create(Page::class); +$page->setTitle('Cms D') + ->setIdentifier('page-d') + ->setIsActive(1) + ->setContent('

Cms Page D

') + ->setPageLayout('1column') + ->setCustomTheme('Magento/blank') + ->setStores([$storeID, $secondStoreId]); +$pageResource->save($page); + +$page = $objectManager->create(Page::class); +$page->setTitle('Cms E') + ->setIdentifier('page-e') + ->setIsActive(1) + ->setContent('

Cms Page E

') + ->setPageLayout('1column') + ->setCustomTheme('Magento/blank') + ->setStores([$storeID, $secondStoreId]); +$pageResource->save($page); + $rewrite = $objectManager->create(UrlRewrite::class); $rewrite->setEntityType('custom') ->setRequestPath('page-one/') @@ -88,7 +108,7 @@ ->setTargetPath('page-a') ->setRedirectType(OptionProvider::PERMANENT) ->setStoreId($storeID) - ->setDescription('From age-similar without trailing slash to page-a'); + ->setDescription('From page-similar without trailing slash to page-a'); $rewriteResource->save($rewrite); $rewrite = $objectManager->create(UrlRewrite::class); @@ -97,7 +117,7 @@ ->setTargetPath('page-b') ->setRedirectType(OptionProvider::PERMANENT) ->setStoreId($storeID) - ->setDescription('From age-similar with trailing slash to page-b'); + ->setDescription('From page-similar with trailing slash to page-b'); $rewriteResource->save($rewrite); //Emulating auto-generated aliases (like the ones used for categories). @@ -117,3 +137,21 @@ ->setRedirectType(0) ->setStoreId($secondStoreId); $rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-similar-query-param') + ->setTargetPath('page-d?param1=1') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-similar-query-param to page-d with query param'); +$rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-similar-query-param/') + ->setTargetPath('page-e?param1=1') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-similar-query-param with trailing slash to page-e with query param'); +$rewriteResource->save($rewrite); diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php index 22d95751fbf26..76b84810ac433 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php @@ -19,6 +19,8 @@ $pageRepository->deleteById('page-a'); $pageRepository->deleteById('page-b'); $pageRepository->deleteById('page-c'); +$pageRepository->deleteById('page-d'); +$pageRepository->deleteById('page-e'); /** @var \Magento\Catalog\Api\ProductRepositoryInterface $productRepository */ $productRepository = \Magento\TestFramework\Helper\Bootstrap::getObjectManager() @@ -29,7 +31,7 @@ ->create(\Magento\UrlRewrite\Model\ResourceModel\UrlRewriteCollection::class); $collection = $urlRewriteCollection ->addFieldToFilter('entity_type', 'custom') - ->addFieldToFilter('target_path', ['page-a/', 'page-a', 'page-b', 'page-c']) + ->addFieldToFilter('target_path', ['page-a/', 'page-a', 'page-b', 'page-c', 'page-d?param1=1', 'page-e?param1=1']) ->load() ->walk('delete'); From b1a2a29671de32d90dbc10a3b0ced6a3d71b2b72 Mon Sep 17 00:00:00 2001 From: Ihor Sviziev Date: Tue, 3 Dec 2019 21:45:36 +0200 Subject: [PATCH 4/4] magento/magento2#25603 Fix removing query string from url after redirect Cover with integration tests cases with external URLs as target path --- .../UrlRewrite/Controller/UrlRewriteTest.php | 27 ++++++++++++-- .../Magento/UrlRewrite/_files/url_rewrite.php | 36 +++++++++++++++++++ .../_files/url_rewrite_rollback.php | 16 ++++++++- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php index b6a551cc6ad50..cea44226f6192 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/Controller/UrlRewriteTest.php @@ -44,8 +44,7 @@ public function testMatchUrlRewrite( $location = $response->getHeader('Location')->getFieldValue(); $this->assertStringEndsWith( $redirect, - $location, - 'Invalid location header' + $location ); } } @@ -110,6 +109,30 @@ public function requestDataProvider(): array 'request' => '/page-similar-query-param/?param2=1', 'redirect' => '/page-e?param1=1¶m2=1', ], + 'Use Case #13: Rewrite: page-external1 --(301)--> http://example.com/external;' + . 'Request: page-external1?param1=1 --(301)--> http://example.com/external (not fills get params)' => [ + 'request' => '/page-external1?param1=1', + 'redirect' => 'http://example.com/external', + ], + 'Use Case #14: Rewrite: page-external2/ --(301)--> https://example.com/external2/;' + . 'Request: page-external2?param2=1 --(301)--> https://example.com/external2/ (not fills get params)' => [ + 'request' => '/page-external2?param2=1', + 'redirect' => 'https://example.com/external2/', + ], + 'Use Case #15: Rewrite: page-external3 --(301)--> http://example.com/external?param1=value1;' + . 'Request: page-external3?param1=custom1¶m2=custom2 --(301)--> ' + . 'http://example.com/external?param1=value1' + . ' (fills get param from target path)' => [ + 'request' => '/page-external3?param1=custom1¶m2=custom2', + 'redirect' => 'http://example.com/external?param1=value1', + ], + 'Use Case #16: Rewrite: page-external4/ --(301)--> https://example.com/external2/?param2=value2;' + . 'Request: page-external4?param1=custom1¶m2=custom2 --(301)--> ' + . 'https://example.com/external2/?param2=value2 ' + . ' (fills get param from target path)' => [ + 'request' => '/page-external4?param1=custom1¶m2=custom2', + 'redirect' => 'https://example.com/external2/?param2=value2', + ], ]; } } diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php index dc3f5490f5a53..68d1212539c6d 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite.php @@ -155,3 +155,39 @@ ->setStoreId($storeID) ->setDescription('From page-similar-query-param with trailing slash to page-e with query param'); $rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-external1') + ->setTargetPath('http://example.com/external') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-external to external URL'); +$rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-external2/') + ->setTargetPath('https://example.com/external2/') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-external with trailing slash to external URL'); +$rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-external3') + ->setTargetPath('http://example.com/external?param1=value1') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-external to external URL'); +$rewriteResource->save($rewrite); + +$rewrite = $objectManager->create(UrlRewrite::class); +$rewrite->setEntityType('custom') + ->setRequestPath('page-external4/') + ->setTargetPath('https://example.com/external2/?param2=value2') + ->setRedirectType(OptionProvider::PERMANENT) + ->setStoreId($storeID) + ->setDescription('From page-external with trailing slash to external URL'); +$rewriteResource->save($rewrite); diff --git a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php index 76b84810ac433..a5c21f7a00e48 100644 --- a/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php +++ b/dev/tests/integration/testsuite/Magento/UrlRewrite/_files/url_rewrite_rollback.php @@ -31,7 +31,21 @@ ->create(\Magento\UrlRewrite\Model\ResourceModel\UrlRewriteCollection::class); $collection = $urlRewriteCollection ->addFieldToFilter('entity_type', 'custom') - ->addFieldToFilter('target_path', ['page-a/', 'page-a', 'page-b', 'page-c', 'page-d?param1=1', 'page-e?param1=1']) + ->addFieldToFilter( + 'target_path', + [ + 'page-a/', + 'page-a', + 'page-b', + 'page-c', + 'page-d?param1=1', + 'page-e?param1=1', + 'http://example.com/external', + 'https://example.com/external2/', + 'http://example.com/external?param1=value1', + 'https://example.com/external2/?param2=value2' + ] + ) ->load() ->walk('delete');