From deca23e1caf47e783a99722b7d2ce941bd829bc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:15:58 +0200 Subject: [PATCH 01/11] Remove the `pageType` from `\wcf\system\request\Request` This value is currently unused, incorrectly documented (`page` was a valid value since the addition of CMS pages), of questionable value and will break with the addition of PSR-15 controllers. --- .../system/request/ControllerMap.class.php | 12 ++++------- .../lib/system/request/Request.class.php | 20 +------------------ .../system/request/RequestHandler.class.php | 3 --- 3 files changed, 5 insertions(+), 30 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/ControllerMap.class.php b/wcfsetup/install/files/lib/system/request/ControllerMap.class.php index ac0b7aa7710..ca63a1b467a 100644 --- a/wcfsetup/install/files/lib/system/request/ControllerMap.class.php +++ b/wcfsetup/install/files/lib/system/request/ControllerMap.class.php @@ -68,7 +68,7 @@ protected function init() * @param string $controller url controller * @param bool $isAcpRequest true if this is an ACP request * @param bool $skipCustomUrls true if custom url resolution should be suppressed, is always true for ACP requests - * @return mixed array containing className, controller and pageType or a string containing the controller name for aliased controllers + * @return mixed array containing className and controller or a string containing the controller name for aliased controllers * @throws SystemException */ public function resolve($application, $controller, $isAcpRequest, $skipCustomUrls = false) @@ -167,7 +167,6 @@ public function resolveCustomController($application, $controller) return [ 'className' => CmsPage::class, 'controller' => 'cms', - 'pageType' => 'page', // CMS page meta data 'cmsPageID' => $matches['pageID'], @@ -179,7 +178,6 @@ public function resolveCustomController($application, $controller) return [ 'className' => $data, 'controller' => $matches[1], - 'pageType' => \strtolower($matches[2]), ]; } } @@ -399,7 +397,7 @@ public function getApplicationOverride($application, $controller) * @param string $application application identifier * @param string $controller controller name * @param bool $isAcpRequest true if this is an ACP request - * @return string[]|null className, controller and pageType, or null if this is not a legacy controller name + * @return string[]|null className and controller, or null if this is not a legacy controller name */ protected function getLegacyClassData($application, $controller, $isAcpRequest) { @@ -407,11 +405,10 @@ protected function getLegacyClassData($application, $controller, $isAcpRequest) if (isset($this->ciControllers['lookup'][$application][$environment][$controller])) { $className = $this->ciControllers['lookup'][$application][$environment][$controller]; - if (\preg_match('~\\\\(?P[^\\\\]+)(?PAction|Form|Page)$~', $className, $matches)) { + if (\preg_match('~\\\\(?P[^\\\\]+)(Action|Form|Page)$~', $className, $matches)) { return [ 'className' => $className, 'controller' => $matches['controller'], - 'pageType' => \strtolower($matches['pageType']), ]; } } @@ -427,7 +424,7 @@ protected function getLegacyClassData($application, $controller, $isAcpRequest) * @param string $controller controller name * @param bool $isAcpRequest true if this is an ACP request * @param string $pageType page type, e.g. 'form' or 'action' - * @return string[]|null className, controller and pageType + * @return string[]|null className and controller */ protected function getClassData($application, $controller, $isAcpRequest, $pageType) { @@ -453,7 +450,6 @@ protected function getClassData($application, $controller, $isAcpRequest, $pageT return [ 'className' => $className, 'controller' => $controller, - 'pageType' => $pageType, ]; } diff --git a/wcfsetup/install/files/lib/system/request/Request.class.php b/wcfsetup/install/files/lib/system/request/Request.class.php index d20c144faea..64dfdabe24d 100644 --- a/wcfsetup/install/files/lib/system/request/Request.class.php +++ b/wcfsetup/install/files/lib/system/request/Request.class.php @@ -47,12 +47,6 @@ final class Request implements RequestHandlerInterface */ protected $pageName = ''; - /** - * page type - * @var string - */ - protected $pageType = ''; - /** * request object * @var object @@ -64,15 +58,13 @@ final class Request implements RequestHandlerInterface * * @param string $className fully qualified name * @param string $pageName class name - * @param string $pageType can be 'action', 'form' or 'page' * @param string[] $metaData additional meta data */ - public function __construct($className, $pageName, $pageType, array $metaData) + public function __construct($className, $pageName, array $metaData) { $this->className = $className; $this->metaData = $metaData; $this->pageName = $pageName; - $this->pageType = $pageType; } /** @@ -154,16 +146,6 @@ public function getPageName() return $this->pageName; } - /** - * Returns the page type of this request. - * - * @return string - */ - public function getPageType() - { - return $this->pageType; - } - /** * Returns the current request object. * diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 9b4c719ff3e..f848de4d8f9 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -153,12 +153,10 @@ protected function buildRequest($application) $classData = [ 'className' => $routeData['className'], 'controller' => $routeData['controller'], - 'pageType' => $routeData['pageType'], ]; unset($routeData['className']); unset($routeData['controller']); - unset($routeData['pageType']); } else { if ( $this->isACPRequest() @@ -216,7 +214,6 @@ protected function buildRequest($application) $this->activeRequest = new Request( $classData['className'], $classData['controller'], - $classData['pageType'], $metaData ); From 0899048329e3629ee5467fa4c5540386a35a471c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:30:56 +0200 Subject: [PATCH 02/11] Move local `$controller` variable into a more appropriate "scope" in RequestHandler::buildRequest() The variable is only referenced in the `else`, move it there to make the data flow clearer. --- .../install/files/lib/system/request/RequestHandler.class.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index f848de4d8f9..66eedc99151 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -147,8 +147,6 @@ protected function buildRequest($application) $routeData['controller'] = 'index'; } - $controller = $routeData['controller']; - if (isset($routeData['className'])) { $classData = [ 'className' => $routeData['className'], @@ -158,6 +156,8 @@ protected function buildRequest($application) unset($routeData['className']); unset($routeData['controller']); } else { + $controller = $routeData['controller']; + if ( $this->isACPRequest() && ($controller === 'login' || $controller === 'index') From b6894324908421007b2ac282dcb3d68ac9e00a6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:35:04 +0200 Subject: [PATCH 03/11] Mark RequestHandler as final --- .../install/files/lib/system/request/RequestHandler.class.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 66eedc99151..2ee37d02b33 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -26,11 +26,11 @@ * Handles http requests. * * @author Marcel Werk - * @copyright 2001-2020 WoltLab GmbH + * @copyright 2001-2022 WoltLab GmbH * @license GNU Lesser General Public License * @package WoltLabSuite\Core\System\Request */ -class RequestHandler extends SingletonFactory +final class RequestHandler extends SingletonFactory { /** * active request object From fc7d53d78601990e141e8fe88dc20460a92456f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:36:06 +0200 Subject: [PATCH 04/11] Remove dead branch in RequestHandler::redirect() The `redirect()` method is only called in a single location where `$controller` will also be set to a string. --- .../files/lib/system/request/RequestHandler.class.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 2ee37d02b33..109f9824658 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -241,15 +241,11 @@ protected function buildRequest($application) * Redirects to the actual URL, e.g. controller has been aliased or mistyped (boardlist instead of board-list). * * @param string[] $routeData - * @param string $application - * @param string $controller */ - protected function redirect(array $routeData, $application, $controller = null) + protected function redirect(array $routeData, string $application, string $controller) { $routeData['application'] = $application; - if ($controller !== null) { - $routeData['controller'] = $controller; - } + $routeData['controller'] = $controller; // append the remaining query parameters foreach ($_GET as $key => $value) { From 662b3c73f9cffedca8c9c88397313f4aec243a77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:38:37 +0200 Subject: [PATCH 05/11] Do not take `routeData` by reference in RequestHandler::handleDefaultController() By returning the updated data the data flow becomes clearer. --- .../files/lib/system/request/RequestHandler.class.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 109f9824658..56a999f10a6 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -124,7 +124,7 @@ protected function buildRequest($application) // handle landing page for frontend requests if (!$this->isACPRequest()) { - $this->handleDefaultController($application, $routeData); + $routeData = $this->handleDefaultController($application, $routeData); // check if accessing from the wrong domain (e.g. "www." omitted but domain was configured with) $domainName = ApplicationHandler::getInstance()->getDomainName(); @@ -267,10 +267,10 @@ protected function redirect(array $routeData, string $application, string $contr * @param string[] $routeData * @throws IllegalLinkException */ - protected function handleDefaultController($application, array &$routeData) + protected function handleDefaultController(string $application, array $routeData): array { if (!RouteHandler::getInstance()->isDefaultController()) { - return; + return $routeData; } $data = ControllerMap::getInstance()->lookupDefaultController($application); @@ -308,6 +308,8 @@ protected function handleDefaultController($application, array &$routeData) } $routeData['isDefaultController'] = true; + + return $routeData; } /** From 404b7a426302a6190432d1cfc7368a6261760401 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:40:59 +0200 Subject: [PATCH 06/11] Remove `$pageName` from `wcf\system\request\Request` The value is unused and badly named, because it refers to the controller name and not the name of some page. --- .../lib/system/request/Request.class.php | 20 +------------------ .../system/request/RequestHandler.class.php | 1 - 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/Request.class.php b/wcfsetup/install/files/lib/system/request/Request.class.php index 64dfdabe24d..d12a6607e60 100644 --- a/wcfsetup/install/files/lib/system/request/Request.class.php +++ b/wcfsetup/install/files/lib/system/request/Request.class.php @@ -41,12 +41,6 @@ final class Request implements RequestHandlerInterface */ protected $pageID; - /** - * page name - * @var string - */ - protected $pageName = ''; - /** * request object * @var object @@ -57,14 +51,12 @@ final class Request implements RequestHandlerInterface * Creates a new request object. * * @param string $className fully qualified name - * @param string $pageName class name * @param string[] $metaData additional meta data */ - public function __construct($className, $pageName, array $metaData) + public function __construct($className, array $metaData) { $this->className = $className; $this->metaData = $metaData; - $this->pageName = $pageName; } /** @@ -136,16 +128,6 @@ public function getMetaData() return $this->metaData; } - /** - * Returns the page name of this request. - * - * @return string - */ - public function getPageName() - { - return $this->pageName; - } - /** * Returns the current request object. * diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 56a999f10a6..2a3572ab973 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -213,7 +213,6 @@ protected function buildRequest($application) $this->activeRequest = new Request( $classData['className'], - $classData['controller'], $metaData ); From 3172fd7a32bb783204a5ed740d2432a87abb835f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:43:57 +0200 Subject: [PATCH 07/11] Remove dead stores in RequestHandler::buildRequest() These array fields are never accessed after being unset. --- .../files/lib/system/request/RequestHandler.class.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 2a3572ab973..d8f20d340b1 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -152,9 +152,6 @@ protected function buildRequest($application) 'className' => $routeData['className'], 'controller' => $routeData['controller'], ]; - - unset($routeData['className']); - unset($routeData['controller']); } else { $controller = $routeData['controller']; @@ -206,9 +203,6 @@ protected function buildRequest($application) ) { WCF::setLanguage($routeData['cmsPageLanguageID']); } - - unset($routeData['cmsPageID']); - unset($routeData['cmsPageLanguageID']); } $this->activeRequest = new Request( From c16a2686171f624fff01fc861d0ab8db8d11abf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:46:47 +0200 Subject: [PATCH 08/11] Make ControllerMap::isLandingPage() take a className, not a classData array It is only interested in the className field of the given array and taking a className directly makes the API clearer. --- .../files/lib/system/request/ControllerMap.class.php | 7 +++---- .../files/lib/system/request/RequestHandler.class.php | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/ControllerMap.class.php b/wcfsetup/install/files/lib/system/request/ControllerMap.class.php index ca63a1b467a..29914b761fd 100644 --- a/wcfsetup/install/files/lib/system/request/ControllerMap.class.php +++ b/wcfsetup/install/files/lib/system/request/ControllerMap.class.php @@ -354,17 +354,16 @@ public function isDefaultController($application, $controller) /** * Returns true if currently active request represents the landing page. * - * @param string[] $classData * @param array $metaData * @return bool */ - public function isLandingPage(array $classData, array $metaData) + public function isLandingPage(string $className, array $metaData) { - if ($classData['className'] !== $this->landingPages['wcf'][2]) { + if ($className !== $this->landingPages['wcf'][2]) { return false; } - if ($classData['className'] === CmsPage::class) { + if ($className === CmsPage::class) { // check if page id matches if ($this->landingPages['wcf'][1] !== '__WCF_CMS__' . $metaData['cms']['pageID']) { return false; diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index d8f20d340b1..bdded4e2139 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -212,7 +212,7 @@ protected function buildRequest($application) if (!$this->isACPRequest()) { // determine if current request matches the landing page - if (ControllerMap::getInstance()->isLandingPage($classData, $metaData)) { + if (ControllerMap::getInstance()->isLandingPage($classData['className'], $metaData)) { $this->activeRequest->setIsLandingPage(); } } From d388f6f9b8803e2b110870dc3d40fa64980e9560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 14:51:31 +0200 Subject: [PATCH 09/11] Clean up `$classData` in RequestHandler::buildRequest() Since the recent changes only the `className` member of this array was access. Simplify this to a simple `$className` variable. --- .../files/lib/system/request/RequestHandler.class.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index bdded4e2139..c5ae62979c1 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -148,10 +148,7 @@ protected function buildRequest($application) } if (isset($routeData['className'])) { - $classData = [ - 'className' => $routeData['className'], - 'controller' => $routeData['controller'], - ]; + $className = $routeData['className']; } else { $controller = $routeData['controller']; @@ -186,6 +183,8 @@ protected function buildRequest($application) ); if (\is_string($classData)) { $this->redirect($routeData, $application, $classData); + } else { + $className = $classData['className']; } } @@ -206,13 +205,13 @@ protected function buildRequest($application) } $this->activeRequest = new Request( - $classData['className'], + $className, $metaData ); if (!$this->isACPRequest()) { // determine if current request matches the landing page - if (ControllerMap::getInstance()->isLandingPage($classData['className'], $metaData)) { + if (ControllerMap::getInstance()->isLandingPage($className, $metaData)) { $this->activeRequest->setIsLandingPage(); } } From 624d42f1f6886fc22bb9967a9af611b7fb819b27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 15:01:39 +0200 Subject: [PATCH 10/11] Rewrite condition in RequestHandler::buildRequest() to be more clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It now has an explicit “then” (ACP) and “else” (Frontend) part. --- .../files/lib/system/request/RequestHandler.class.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index c5ae62979c1..43e6ae36a99 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -122,8 +122,12 @@ protected function buildRequest($application) try { $routeData = RouteHandler::getInstance()->getRouteData(); - // handle landing page for frontend requests - if (!$this->isACPRequest()) { + if ($this->isACPRequest()) { + if (empty($routeData['controller'])) { + $routeData['controller'] = 'index'; + } + } else { + // handle landing page for frontend requests $routeData = $this->handleDefaultController($application, $routeData); // check if accessing from the wrong domain (e.g. "www." omitted but domain was configured with) @@ -143,8 +147,6 @@ protected function buildRequest($application) exit; } - } elseif (empty($routeData['controller'])) { - $routeData['controller'] = 'index'; } if (isset($routeData['className'])) { From 5da310cc32d5300cb393dd127ce5ec7abc9315bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 20 May 2022 15:37:09 +0200 Subject: [PATCH 11/11] Enforce that routes return a controller for non-default-controller routes --- .../files/lib/system/request/RequestHandler.class.php | 2 ++ .../files/lib/system/request/RouteHandler.class.php | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php index 43e6ae36a99..61b0146377e 100644 --- a/wcfsetup/install/files/lib/system/request/RequestHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RequestHandler.class.php @@ -122,6 +122,8 @@ protected function buildRequest($application) try { $routeData = RouteHandler::getInstance()->getRouteData(); + \assert(RouteHandler::getInstance()->isDefaultController() || $routeData['controller']); + if ($this->isACPRequest()) { if (empty($routeData['controller'])) { $routeData['controller'] = 'index'; diff --git a/wcfsetup/install/files/lib/system/request/RouteHandler.class.php b/wcfsetup/install/files/lib/system/request/RouteHandler.class.php index bcd213338b9..c84c417226d 100644 --- a/wcfsetup/install/files/lib/system/request/RouteHandler.class.php +++ b/wcfsetup/install/files/lib/system/request/RouteHandler.class.php @@ -144,6 +144,15 @@ public function matches() $this->isDefaultController = $this->routeData['isDefaultController']; unset($this->routeData['isDefaultController']); + if (!isset($this->routeData['controller']) || $this->routeData['controller'] === '') { + if (!$this->isDefaultController()) { + throw new \DomainException(\sprintf( + "Route implementation '%s' is buggy: Matched route is not the default controller, but no controller was returned.", + $route::class + )); + } + } + if (isset($this->routeData['isRenamedController'])) { $this->isRenamedController = $this->routeData['isRenamedController']; unset($this->routeData['isRenamedController']);