[BUGFIX] Make redirect sources with Query matchable 22/58522/6
authorDaniel Goerz <daniel.goerz@posteo.de>
Mon, 1 Oct 2018 11:26:33 +0000 (13:26 +0200)
committerAndreas Fernandez <a.fernandez@scripting-base.de>
Mon, 1 Oct 2018 17:59:21 +0000 (19:59 +0200)
Resolves: #86503
Releases: master
Change-Id: Iaf924b9f851bd25cb2d16ab5e5656703603a669b
Reviewed-on: https://review.typo3.org/58522
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
typo3/sysext/redirects/Classes/Http/Middleware/RedirectHandler.php
typo3/sysext/redirects/Classes/Service/RedirectCacheService.php
typo3/sysext/redirects/Classes/Service/RedirectService.php
typo3/sysext/redirects/Configuration/TCA/sys_redirect.php
typo3/sysext/redirects/Resources/Private/Language/locallang_db.xlf
typo3/sysext/redirects/Tests/Unit/Service/RedirectServiceTest.php
typo3/sysext/redirects/ext_tables.sql

index 12380f4..0846442 100644 (file)
@@ -51,7 +51,8 @@ class RedirectHandler implements MiddlewareInterface, LoggerAwareInterface
         $port = $request->getUri()->getPort();
         $matchedRedirect = $redirectService->matchRedirect(
             $request->getUri()->getHost() . ($port ? ':' . $port : ''),
-            $request->getUri()->getPath()
+            $request->getUri()->getPath(),
+            $request->getUri()->getQuery() ?? ''
         );
 
         // If the matched redirect is found, resolve it, and check further
index ec3783a..9e563b1 100644 (file)
@@ -60,7 +60,7 @@ class RedirectCacheService
     }
 
     /**
-     * Rebuilds the cache for all redirects, grouped by host and by regular expressions.
+     * Rebuilds the cache for all redirects, grouped by host as well as by regular expressions and respect_query_parameters.
      * Does not include deleted redirects, but includes the ones with dynamic starttime/endtime.
      */
     public function rebuild(): array
@@ -79,6 +79,8 @@ class RedirectCacheService
             $host = $row['source_host'] ?: '*';
             if ($row['is_regexp']) {
                 $redirects[$host]['regexp'][$row['source_path']][$row['uid']] = $row;
+            } elseif ($row['respect_query_parameters']) {
+                $redirects[$host]['respect_query_parameters'][$row['source_path']][$row['uid']] = $row;
             } else {
                 $redirects[$host]['flat'][rtrim($row['source_path'], '/') . '/'][$row['uid']] = $row;
             }
index 5d62e85..57dd3c4 100644 (file)
@@ -42,9 +42,10 @@ class RedirectService implements LoggerAwareInterface
      *
      * @param string $domain
      * @param string $path
+     * @param string $query
      * @return array|null
      */
-    public function matchRedirect(string $domain, string $path)
+    public function matchRedirect(string $domain, string $path, string $query = '')
     {
         $allRedirects = $this->fetchRedirects();
         // Check if the domain matches, or if there is a
@@ -55,10 +56,22 @@ class RedirectService implements LoggerAwareInterface
             }
 
             $possibleRedirects = [];
-            // match if a flat redirect matches
+            // check if a flat redirect matches
             if (!empty($allRedirects[$domainName]['flat'][rtrim($path, '/') . '/'])) {
                 $possibleRedirects = $allRedirects[$domainName]['flat'][rtrim($path, '/') . '/'];
             }
+            // check if a flat redirect matches with the Query applied
+            if (!empty($query)) {
+                $pathWithQuery = rtrim($path, '/') . '?' . ltrim($query, '?');
+                if (!empty($allRedirects[$domainName]['respect_query_parameters'][$pathWithQuery])) {
+                    $possibleRedirects = $allRedirects[$domainName]['respect_query_parameters'][$pathWithQuery];
+                } else {
+                    $pathWithQueryAndSlash = rtrim($path, '/') . '/?' . ltrim($query, '?');
+                    if (!empty($allRedirects[$domainName]['respect_query_parameters'][$pathWithQueryAndSlash])) {
+                        $possibleRedirects = $allRedirects[$domainName]['respect_query_parameters'][$pathWithQueryAndSlash];
+                    }
+                }
+            }
             // check all redirects that are registered as regex
             if (!empty($allRedirects[$domainName]['regexp'])) {
                 $allRegexps = array_keys($allRedirects[$domainName]['regexp']);
index feeefd2..578db56 100644 (file)
@@ -28,7 +28,7 @@ return [
         'searchFields' => 'source_host,source_path,target,target_statuscode',
     ],
     'interface' => [
-        'showRecordFieldList' => 'disabled, source_host, source_path, is_regexp, force_https, keep_query_parameters, target, target_statuscode, hitcount, lasthiton, disable_hitcount',
+        'showRecordFieldList' => 'disabled, source_host, source_path, respect_query_parameters, is_regexp, force_https, keep_query_parameters, target, target_statuscode, hitcount, lasthiton, disable_hitcount',
     ],
     'types' => [
         '1' => [
@@ -43,7 +43,7 @@ return [
             'showitem' => 'disabled, --linebreak--, starttime, endtime'
         ],
         'source' => [
-            'showitem' => 'source_host, --linebreak--, source_path, is_regexp'
+            'showitem' => 'source_host, --linebreak--, source_path, respect_query_parameters, is_regexp'
         ],
         'targetdetails' => [
             'showitem' => 'target, target_statuscode, --linebreak--, force_https, keep_query_parameters'
@@ -143,6 +143,21 @@ return [
                 ],
             ]
         ],
+        'respect_query_parameters' => [
+            'exclude' => true,
+            'label' => 'LLL:EXT:redirects/Resources/Private/Language/locallang_db.xlf:sys_redirect.respect_query_parameters.0',
+            'config' => [
+                'type' => 'check',
+                'renderType' => 'checkboxToggle',
+                'default' => 0,
+                'items' => [
+                    [
+                        0 => '',
+                        1 => '',
+                    ]
+                ],
+            ]
+        ],
         'target' => [
             'label' => 'LLL:EXT:redirects/Resources/Private/Language/locallang_db.xlf:sys_redirect.target',
             'config' => [
index 07761d0..fd1d051 100644 (file)
                        <trans-unit id="sys_redirect.keep_query_parameters.0">
                                <source>Keep GET Parameters</source>
                        </trans-unit>
+                       <trans-unit id="sys_redirect.respect_query_parameters">
+                               <source>GET Parameters (source)</source>
+                       </trans-unit>
+                       <trans-unit id="sys_redirect.respect_query_parameters.0">
+                               <source>Respect GET Parameters</source>
+                       </trans-unit>
                        <trans-unit id="sys_redirect.target">
                                <source>Target</source>
                        </trans-unit>
index b38ab17..a25e89a 100644 (file)
@@ -104,6 +104,187 @@ class RedirectServiceTest extends UnitTestCase
     /**
      * @test
      */
+    public function matchRedirectReturnsRedirectOnRespectQueryParametersMatch()
+    {
+        $row = [
+            'target' => 'https://example.com',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '1',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $this->redirectCacheServiceProphecy->getRedirects()->willReturn(
+            [
+                'example.com' => [
+                    'respect_query_parameters' => [
+                        'index.php?id=123' => [
+                            1 => $row,
+                        ],
+                    ],
+                ],
+            ]
+        );
+        GeneralUtility::addInstance(RedirectCacheService::class, $this->redirectCacheServiceProphecy->reveal());
+
+        $result = $this->redirectService->matchRedirect('example.com', 'index.php', 'id=123');
+
+        self::assertSame($row, $result);
+    }
+
+    /**
+     * @test
+     */
+    public function matchRedirectReturnsRedirectOnRespectQueryParametersMatchWithSlash()
+    {
+        $row = [
+            'target' => 'https://example.com',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '1',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $this->redirectCacheServiceProphecy->getRedirects()->willReturn(
+            [
+                'example.com' => [
+                    'respect_query_parameters' => [
+                        'index.php/?id=123' => [
+                            1 => $row,
+                        ],
+                    ],
+                ],
+            ]
+        );
+        GeneralUtility::addInstance(RedirectCacheService::class, $this->redirectCacheServiceProphecy->reveal());
+
+        $result = $this->redirectService->matchRedirect('example.com', 'index.php', 'id=123');
+
+        self::assertSame($row, $result);
+    }
+
+    /**
+     * @test
+     */
+    public function matchRedirectReturnsRedirectOnFullRespectQueryParametersMatch()
+    {
+        $row = [
+            'target' => 'https://example.com/target',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '1',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $this->redirectCacheServiceProphecy->getRedirects()->willReturn(
+            [
+                'example.com' => [
+                    'respect_query_parameters' => [
+                        'index.php?id=123&a=b' => [
+                            1 => $row,
+                        ],
+                    ],
+                ],
+            ]
+        );
+        GeneralUtility::addInstance(RedirectCacheService::class, $this->redirectCacheServiceProphecy->reveal());
+
+        $result = $this->redirectService->matchRedirect('example.com', 'index.php', 'id=123&a=b');
+
+        self::assertSame($row, $result);
+    }
+
+    /**
+     * @test
+     */
+    public function matchRedirectReturnsNullOnPartialRespectQueryParametersMatch()
+    {
+        $row = [
+            'target' => 'https://example.com/target',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '1',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $this->redirectCacheServiceProphecy->getRedirects()->willReturn(
+            [
+                'example.com' => [
+                    'respect_query_parameters' => [
+                        'index.php?id=123&a=b' => [
+                            1 => $row,
+                        ],
+                    ],
+                ],
+            ]
+        );
+        GeneralUtility::addInstance(RedirectCacheService::class, $this->redirectCacheServiceProphecy->reveal());
+
+        $result = $this->redirectService->matchRedirect('example.com', 'index.php', 'id=123&a=a');
+
+        self::assertSame(null, $result);
+    }
+
+    /**
+     * @test
+     */
+    public function matchRedirectReturnsMatchingRedirectWithMatchingQueryParametersOverMatchingPath()
+    {
+        $row1 = [
+            'target' => 'https://example.com/no-promotion',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '0',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $row2 = [
+            'target' => 'https://example.com/promotion',
+            'force_https' => '0',
+            'keep_query_parameters' => '0',
+            'respect_query_parameters' => '1',
+            'target_statuscode' => '307',
+            'disabled' => '0',
+            'starttime' => '0',
+            'endtime' => '0'
+        ];
+        $this->redirectCacheServiceProphecy->getRedirects()->willReturn(
+            [
+                'example.com' => [
+                    'flat' => [
+                        'special/page/' =>
+                        [
+                            1 => $row1,
+                        ]
+                    ],
+                    'respect_query_parameters' => [
+                        'special/page?key=998877' => [
+                            1 => $row2,
+                        ],
+                    ],
+                ],
+            ]
+        );
+        GeneralUtility::addInstance(RedirectCacheService::class, $this->redirectCacheServiceProphecy->reveal());
+
+        $result = $this->redirectService->matchRedirect('example.com', 'special/page', 'key=998877');
+
+        self::assertSame($row2, $result);
+    }
+
+    /**
+     * @test
+     */
     public function matchRedirectReturnsRedirectSpecificToDomainOnFlatMatchIfSpecificAndNonSpecificExist()
     {
         $row1 = [
index 5b6d631..e5dc3fc 100644 (file)
@@ -7,6 +7,7 @@ CREATE TABLE sys_redirect (
        is_regexp tinyint(1) unsigned DEFAULT '0' NOT NULL,
 
        force_https tinyint(1) unsigned DEFAULT '0' NOT NULL,
+       respect_query_parameters tinyint(1) unsigned DEFAULT '0' NOT NULL,
        keep_query_parameters tinyint(1) unsigned DEFAULT '0' NOT NULL,
        target varchar(255) DEFAULT '' NOT NULL,
        target_statuscode int(11) DEFAULT '307' NOT NULL,