From d1d4375c95a264a6b63cbaefc2c1d12f808bfd82 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Thu, 15 Jun 2017 18:41:44 +1200 Subject: [PATCH] API Refactor $flush into HTPPApplication API Enforce health check in Controller::pushCurrent() API Better global backup / restore Updated Director::test() to use new API --- cli-script.php | 3 +- main.php | 3 +- src/Control/Controller.php | 9 +- src/Control/Director.php | 169 ++++++++---------- src/Control/HTTPRequest.php | 18 +- src/Core/AppKernel.php | 28 ++- src/Core/Application.php | 4 +- src/Core/CoreKernel.php | 2 +- src/Core/HTTPApplication.php | 11 +- src/Core/Kernel.php | 4 +- .../Startup/ErrorControlChainMiddleware.php | 2 +- src/Core/TestKernel.php | 4 +- src/Dev/Install/install.php5 | 5 + src/Dev/SapphireTest.php | 9 +- src/Dev/TestSession.php | 5 +- 15 files changed, 134 insertions(+), 142 deletions(-) diff --git a/cli-script.php b/cli-script.php index 3b2a702cf..867d8777f 100755 --- a/cli-script.php +++ b/cli-script.php @@ -11,10 +11,9 @@ require __DIR__ . '/src/includes/autoload.php'; // Build request and detect flush $request = HTTPRequest::createFromEnvironment(); -$flush = $request->getVar('flush') || strpos($request->getURL(), 'dev/build') === 0; // Default application -$kernel = new AppKernel($flush); +$kernel = new AppKernel(); $app = new HTTPApplication($kernel); $app->addMiddleware(new OutputMiddleware()); $app->handle($request); diff --git a/main.php b/main.php index 8662f7464..f8696d9da 100644 --- a/main.php +++ b/main.php @@ -10,10 +10,9 @@ require __DIR__ . '/src/includes/autoload.php'; // Build request and detect flush $request = HTTPRequest::createFromEnvironment(); -$flush = $request->getVar('flush') || strpos($request->getURL(), 'dev/build') === 0; // Default application -$kernel = new AppKernel($flush); +$kernel = new AppKernel(); $app = new HTTPApplication($kernel); $app->addMiddleware(new OutputMiddleware()); $app->addMiddleware(new ErrorControlChainMiddleware($app, $request)); diff --git a/src/Control/Controller.php b/src/Control/Controller.php index 2ac3febff..0a7233703 100644 --- a/src/Control/Controller.php +++ b/src/Control/Controller.php @@ -154,10 +154,10 @@ class Controller extends RequestHandler implements TemplateGlobalProvider */ protected function beforeHandleRequest(HTTPRequest $request) { - //Push the current controller to protect against weird session issues - $this->pushCurrent(); //Set up the internal dependencies (request, response) $this->setRequest($request); + //Push the current controller to protect against weird session issues + $this->pushCurrent(); $this->setResponse(new HTTPResponse()); //kick off the init functionality $this->doInit(); @@ -588,9 +588,14 @@ class Controller extends RequestHandler implements TemplateGlobalProvider * Pushes this controller onto the stack of current controllers. This means that any redirection, * session setting, or other things that rely on Controller::curr() will now write to this * controller object. + * + * Note: Ensure this controller is assigned a request with a valid session before pushing + * it to the stack. */ public function pushCurrent() { + // Ensure this controller has a valid session + $this->getRequest()->getSession(); array_unshift(self::$controller_stack, $this); } diff --git a/src/Control/Director.php b/src/Control/Director.php index ba61cfbd3..f955f4732 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -3,12 +3,10 @@ namespace SilverStripe\Control; use SilverStripe\CMS\Model\SiteTree; -use SilverStripe\Core\Config\Config; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Kernel; use SilverStripe\Dev\Deprecation; -use SilverStripe\ORM\ArrayLib; use SilverStripe\Versioned\Versioned; use SilverStripe\View\Requirements; use SilverStripe\View\Requirements_Backend; @@ -188,76 +186,82 @@ class Director implements TemplateGlobalProvider $cookies = array(), &$request = null ) { - Config::nest(); - Injector::nest(); + // Build list of cleanup promises + $finally = []; + + /** @var Kernel $kernel */ + $kernel = Injector::inst()->get(Kernel::class); + $kernel->nest(); + $finally[] = function () use ($kernel) { + $kernel->activate(); + }; + + // backup existing vars, and create new vars + $existingVars = static::envToVars(); + $finally[] = function () use ($existingVars) { + static::varsToEnv($existingVars); + }; + $newVars = $existingVars; // These are needed so that calling Director::test() does not muck with whoever is calling it. // Really, it's some inappropriate coupling and should be resolved by making less use of statics. - $oldReadingMode = null; if (class_exists(Versioned::class)) { $oldReadingMode = Versioned::get_reading_mode(); - } - $getVars = array(); - - if (!$httpMethod) { - $httpMethod = ($postVars || is_array($postVars)) ? "POST" : "GET"; + $finally[] = function () use ($oldReadingMode) { + Versioned::set_reading_mode($oldReadingMode); + }; } - if (!$session) { - $session = new Session([]); - } + // Default httpMethod + $newVars['_SERVER']['REQUEST_METHOD'] = $httpMethod + ?: (($postVars || is_array($postVars)) ? "POST" : "GET"); + + // Setup session + $newVars['_SESSION'] = $session instanceof Session + ? $session->getAll() + : ($session ?: []); + + // Setup cookies $cookieJar = $cookies instanceof Cookie_Backend ? $cookies : Injector::inst()->createWithArgs(Cookie_Backend::class, array($cookies ?: [])); - - // Back up the current values of the superglobals - $existingRequestVars = isset($_REQUEST) ? $_REQUEST : array(); - $existingGetVars = isset($_GET) ? $_GET : array(); - $existingPostVars = isset($_POST) ? $_POST : array(); - $existingSessionVars = isset($_SESSION) ? $_SESSION : array(); - $existingCookies = isset($_COOKIE) ? $_COOKIE : array(); - $existingServer = isset($_SERVER) ? $_SERVER : array(); - - $existingRequirementsBackend = Requirements::backend(); - + $newVars['_COOKIE'] = $cookieJar->getAll(false); Cookie::config()->update('report_errors', false); - Requirements::set_backend(Requirements_Backend::create()); + Injector::inst()->registerService($cookieJar, Cookie_Backend::class); - if (strpos($url, '#') !== false) { - $url = substr($url, 0, strpos($url, '#')); - } + // Backup requirements + $existingRequirementsBackend = Requirements::backend(); + Requirements::set_backend(Requirements_Backend::create()); + $finally[] = function () use ($existingRequirementsBackend) { + Requirements::set_backend($existingRequirementsBackend); + }; + + // Strip any hash + $url = strtok($url, '#'); // Handle absolute URLs if (parse_url($url, PHP_URL_HOST)) { $bits = parse_url($url); + // If a port is mentioned in the absolute URL, be sure to add that into the HTTP host - if (isset($bits['port'])) { - $_SERVER['HTTP_HOST'] = $bits['host'].':'.$bits['port']; - } else { - $_SERVER['HTTP_HOST'] = $bits['host']; - } + $newVars['_SERVER']['HTTP_HOST'] = isset($bits['port']) + ? $bits['host'].':'.$bits['port'] + : $bits['host']; } // Ensure URL is properly made relative. // Example: url passed is "/ss31/my-page" (prefixed with BASE_URL), this should be changed to "my-page" $url = self::makeRelative($url); - - $urlWithQuerystring = $url; if (strpos($url, '?') !== false) { list($url, $getVarsEncoded) = explode('?', $url, 2); - parse_str($getVarsEncoded, $getVars); + parse_str($getVarsEncoded, $newVars['_GET']); + } else { + $newVars['_GET'] = []; } + $newVars['_SERVER']['REQUEST_URI'] = Director::baseURL() . $url; - // Replace the super globals with appropriate test values - $_REQUEST = ArrayLib::array_merge_recursive((array) $getVars, (array) $postVars); - $_GET = (array) $getVars; - $_POST = (array) $postVars; - $_SESSION = $session ? $session->getAll() : array(); - $_COOKIE = $cookieJar->getAll(false); - Injector::inst()->registerService($cookieJar, Cookie_Backend::class); - $_SERVER['REQUEST_URI'] = Director::baseURL() . $urlWithQuerystring; - - $request = new HTTPRequest($httpMethod, $url, $getVars, $postVars, $body); + // Create new request + $request = HTTPRequest::createFromVariables($newVars, $body); if ($headers) { foreach ($headers as $k => $v) { $request->addHeader($k, $v); @@ -265,53 +269,13 @@ class Director implements TemplateGlobalProvider } try { - // Pre-request filtering - $requestProcessor = Injector::inst()->get(RequestProcessor::class); - $output = $requestProcessor->preRequest($request); - if ($output === false) { - throw new HTTPResponse_Exception(_t('SilverStripe\\Control\\Director.INVALID_REQUEST', 'Invalid request'), 400); - } - - // Process request - $result = Director::handleRequest($request); - - // Ensure that the result is an HTTPResponse object - if (is_string($result)) { - if (substr($result, 0, 9) == 'redirect:') { - $response = new HTTPResponse(); - $response->redirect(substr($result, 9)); - $result = $response; - } else { - $result = new HTTPResponse($result); - } - } - - $output = $requestProcessor->postRequest($request, $result); - if ($output === false) { - throw new HTTPResponse_Exception("Invalid response"); - } - - // Return valid response - return $result; + // Normal request handling + return static::direct($request); } finally { - // Restore the super globals - $_REQUEST = $existingRequestVars; - $_GET = $existingGetVars; - $_POST = $existingPostVars; - $_SESSION = $existingSessionVars; - $_COOKIE = $existingCookies; - $_SERVER = $existingServer; - - Requirements::set_backend($existingRequirementsBackend); - - // These are needed so that calling Director::test() does not muck with whoever is calling it. - // Really, it's some inappropriate coupling and should be resolved by making less use of statics - if (class_exists(Versioned::class)) { - Versioned::set_reading_mode($oldReadingMode); + // Restore state in reverse order to assignment + foreach (array_reverse($finally) as $callback) { + call_user_func($callback); } - - Injector::unnest(); // Restore old CookieJar, etc - Config::unnest(); } } @@ -372,6 +336,29 @@ class Director implements TemplateGlobalProvider return new HTTPResponse('No URL rule was matched', 404); } + /** + * Extract env vars prior to modification + * + * @return array List of all super globals + */ + public static function envToVars() + { + // Suppress return by-ref + return array_merge($GLOBALS, []); + } + + /** + * Restore a backed up or modified list of vars to $globals + * + * @param array $vars + */ + public static function varsToEnv(array $vars) + { + foreach ($vars as $key => $value) { + $GLOBALS[$key] = $value; + } + } + /** * Return the {@link SiteTree} object that is currently being viewed. If there is no SiteTree * object to return, then this will return the current controller. diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 9eb6b24d6..e1e0debc4 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -2,10 +2,10 @@ namespace SilverStripe\Control; +use ArrayAccess; use BadMethodCallException; use SilverStripe\Core\ClassInfo; use SilverStripe\ORM\ArrayLib; -use ArrayAccess; /** * Represents a HTTP-request, including a URL that is tokenised for parsing, and a request method @@ -159,8 +159,8 @@ class HTTPRequest implements ArrayAccess public static function createFromEnvironment() { // Health-check prior to creating environment - $variables = static::variablesFromEnvironment(); - return self::createFromVariables($variables, @file_get_contents('php://input')); + static::validateEnvironment(); + return self::createFromVariables(Director::envToVars(), @file_get_contents('php://input')); } /** @@ -256,9 +256,8 @@ class HTTPRequest implements ArrayAccess * Error conditions will raise HTTPResponse_Exceptions * * @throws HTTPResponse_Exception - * @return array */ - protected static function variablesFromEnvironment() + protected static function validateEnvironment() { // Validate $_FILES array before merging it with $_POST foreach ($_FILES as $key => $value) { @@ -284,15 +283,6 @@ class HTTPRequest implements ArrayAccess throw new HTTPResponse_Exception('Invalid Host', 400); } } - - return [ - '_SERVER' => $_SERVER, - '_GET' => $_GET, - '_POST' => $_POST, - '_FILES' => $_FILES, - '_SESSION' => isset($_SESSION) ? $_SESSION : null, - '_COOKIE' => $_COOKIE - ]; } /** diff --git a/src/Core/AppKernel.php b/src/Core/AppKernel.php index 0002a7b7f..3f759910f 100644 --- a/src/Core/AppKernel.php +++ b/src/Core/AppKernel.php @@ -28,15 +28,8 @@ use SilverStripe\View\ThemeResourceLoader; class AppKernel extends CoreKernel { - /** - * @var bool - */ - protected $flush = false; - - public function __construct($flush = false) + public function __construct() { - $this->flush = $flush; - // Initialise the dependency injector as soon as possible, as it is // subsequently used by some of the following code $injectorLoader = InjectorLoader::inst(); @@ -134,13 +127,10 @@ class AppKernel extends CoreKernel return null; } - /** - * @throws HTTPResponse_Exception - */ - public function boot() + public function boot($flush = false) { $this->bootPHP(); - $this->bootManifests(); + $this->bootManifests($flush); $this->bootErrorHandling(); $this->bootDatabase(); } @@ -374,17 +364,19 @@ class AppKernel extends CoreKernel /** * Boot all manifests + * + * @param bool $flush */ - protected function bootManifests() + protected function bootManifests($flush) { // Setup autoloader - $this->getClassLoader()->init($this->getIncludeTests(), $this->flush); + $this->getClassLoader()->init($this->getIncludeTests(), $flush); // Find modules - $this->getModuleLoader()->init($this->getIncludeTests(), $this->flush); + $this->getModuleLoader()->init($this->getIncludeTests(), $flush); // Flush config - if ($this->flush) { + if ($flush) { $config = $this->getConfigLoader()->getManifest(); if ($config instanceof CachedConfigCollection) { $config->setFlush(true); @@ -397,7 +389,7 @@ class AppKernel extends CoreKernel // Find default templates $defaultSet = $this->getThemeResourceLoader()->getSet('$default'); if ($defaultSet instanceof ThemeManifest) { - $defaultSet->init($this->getIncludeTests(), $this->flush); + $defaultSet->init($this->getIncludeTests(), $flush); } } diff --git a/src/Core/Application.php b/src/Core/Application.php index c24197e18..deb0d60aa 100644 --- a/src/Core/Application.php +++ b/src/Core/Application.php @@ -18,6 +18,8 @@ interface Application * Invoke the application control chain * * @param callable $callback + * @param bool $flush + * @return mixed */ - public function execute(callable $callback); + public function execute(callable $callback, $flush = false); } diff --git a/src/Core/CoreKernel.php b/src/Core/CoreKernel.php index 7e81e436c..8fbae8bb3 100644 --- a/src/Core/CoreKernel.php +++ b/src/Core/CoreKernel.php @@ -55,7 +55,7 @@ class CoreKernel implements Kernel */ protected $themeResourceLoader = null; - public function boot() + public function boot($flush = false) { } diff --git a/src/Core/HTTPApplication.php b/src/Core/HTTPApplication.php index ff1bcbac7..1a64106c6 100644 --- a/src/Core/HTTPApplication.php +++ b/src/Core/HTTPApplication.php @@ -90,26 +90,29 @@ class HTTPApplication implements Application */ public function handle(HTTPRequest $request) { + $flush = $request->getVar('flush') || strpos($request->getURL(), 'dev/build') === 0; + // Ensure boot is invoked return $this->execute(function () use ($request) { // Start session and execute $request->getSession()->init(); return Director::direct($request); - }); + }, $flush); } /** * Safely boot the application and execute the given main action * * @param callable $callback + * @param bool $flush * @return HTTPResponse */ - public function execute(callable $callback) + public function execute(callable $callback, $flush = false) { try { - return $this->callMiddleware(function () use ($callback) { + return $this->callMiddleware(function () use ($callback, $flush) { // Pre-request boot - $this->getKernel()->boot(); + $this->getKernel()->boot($flush); return call_user_func($callback); }); } finally { diff --git a/src/Core/Kernel.php b/src/Core/Kernel.php index 6541fc8d9..a80a23203 100644 --- a/src/Core/Kernel.php +++ b/src/Core/Kernel.php @@ -32,8 +32,10 @@ interface Kernel /* * Boots the current kernel + * + * @param bool $flush */ - public function boot(); + public function boot($flush = false); /** * Shutdowns the kernel. diff --git a/src/Core/Startup/ErrorControlChainMiddleware.php b/src/Core/Startup/ErrorControlChainMiddleware.php index 87c1b662c..02f875bd6 100644 --- a/src/Core/Startup/ErrorControlChainMiddleware.php +++ b/src/Core/Startup/ErrorControlChainMiddleware.php @@ -90,7 +90,7 @@ class ErrorControlChainMiddleware protected function safeReloadWithToken($reloadToken) { // Safe reload requires manual boot - $this->getApplication()->getKernel()->boot(); + $this->getApplication()->getKernel()->boot(false); // Ensure session is started $this->getRequest()->getSession()->init(); diff --git a/src/Core/TestKernel.php b/src/Core/TestKernel.php index 55e69c02d..fdafbf7ac 100644 --- a/src/Core/TestKernel.php +++ b/src/Core/TestKernel.php @@ -7,10 +7,10 @@ namespace SilverStripe\Core; */ class TestKernel extends AppKernel { - public function __construct($flush = true) + public function __construct() { $this->setEnvironment(self::DEV); - parent::__construct($flush); + parent::__construct(); } /** diff --git a/src/Dev/Install/install.php5 b/src/Dev/Install/install.php5 index 3baad35ed..d3a8a5349 100755 --- a/src/Dev/Install/install.php5 +++ b/src/Dev/Install/install.php5 @@ -9,6 +9,8 @@ ************************************************************************************ ************************************************************************************/ use SilverStripe\Control\Controller; +use SilverStripe\Control\HTTPRequest; +use SilverStripe\Control\Session; use SilverStripe\Core\Startup\ParameterConfirmationToken; use SilverStripe\Dev\Install\DatabaseAdapterRegistry; use SilverStripe\Dev\Install\DatabaseConfigurationHelper; @@ -1503,7 +1505,10 @@ PHP require_once 'Core/Core.php'; // Build database + $request = new HTTPRequest('GET', '/'); + $request->setSession(new Session([])); $con = new Controller(); + $con->setRequest($request); $con->pushCurrent(); global $databaseConfig; diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index 6e5d1d6e8..f1a5d3bb6 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -897,6 +897,9 @@ class SapphireTest extends PHPUnit_Framework_TestCase // Custom application $app->execute(function () use ($request) { + // Start session and execute + $request->getSession()->init(); + // Invalidate classname spec since the test manifest will now pull out new subclasses for each internal class // (e.g. Member will now have various subclasses of DataObjects that implement TestOnly) DataObject::reset(); @@ -906,7 +909,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase $controller->setRequest($request); $controller->pushCurrent(); $controller->doInit(); - }); + }, true); // Register state static::$state = SapphireTestState::singleton(); @@ -1137,7 +1140,9 @@ class SapphireTest extends PHPUnit_Framework_TestCase */ public function logOut() { - Injector::inst()->get(IdentityStore::class)->logOut(); + /** @var IdentityStore $store */ + $store = Injector::inst()->get(IdentityStore::class); + $store->logOut(); } /** diff --git a/src/Dev/TestSession.php b/src/Dev/TestSession.php index 794c3b45d..bd1c88e68 100644 --- a/src/Dev/TestSession.php +++ b/src/Dev/TestSession.php @@ -3,6 +3,7 @@ namespace SilverStripe\Dev; use SilverStripe\Control\Cookie_Backend; +use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\Session; use SilverStripe\Control\Controller; use SilverStripe\Control\Director; @@ -55,8 +56,10 @@ class TestSession { $this->session = Injector::inst()->create(Session::class, array()); $this->cookies = Injector::inst()->create(Cookie_Backend::class); + $request = new HTTPRequest('GET', '/'); + $request->setSession($this->session()); $this->controller = new Controller(); - // @todo - Ensure $this->session is set on all requests + $this->controller->setRequest($request); $this->controller->pushCurrent(); }