MAGETWO-96663: UrlRewrite removes query string from url, if url has trailing slash

Author: serhii-balko

app/code/Magento/UrlRewrite/Controller/Router.php

@@ -77,7 +77,7 @@ public function __construct(
     public function match(RequestInterface $request)
     {
         $rewrite = $this->getRewrite(
-            $request->getPathInfo(),
+            $this->getNormalisedRequestPath($request),
             $this->storeManager->getStore()->getId()
         );
 
@@ -153,4 +153,30 @@ protected function getRewrite($requestPath, $storeId)
             ]
         );
     }
+
+    /**
+     * Get normalized request path
+     *
+     * @param RequestInterface|HttpRequest $request
+     * @return string
+     */
+    private function getNormalisedRequestPath(RequestInterface $request)
+    {
+        $path = $request->getPathInfo();
+        /**
+         * If request contains query params then we need to trim a slash in end of the path.
+         * For example:
+         * the original request is: http://my-host.com/category-url-key.html/?color=black
+         * where the original path is: category-url-key.html/
+         * and the result path will be: category-url-key.html
+         *
+         * It need to except a redirect like this:
+         * http://my-host.com/category-url-key.html/?color=black => http://my-host.com/category-url-key.html
+         */
+        if (!empty($path) && $request->getQuery()->count()) {
+            $path = rtrim($path, '/');
+        }
+
+        return $path;
+    }
 }

app/code/Magento/UrlRewrite/Test/Unit/Controller/RouterTest.php

@@ -3,61 +3,88 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
-
 namespace Magento\UrlRewrite\Test\Unit\Controller;
 
 use Magento\Framework\App\Action\Forward;
 use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
+use Magento\Framework\UrlInterface;
 use Magento\UrlRewrite\Service\V1\Data\UrlRewrite;
 use Magento\Store\Model\Store;
+use PHPUnit\Framework\MockObject\MockObject;
+use Zend\Stdlib\ParametersInterface;
 
 /**
+ * Test class for UrlRewrite Controller Router
+ *
  * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
  */
 class RouterTest extends \PHPUnit\Framework\TestCase
 {
-    /** @var \Magento\UrlRewrite\Controller\Router */
-    protected $router;
+    /**
+     * @var \Magento\UrlRewrite\Controller\Router
+     */
+    private $router;
+
+    /**
+     * @var \Magento\Framework\App\ActionFactory|MockObject
+     */
+    private $actionFactory;
 
-    /** @var \Magento\Framework\App\ActionFactory|\PHPUnit_Framework_MockObject_MockObject */
-    protected $actionFactory;
+    /**
+     * @var UrlInterface|MockObject
+     */
+    private $url;
 
-    /** @var \Magento\Framework\UrlInterface|\PHPUnit_Framework_MockObject_MockObject */
-    protected $url;
+    /**
+     * @var \Magento\Store\Model\StoreManagerInterface|MockObject
+     */
+    private $storeManager;
 
-    /** @var \Magento\Store\Model\StoreManagerInterface|\PHPUnit_Framework_MockObject_MockObject */
-    protected $storeManager;
+    /**
+     * @var Store|MockObject
+     */
+    private $store;
 
-    /** @var \Magento\Store\Model\Store|\PHPUnit_Framework_MockObject_MockObject */
-    protected $store;
+    /**
+     * @var \Magento\Framework\App\ResponseInterface|MockObject
+     */
+    private $response;
 
-    /** @var \Magento\Framework\App\ResponseInterface|\PHPUnit_Framework_MockObject_MockObject */
-    protected $response;
+    /**
+     * @var \Magento\Framework\App\RequestInterface|MockObject
+     */
+    private $request;
 
-    /** @var \Magento\Framework\App\RequestInterface|\PHPUnit_Framework_MockObject_MockObject */
-    protected $request;
+    /**
+     * @var ParametersInterface|MockObject
+     */
+    private $requestQuery;
 
-    /** @var \Magento\UrlRewrite\Model\UrlFinderInterface|\PHPUnit_Framework_MockObject_MockObject */
-    protected $urlFinder;
+    /**
+     * @var \Magento\UrlRewrite\Model\UrlFinderInterface|MockObject
+     */
+    private $urlFinder;
 
     /**
-     * @return void
+     * @inheritDoc
      */
     protected function setUp()
     {
         $objectManager = new ObjectManager($this);
         $this->actionFactory = $this->createMock(\Magento\Framework\App\ActionFactory::class);
-        $this->url = $this->createMock(\Magento\Framework\UrlInterface::class);
+        $this->url = $this->createMock(UrlInterface::class);
         $this->storeManager = $this->createMock(\Magento\Store\Model\StoreManagerInterface::class);
         $this->response = $this->createPartialMock(
             \Magento\Framework\App\ResponseInterface::class,
             ['setRedirect', 'sendResponse']
         );
+        $this->requestQuery = $this->createMock(ParametersInterface::class);
         $this->request = $this->getMockBuilder(\Magento\Framework\App\Request\Http::class)
             ->disableOriginalConstructor()->getMock();
+        $this->request->method('getQuery')->willReturn($this->requestQuery);
         $this->urlFinder = $this->createMock(\Magento\UrlRewrite\Model\UrlFinderInterface::class);
         $this->store = $this->getMockBuilder(
-            \Magento\Store\Model\Store::class
+            Store::class
         )->disableOriginalConstructor()->getMock();
 
         $this->router = $objectManager->getObject(
@@ -166,17 +193,17 @@ public function testNoRewriteAfterStoreSwitcherWhenNoOldRewrite()
         $this->request->expects($this->any())->method('getPathInfo')->will($this->returnValue('request-path'));
         $this->request->expects($this->any())->method('getParam')->with('___from_store')
             ->will($this->returnValue('old-store'));
-        $oldStore = $this->getMockBuilder(\Magento\Store\Model\Store::class)->disableOriginalConstructor()->getMock();
+        $oldStore = $this->getMockBuilder(Store::class)->disableOriginalConstructor()->getMock();
         $this->storeManager->expects($this->any())->method('getStore')
             ->will($this->returnValueMap([['old-store', $oldStore], [null, $this->store]]));
         $oldStore->expects($this->any())->method('getId')->will($this->returnValue('old-store-id'));
         $this->store->expects($this->any())->method('getId')->will($this->returnValue('current-store-id'));
-        $oldUrlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $oldUrlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $oldUrlRewrite->expects($this->any())->method('getEntityType')->will($this->returnValue('entity-type'));
         $oldUrlRewrite->expects($this->any())->method('getEntityId')->will($this->returnValue('entity-id'));
         $oldUrlRewrite->expects($this->any())->method('getRequestPath')->will($this->returnValue('request-path'));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getRequestPath')->will($this->returnValue('request-path'));
 
@@ -191,17 +218,17 @@ public function testNoRewriteAfterStoreSwitcherWhenOldRewriteEqualsToNewOne()
         $this->request->expects($this->any())->method('getPathInfo')->will($this->returnValue('request-path'));
         $this->request->expects($this->any())->method('getParam')->with('___from_store')
             ->will($this->returnValue('old-store'));
-        $oldStore = $this->getMockBuilder(\Magento\Store\Model\Store::class)->disableOriginalConstructor()->getMock();
+        $oldStore = $this->getMockBuilder(Store::class)->disableOriginalConstructor()->getMock();
         $this->storeManager->expects($this->any())->method('getStore')
             ->will($this->returnValueMap([['old-store', $oldStore], [null, $this->store]]));
         $oldStore->expects($this->any())->method('getId')->will($this->returnValue('old-store-id'));
         $this->store->expects($this->any())->method('getId')->will($this->returnValue('current-store-id'));
-        $oldUrlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $oldUrlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $oldUrlRewrite->expects($this->any())->method('getEntityType')->will($this->returnValue('entity-type'));
         $oldUrlRewrite->expects($this->any())->method('getEntityId')->will($this->returnValue('entity-id'));
         $oldUrlRewrite->expects($this->any())->method('getRequestPath')->will($this->returnValue('old-request-path'));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getRequestPath')->will($this->returnValue('old-request-path'));
 
@@ -234,7 +261,7 @@ public function testNoRewriteAfterStoreSwitcherWhenOldRewriteEqualsToNewOne()
     public function testMatchWithRedirect()
     {
         $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getRedirectType')->will($this->returnValue('redirect-code'));
         $urlRewrite->expects($this->any())->method('getTargetPath')->will($this->returnValue('target-path'));
@@ -256,7 +283,7 @@ public function testMatchWithRedirect()
     public function testMatchWithCustomInternalRedirect()
     {
         $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getEntityType')->will($this->returnValue('custom'));
         $urlRewrite->expects($this->any())->method('getRedirectType')->will($this->returnValue('redirect-code'));
@@ -278,7 +305,7 @@ public function testMatchWithCustomInternalRedirect()
     public function testMatchWithCustomExternalRedirect($targetPath)
     {
         $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getEntityType')->will($this->returnValue('custom'));
         $urlRewrite->expects($this->any())->method('getRedirectType')->will($this->returnValue('redirect-code'));
@@ -310,18 +337,68 @@ public function externalRedirectTargetPathDataProvider()
     public function testMatch()
     {
         $this->storeManager->expects($this->any())->method('getStore')->will($this->returnValue($this->store));
-        $urlRewrite = $this->getMockBuilder(\Magento\UrlRewrite\Service\V1\Data\UrlRewrite::class)
+        $urlRewrite = $this->getMockBuilder(UrlRewrite::class)
             ->disableOriginalConstructor()->getMock();
         $urlRewrite->expects($this->any())->method('getRedirectType')->will($this->returnValue(0));
         $urlRewrite->expects($this->any())->method('getTargetPath')->will($this->returnValue('target-path'));
         $urlRewrite->expects($this->any())->method('getRequestPath')->will($this->returnValue('request-path'));
         $this->urlFinder->expects($this->any())->method('findOneByData')->will($this->returnValue($urlRewrite));
         $this->request->expects($this->once())->method('setPathInfo')->with('/target-path');
         $this->request->expects($this->once())->method('setAlias')
-            ->with(\Magento\Framework\UrlInterface::REWRITE_REQUEST_PATH_ALIAS, 'request-path');
+            ->with(UrlInterface::REWRITE_REQUEST_PATH_ALIAS, 'request-path');
         $this->actionFactory->expects($this->once())->method('create')
             ->with(\Magento\Framework\App\Action\Forward::class);
 
         $this->router->match($this->request);
     }
+
+    /**
+     * Test to match corresponding URL Rewrite on request with query params
+     *
+     * @param string $originalRequestPath
+     * @param string $requestPath
+     * @param int $countOfQueryParams
+     * @dataProvider matchWithQueryParamsDataProvider
+     */
+    public function testMatchWithQueryParams(string $originalRequestPath, string $requestPath, int $countOfQueryParams)
+    {
+        $targetPath = 'target-path';
+
+        $this->storeManager->method('getStore')->willReturn($this->store);
+        $urlRewrite = $this->createMock(UrlRewrite::class);
+        $urlRewrite->method('getRedirectType')->willReturn(0);
+        $urlRewrite->method('getTargetPath')->willReturn($targetPath);
+        $urlRewrite->method('getRequestPath')->willReturn($requestPath);
+        $this->urlFinder->method('findOneByData')
+            ->with([UrlRewrite::REQUEST_PATH => $requestPath, UrlRewrite::STORE_ID => $this->store->getId()])
+            ->willReturn($urlRewrite);
+
+        $this->requestQuery->method('count')->willReturn($countOfQueryParams);
+        $this->request->method('getPathInfo')
+            ->willReturn($originalRequestPath);
+        $this->request->expects($this->once())
+            ->method('setPathInfo')
+            ->with('/' . $targetPath);
+        $this->request->expects($this->once())
+            ->method('setAlias')
+            ->with(UrlInterface::REWRITE_REQUEST_PATH_ALIAS, $requestPath);
+        $this->actionFactory->expects($this->once())
+            ->method('create')
+            ->with(Forward::class);
+
+        $this->router->match($this->request);
+    }
+
+    /**
+     * Data provider for Test to match corresponding URL Rewrite on request with query params
+     *
+     * @return array
+     */
+    public function matchWithQueryParamsDataProvider(): array
+    {
+        return [
+            ['/category.html/', 'category.html/', 0],
+            ['/category.html/', 'category.html', 1],
+        ];
+    }
 }