From 3b9056fc014d1a9432aa1142174904ac6c1f8e08 Mon Sep 17 00:00:00 2001 From: Daniel Hensby Date: Sun, 4 May 2014 14:34:58 +0100 Subject: [PATCH 1/2] NEW Cookie_Backend for managing cookie state I've decoupled `Cookie` from the actual act of setting and getting cookies. Currently there are a few limitations to how Cookie works that this change mitigates: 0. `Cookie` currently changes the super global `$_COOKIE` when setting to make the state of an application a bit more managable, but this is bad because we shouldn't be modifying super globals 0. One can't actually change the `$cookie_class` once the `Cookie::$inst` has been instantiated 0. One can't test cookies as there is no class that holds the state of the cookies (it's just held in the super global which is reset as part of `Director::test()` 0. One can't tell the origin of a cookie (eg: did the application set it and it needs to be sent, or did we receive it from the browser?) 0. `time()` was used, so testing was made difficult 0. There was no way to get all the cookies at once (without accessing the super global) Todos are on the phpdoc and I'd like to write some tests for the backend as well as update the docs (if there are any) around cookies. DOCS Adding `Cookie` docs Explains basic usage of `Cookie` as well as how the `Cookie_Backend` controls the setting and getting of cookies and manages state of sent vs received cookies Fixing `Cookie` usage `Cookie` is being used inconsistently with the API throughout framework. Either by not using `force_expiry` to expire cookies or setting them to null and then expiring them (which is redundant). NEW `Director::test()` takes `Cookie_Backend` rather than `array` for `$cookies` param --- control/Cookie.php | 92 ++++--------------- control/CookieJar.php | 156 ++++++++++++++++++++++++++++++++ control/Cookie_Backend.php | 59 ++++++++++++ control/Director.php | 9 +- control/Session.php | 1 + control/injector/Injector.php | 11 ++- dev/TestSession.php | 24 ++++- docs/en/reference/cookies.md | 128 ++++++++++++++++++++++++++ model/DB.php | 4 +- model/Versioned.php | 8 +- security/Member.php | 2 - tests/control/CookieJarTest.php | 135 +++++++++++++++++++++++++++ tests/control/CookieTest.php | 104 +++++++++++++++++++++ tests/control/DirectorTest.php | 15 ++- 14 files changed, 654 insertions(+), 94 deletions(-) create mode 100644 control/CookieJar.php create mode 100644 control/Cookie_Backend.php create mode 100644 docs/en/reference/cookies.md create mode 100644 tests/control/CookieJarTest.php create mode 100644 tests/control/CookieTest.php diff --git a/control/Cookie.php b/control/Cookie.php index 8c0adf388..6c6ade4f9 100644 --- a/control/Cookie.php +++ b/control/Cookie.php @@ -14,17 +14,19 @@ class Cookie { private static $report_errors = true; /** - * @var string cookie class + * Fetch the current instance of the cookie backend + * + * @return Cookie_Backend The cookie backend */ - static $cookie_class = 'Cookie'; - - private static $inst = null; - public static function get_inst() { - if(is_null(self::$inst)) { - self::$inst = new self::$cookie_class(); + //if we don't have a CookieJar service yet, register it + if(!Injector::inst()->hasService('Cookie_Backend')) { + Injector::inst()->registerService( + Injector::inst()->create('CookieJar', $_COOKIE), + 'Cookie_Backend' + ); } - return self::$inst; + return Injector::inst()->get('Cookie_Backend'); } /** @@ -41,7 +43,7 @@ class Cookie { public static function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = true ) { - return self::get_inst()->inst_set($name, $value, $expiry, $path, $domain, $secure, $httpOnly); + return self::get_inst()->set($name, $value, $expiry, $path, $domain, $secure, $httpOnly); } /** @@ -51,7 +53,7 @@ class Cookie { * @return mixed */ public static function get($name) { - return self::get_inst()->inst_get($name); + return self::get_inst()->get($name); } /** @@ -59,77 +61,15 @@ class Cookie { * @param string * @param string */ - public static function force_expiry($name, $path = null, $domain = null) { - return self::get_inst()->inst_force_expiry($name, $path, $domain); - } - - /** - * @deprecated 3.2 Use "Cookie.report_errors" config setting instead - * @param bool - */ - public static function set_report_errors($reportErrors) { - Deprecation::notice('3.2', 'Use "Cookie.report_errors" config setting instead'); - self::get_inst()->inst_set_report_errors($reportErrors); - } - - /** - * @deprecated 3.2 Use "Cookie.report_errors" config setting instead - * @return bool - */ - public static function report_errors() { - Deprecation::notice('3.2', 'Use "Cookie.report_errors" config setting instead'); - return self::get_inst()->inst_report_errors(); - } - - /** - * Set a cookie variable - * - * @param string $name The variable name - * @param mixed $value The variable value. - * @param int $expiry The expiry time, in days. Defaults to 90. - * @param string $path See http://php.net/set_session - * @param string $domain See http://php.net/set_session - * @param boolean $secure See http://php.net/set_session - * @param boolean $httpOnly See http://php.net/set_session - */ - protected function inst_set($name, $value, $expiry = 90, $path = null, - $domain = null, $secure = false, $httpOnly = true - ) { - if(!headers_sent($file, $line)) { - $expiry = $expiry > 0 ? time()+(86400*$expiry) : $expiry; - $path = ($path) ? $path : Director::baseURL(); - setcookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly); - $_COOKIE[$name] = $value; - } else { - if(Config::inst()->get('Cookie', 'report_errors')) { - user_error("Cookie '$name' can't be set. The site started outputting content at line $line in $file", - E_USER_WARNING); - } - } - } - - /** - * @param string - * @return mixed - */ - protected function inst_get($name) { - return isset($_COOKIE[$name]) ? $_COOKIE[$name] : null; - } - - /** - * @param string - */ - protected function inst_force_expiry($name, $path = null, $domain = null) { - if(!headers_sent($file, $line)) { - self::set($name, null, -20, $path, $domain); - } + public static function force_expiry($name, $path = null, $domain = null, $secure = false, $httpOnly = false) { + return self::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly); } /** * @deprecated 3.2 Use the "Cookie.report_errors" config setting instead * @param bool */ - protected function inst_set_report_errors($reportErrors) { + protected function set_report_errors($reportErrors) { Deprecation::notice('3.2', 'Use the "Cookie.report_errors" config setting instead'); Config::inst()->update('Cookie', 'report_errors', $reportErrors); } @@ -138,7 +78,7 @@ class Cookie { * @deprecated 3.2 Use the "Cookie.report_errors" config setting instead * @return bool */ - protected function inst_report_errors() { + protected function report_errors() { Deprecation::notice('3.2', 'Use the "Cookie.report_errors" config setting instead'); return Config::inst()->get('Cookie', 'report_errors'); } diff --git a/control/CookieJar.php b/control/CookieJar.php new file mode 100644 index 000000000..7b295cd29 --- /dev/null +++ b/control/CookieJar.php @@ -0,0 +1,156 @@ +current = $this->existing = !empty($cookies) ? $cookies : $this->existing; + } + + /** + * Set a cookie + * + * @param string $name The name of the cookie + * @param string $value The value for the cookie to hold + * @param int $expiry The number of days until expiry; 0 indicates a cookie valid for the current session + * @param string $path The path to save the cookie on (falls back to site base) + * @param string $domain The domain to make the cookie available on + * @param boolean $secure Can the cookie only be sent over SSL? + * @param boolean $httpOnly Prevent the cookie being accessible by JS + */ + public function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = true) { + //are we setting or clearing a cookie? false values are reserved for clearing cookies (see PHP manual) + $clear = false; + if ($value === false || $value === '' || $expiry < 0) { + $clear = true; + $value = false; + } + + //expiry === 0 is a special case where we set a cookie for the current user session + if ($expiry !== 0) { + //don't do the maths if we are clearing + $expiry = $clear ? -1 : SS_Datetime::now()->Format('U') + (86400 * $expiry); + } + //set the path up + $path = $path ? $path : Director::baseURL(); + //send the cookie + $this->outputCookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly); + //keep our variables in check + if ($clear) { + unset ($this->new[$name], $this->current[$name]); + } + else { + $this->new[$name] = $this->current[$name] = $value; + } + + } + + /** + * Get the cookie value by name + * + * @param string $name The name of the cookie to get + * @param boolean $includeUnsent Include cookies we've yet to send when fetching values + * + * @return string|null The cookie value or null if unset + */ + public function get($name, $includeUnsent = true) { + $cookies = $includeUnsent ? $this->current : $this->existing; + if (isset($cookies[$name])) { + return $cookies[$name]; + } + } + + /** + * Get all the cookies + * + * @param boolean $includeUnsent Include cookies we've yet to send + * + * @return array All the cookies + */ + public function getAll($includeUnsent = true) { + return $includeUnsent ? $this->current : $this->existing; + } + + /** + * Force the expiry of a cookie by name + * + * @param string $name The name of the cookie to expire + * @param string $path The path to save the cookie on (falls back to site base) + * @param string $domain The domain to make the cookie available on + * @param boolean $secure Can the cookie only be sent over SSL? + * @param boolean $httpOnly Prevent the cookie being accessible by JS + */ + public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = false) { + $this->set($name, false,-1, $path, $domain, $secure, $httpOnly); + } + + /** + * The function that actually sets the cookie using PHP + * + * @see http://uk3.php.net/manual/en/function.setcookie.php + * + * @param string $name The name of the cookie + * @param string|array $value The value for the cookie to hold + * @param int $expiry The number of days until expiry + * @param string $path The path to save the cookie on (falls back to site base) + * @param string $domain The domain to make the cookie available on + * @param boolean $secure Can the cookie only be sent over SSL? + * @param boolean $httpOnly Prevent the cookie being accessible by JS + * + * @return boolean If the cookie was set or not; doesn't mean it's accepted by the browser + */ + protected function outputCookie( + $name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = false + ) { + // if headers aren't sent, we can set the cookie + if(!headers_sent($file, $line)) { + return setcookie($name, $value, $expiry, $path, $domain, $secure, $httpOnly); + } else if(Config::inst()->get('Cookie', 'report_errors')) { + throw new LogicException( + "Cookie '$name' can't be set. The site started outputting content at line $line in $file" + ); + } + } + +} diff --git a/control/Cookie_Backend.php b/control/Cookie_Backend.php new file mode 100644 index 000000000..ffbf1a7e5 --- /dev/null +++ b/control/Cookie_Backend.php @@ -0,0 +1,59 @@ +create('Session', array()); + if(!$cookies) $cookies = new CookieJar(null); // Back up the current values of the superglobals $existingRequestVars = isset($_REQUEST) ? $_REQUEST : array(); @@ -235,6 +236,7 @@ class Director implements TemplateGlobalProvider { $existingSessionVars = isset($_SESSION) ? $_SESSION : array(); $existingCookies = isset($_COOKIE) ? $_COOKIE : array(); $existingServer = isset($_SERVER) ? $_SERVER : array(); + $existingCookieJar = Cookie::get_inst(); $existingRequirementsBackend = Requirements::backend(); @@ -269,7 +271,8 @@ class Director implements TemplateGlobalProvider { $_GET = (array)$getVars; $_POST = (array)$postVars; $_SESSION = $session ? $session->inst_getAll() : array(); - $_COOKIE = (array) $cookies; + $_COOKIE = $cookies->getAll(false); + Injector::inst()->registerService($cookies, 'CookieJar'); $_SERVER['REQUEST_URI'] = Director::baseURL() . $urlWithQuerystring; $request = new SS_HTTPRequest($httpMethod, $url, $getVars, $postVars, $body); @@ -311,6 +314,8 @@ class Director implements TemplateGlobalProvider { $_COOKIE = $existingCookies; $_SERVER = $existingServer; + Injector::inst()->registerService($existingCookieJar, 'CookieJar'); + Requirements::set_backend($existingRequirementsBackend); // These are needed so that calling Director::test() doesnt muck with whoever is calling it. diff --git a/control/Session.php b/control/Session.php index a20d18222..d7d447d09 100644 --- a/control/Session.php +++ b/control/Session.php @@ -609,6 +609,7 @@ class Session { */ public static function destroy($removeCookie = true) { self::current_session()->inst_destroy($removeCookie); + Cookie::force_expiry(session_name(), $path, null, $secure, true); } /** diff --git a/control/injector/Injector.php b/control/injector/Injector.php index a92ddf205..59b4496b1 100644 --- a/control/injector/Injector.php +++ b/control/injector/Injector.php @@ -740,12 +740,21 @@ class Injector { $this->inject($service); } + /** + * Remove a service that has been registered + * + * @param string $service The service to clear + */ + public function clearService($service) { + unset($this->specs[$service], $this->serviceCache[$service]); + } /** * Register a service with an explicit name * * @deprecated since 3.1.1 */ public function registerNamedService($name, $service) { + Deprecation::notice('3.2', 'registerNamedService is deprecated, use registerService instead'); return $this->registerService($service, $name); } @@ -882,4 +891,4 @@ class Injector { public function createWithArgs($name, $constructorArgs) { return $this->get($name, false, $constructorArgs); } -} +} \ No newline at end of file diff --git a/dev/TestSession.php b/dev/TestSession.php index bb60b1e9b..ee25f61e2 100644 --- a/dev/TestSession.php +++ b/dev/TestSession.php @@ -16,6 +16,7 @@ class TestSession { /** * @var SS_HTTPResponse */ + private $cookies; private $lastResponse; /** @@ -36,6 +37,7 @@ class TestSession { public function __construct() { $this->session = Injector::inst()->create('Session', array()); + $this->cookies = new CookieJar(); $this->controller = new Controller(); $this->controller->setSession($this->session); $this->controller->pushCurrent(); @@ -62,8 +64,15 @@ class TestSession { public function get($url, $session = null, $headers = null, $cookies = null) { $headers = (array) $headers; if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl; - $this->lastResponse - = Director::test($url, null, $session ? $session : $this->session, null, null, $headers, $cookies); + $this->lastResponse = Director::test( + $url, + null, + $session ? $session : $this->session, + null, + null, + $headers, + $cookies ? $cookies : $this->cookies + ); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); return $this->lastResponse; @@ -84,8 +93,15 @@ class TestSession { public function post($url, $data, $headers = null, $session = null, $body = null, $cookies = null) { $headers = (array) $headers; if($this->lastUrl && !isset($headers['Referer'])) $headers['Referer'] = $this->lastUrl; - $this->lastResponse - = Director::test($url, $data, $session ? $session : $this->session, null, $body, $headers, $cookies); + $this->lastResponse = Director::test( + $url, + $data, + $session ? $session : $this->session, + null, + $body, + $headers, + $cookies ? $cookies : $this->cookies + ); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); return $this->lastResponse; diff --git a/docs/en/reference/cookies.md b/docs/en/reference/cookies.md new file mode 100644 index 000000000..691f89163 --- /dev/null +++ b/docs/en/reference/cookies.md @@ -0,0 +1,128 @@ +# Cookies + +## Accessing and Manipulating Cookies + +Cookies can be set/get/expired using the `Cookie` class and its static methods + +setting: + +```php +Cookie::set('CookieName', 'CookieValue'); +``` + +getting: + +```php +Cookie::get('CookieName'); //returns null if not set or the value if set +``` + +expiring / removing / clearing: + +```php +Cookie::force_expiry('CookieName'); +``` + + +## The `Cookie_Backend` + +The `Cookie` class manipulates and sets cookies using a `Cookie_Backend`. The backend is in charge of the logic +that fetches, sets and expires cookies. By default we use a the `CookieJar` backend which uses PHP's +(`setcookie`)[http://www.php.net/manual/en/function.setcookie.php] function. + +The `CookieJar` keeps track of cookies that have been set by the current process as well as those that were recieved +from the browser. + +By default the `Cookie` class will load the `$_COOKIE` superglobal into the `Cookie_Backend`. If you want to change +the initial state of the `Cookie_Backend` you can load your own backend into the `CookieJar` service registered with +the `Injector`. + +eg: + +```php +$myCookies = array( + 'cookie1' => 'value1', +); + +$newBackend = new CookieJar($myCookies); + +Injector::inst()->registerService($newBackend, 'CookieJar'); + +Cookie::get('cookie1'); //will return 'value1' +``` + +### Resetting the Cookie_Backend state + +Assuming that your appliation hasn't messed around with the `$_COOKIE` superglobal, you can reset the state of your +`Cookie_Backend` by simply unregistering the `CookieJar` service with `Injector`. Next time you access `Cookie` it'll +create a new service for you using the `$_COOKIE` superglobal + +eg: + +```php + +Injector::inst()->unregisterNamedObject('CookieJar'); + +Cookie::get('cookiename'); //will return $_COOKIE['cookiename'] if set + +``` + +Alternatively, if you know that the superglobal has been changed (or you aren't sure it hasn't) you can attempt to use +the current `CookieJar` service to tell you what it was like when it was registered. + +eg: + +```php + +//store the cookies that were loaded into the `CookieJar` +$recievedCookie = Cookie::get_inst()->getAll(false); + +//set a new `CookieJar` +Injector::inst()->registerService(new CookieJar($recievedCookie), 'CookieJar'); + +``` + + +### Using your own Cookie_Backend + +If you need to implement your own Cookie_Backend you can use the injector system to force a different class to be used. + +example: + +```yml +Injector: + CookieJar: + class: MyCookieJar +``` + +To be a valid backend your class must implement the `Cookie_Backend` interface. + +## Advanced Usage + +### Sent vs Received Cookies + +Sometimes it's useful to be able to tell if a cookie was set by the process (thus will be sent to the browser) or if it +came from the browser as part of the request. + +Using the `Cookie_Backend` we can do this like such: + +```php + +Cookie::set('CookieName', 'CookieVal'); + +Cookie::get('CookieName'); //gets the cookie as we set it + +//will return the cookie as it was when it was sent in the request +Cookie::get_inst()->get('CookieName', false); +``` + +### Accessing all the cookies at once + +One can also access all of the cookies in one go using the `Cookie_Backend` + +```php + +Cookie::get_inst()->getAll(); //returns all the cookies including ones set during the current process + +Cookie::get_inst()->getAll(false); //returns all the cookies in the request + +``` diff --git a/model/DB.php b/model/DB.php index aad84ef74..5d56efcb8 100644 --- a/model/DB.php +++ b/model/DB.php @@ -162,8 +162,8 @@ class DB { Cookie::set("alternativeDatabaseName", base64_encode($encrypted), 0, null, null, false, true); Cookie::set("alternativeDatabaseNameIv", base64_encode($iv), 0, null, null, false, true); } else { - Cookie::set("alternativeDatabaseName", null, 0, null, null, false, true); - Cookie::set("alternativeDatabaseNameIv", null, 0, null, null, false, true); + Cookie::force_expiry("alternativeDatabaseName", null, null, false, true); + Cookie::force_expiry("alternativeDatabaseNameIv", null, null, false, true); } } diff --git a/model/Versioned.php b/model/Versioned.php index 557f55cf4..99666aa3f 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -1032,15 +1032,13 @@ class Versioned extends DataExtension implements TemplateGlobalProvider { if(!headers_sent() && !Director::is_cli()) { if(Versioned::current_stage() == 'Live') { // clear the cookie if it's set - if(!empty($_COOKIE['bypassStaticCache'])) { - Cookie::set('bypassStaticCache', null, 0, null, null, false, true /* httponly */); - unset($_COOKIE['bypassStaticCache']); + if(Cookie::get('bypassStaticCache')) { + Cookie::force_expiry('bypassStaticCache', null, null, false, true /* httponly */); } } else { // set the cookie if it's cleared - if(empty($_COOKIE['bypassStaticCache'])) { + if(!Cookie::get('bypassStaticCache')) { Cookie::set('bypassStaticCache', '1', 0, null, null, false, true /* httponly */); - $_COOKIE['bypassStaticCache'] = 1; } } } diff --git a/security/Member.php b/security/Member.php index ca5a79479..c9d2d80d3 100644 --- a/security/Member.php +++ b/security/Member.php @@ -394,7 +394,6 @@ class Member extends DataObject implements TemplateGlobalProvider { Cookie::set('alc_enc', $this->ID . ':' . $token, 90, null, null, null, true); } else { $this->RememberLoginToken = null; - Cookie::set('alc_enc', null); Cookie::force_expiry('alc_enc'); } @@ -489,7 +488,6 @@ class Member extends DataObject implements TemplateGlobalProvider { $this->extend('memberLoggedOut'); $this->RememberLoginToken = null; - Cookie::set('alc_enc', null); // // Clear the Remember Me cookie Cookie::force_expiry('alc_enc'); // Switch back to live in order to avoid infinite loops when diff --git a/tests/control/CookieJarTest.php b/tests/control/CookieJarTest.php new file mode 100644 index 000000000..10197589b --- /dev/null +++ b/tests/control/CookieJarTest.php @@ -0,0 +1,135 @@ + 1, + 'cookie2' => 'cookies', + 'cookie3' => 'test', + ); + + $cookieJar = new CookieJar($defaultCookies); + + //make sure all the "recieved" cookies are as expected + $this->assertEquals($defaultCookies, $cookieJar->getAll(false)); + + //make sure there are no "phantom" cookies + $this->assertEquals($defaultCookies, $cookieJar->getAll(false)); + + //check an empty array is accepted + $cookieJar = new CookieJar(array()); + $this->assertEmpty($cookieJar->getAll(false)); + + + //check no argument is accepted + $cookieJar = new CookieJar(); + $this->assertEmpty($cookieJar->getAll(false)); + } + + /** + * Test that we can set and get cookies + */ + public function testSetAndGet() { + $cookieJar = new CookieJar(); + + $this->assertEmpty($cookieJar->get('testCookie')); + + //set a test cookie + $cookieJar->set('testCookie', 'testVal'); + + //make sure it was set + $this->assertEquals('testVal', $cookieJar->get('testCookie')); + + //make sure we can distinguise it from ones that were "existing" + $this->assertEmpty($cookieJar->get('testCookie', false)); + } + + /** + * Test that we can distinguish between vars that were loaded on instantiation + * and those added later + */ + public function testExistingVersusNew() { + //load with a cookie + $cookieJar = new CookieJar(array( + 'cookieExisting' => 'i woz here', + )); + + //set a new cookie + $cookieJar->set('cookieNew', 'i am new'); + + //check we can fetch new and old cookie values + $this->assertEquals('i woz here', $cookieJar->get('cookieExisting')); + $this->assertEquals('i woz here', $cookieJar->get('cookieExisting', false)); + $this->assertEquals('i am new', $cookieJar->get('cookieNew')); + //there should be no original value for the new cookie + $this->assertEmpty($cookieJar->get('cookieNew', false)); + + //change the existing cookie, can we fetch the new and old value + $cookieJar->set('cookieExisting', 'i woz changed'); + + $this->assertEquals('i woz changed', $cookieJar->get('cookieExisting')); + $this->assertEquals('i woz here', $cookieJar->get('cookieExisting', false)); + + //check we can get all cookies + $this->assertEquals(array( + 'cookieExisting' => 'i woz changed', + 'cookieNew' => 'i am new', + ), $cookieJar->getAll()); + + //check we can get all original cookies + $this->assertEquals(array( + 'cookieExisting' => 'i woz here', + ), $cookieJar->getAll(false)); + } + + /** + * Check we can remove cookies and we can access their original values + */ + public function testForceExpiry() { + //load an existing cookie + $cookieJar = new CookieJar(array( + 'cookieExisting' => 'i woz here', + )); + + //make sure it's available + $this->assertEquals('i woz here', $cookieJar->get('cookieExisting')); + + //remove the cookie + $cookieJar->forceExpiry('cookieExisting'); + + //check it's gone + $this->assertEmpty($cookieJar->get('cookieExisting')); + + //check we can get it's original value + $this->assertEquals('i woz here', $cookieJar->get('cookieExisting', false)); + + + //check we can add a new cookie and remove it and it doesn't leave any phantom values + $cookieJar->set('newCookie', 'i am new'); + + //check it's set by not recieved + $this->assertEquals('i am new', $cookieJar->get('newCookie')); + $this->assertEmpty($cookieJar->get('newCookie', false)); + + //remove it + $cookieJar->forceExpiry('newCookie'); + + //check it's neither set nor reveived + $this->assertEmpty($cookieJar->get('newCookie')); + $this->assertEmpty($cookieJar->get('newCookie', false)); + } + +} diff --git a/tests/control/CookieTest.php b/tests/control/CookieTest.php new file mode 100644 index 000000000..2725161ac --- /dev/null +++ b/tests/control/CookieTest.php @@ -0,0 +1,104 @@ +registerService(new CookieJar($_COOKIE), 'Cookie_Backend'); + } + + public function tearDown() { + parent::tearDown(); + Injector::inst()->unregisterNamedObject('Cookie_Backend'); + } + + /** + * Check a new cookie inst will be loaded with the superglobal by default + */ + public function testCheckNewInstTakesSuperglobal() { + //store the superglobal state + $existingCookies = $_COOKIE; + + //set a mock state for the superglobal + $_COOKIE = array( + 'cookie1' => 1, + 'cookie2' => 'cookies', + 'cookie3' => 'test', + ); + + Injector::inst()->unregisterNamedObject('Cookie_Backend'); + + $this->assertEquals($_COOKIE['cookie1'], Cookie::get('cookie1')); + $this->assertEquals($_COOKIE['cookie2'], Cookie::get('cookie2')); + $this->assertEquals($_COOKIE['cookie3'], Cookie::get('cookie3')); + + //for good measure check the CookieJar hasn't stored anything extra + $this->assertEquals($_COOKIE, Cookie::get_inst()->getAll(false)); + + //restore the superglobal state + $_COOKIE = $existingCookies; + } + + /** + * Check we don't mess with super globals when manipulating cookies + * + * State should be managed sperately to the super global + */ + public function testCheckSuperglobalsArentTouched() { + + //store the current state + $before = $_COOKIE; + + //change some cookies + Cookie::set('cookie', 'not me'); + Cookie::force_expiry('cookie2'); + + //assert it hasn't changed + $this->assertEquals($before, $_COOKIE); + + } + + /** + * Check we can actually change a backend + */ + public function testChangeBackend() { + + Cookie::set('test', 'testvalue'); + + $this->assertEquals('testvalue', Cookie::get('test')); + + Injector::inst()->registerService(new CookieJar(array()), 'Cookie_Backend'); + + $this->assertEmpty(Cookie::get('test')); + + } + + /** + * Check we can actually get the backend inst out + */ + public function testGetInst() { + + $inst = new CookieJar(array('test' => 'testvalue')); + + Injector::inst()->registerService($inst, 'Cookie_Backend'); + + $this->assertEquals($inst, Cookie::get_inst()); + + $this->assertEquals('testvalue', Cookie::get('test')); + + } + +} diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 5ec0ee58b..878e2d58a 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -271,8 +271,10 @@ class DirectorTest extends SapphireTest { $_POST = array('somekey' => 'postvalue'); $_COOKIE = array('somekey' => 'cookievalue'); + $cookies = new CookieJar(array('somekey' => 'sometestcookievalue')); + $getresponse = Director::test('errorpage?somekey=sometestgetvalue', array('somekey' => 'sometestpostvalue'), - null, null, null, null, array('somekey' => 'sometestcookievalue')); + null, null, null, null, $cookies); $this->assertEquals('getvalue', $_GET['somekey'], '$_GET reset to original value after Director::test()'); @@ -288,7 +290,16 @@ class DirectorTest extends SapphireTest { foreach(array('return%sValue', 'returnRequestValue', 'returnCookieValue') as $testfunction) { $url = 'DirectorTestRequest_Controller/' . sprintf($testfunction, ucfirst($method)) . '?' . http_build_query($fixture); - $getresponse = Director::test($url, $fixture, null, strtoupper($method), null, null, $fixture); + + $getresponse = Director::test( + $url, + $fixture, + null, + strtoupper($method), + null, + null, + new CookieJar($fixture) + ); $this->assertInstanceOf('SS_HTTPResponse', $getresponse, 'Director::test() returns SS_HTTPResponse'); $this->assertEquals($fixture['somekey'], $getresponse->getBody(), 'Director::test() ' . $testfunction); From 1e612607aa1e837af10189333020a9c4a00dcdae Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 26 Sep 2014 16:01:23 +1200 Subject: [PATCH 2/2] Suggested improvements / test case fixes --- _config/cookie.yml | 5 ++ control/Cookie.php | 49 ++++++--------- control/CookieJar.php | 17 ++--- control/Cookie_Backend.php | 12 ++-- control/Director.php | 62 ++++++++++-------- control/Session.php | 12 +--- control/injector/Injector.php | 10 +-- dev/TestSession.php | 16 +++-- docs/en/reference/cookies.md | 93 ++++++++++++--------------- tests/control/CookieJarTest.php | 2 +- tests/control/CookieTest.php | 108 ++++++++++++++++++++++++++++---- tests/control/DirectorTest.php | 8 ++- tests/injector/InjectorTest.php | 2 +- 13 files changed, 233 insertions(+), 163 deletions(-) create mode 100644 _config/cookie.yml diff --git a/_config/cookie.yml b/_config/cookie.yml new file mode 100644 index 000000000..3d3f0960d --- /dev/null +++ b/_config/cookie.yml @@ -0,0 +1,5 @@ +--- +Name: cookie +--- +Injector: + Cookie_Backend: CookieJar diff --git a/control/Cookie.php b/control/Cookie.php index 6c6ade4f9..76de0e3e0 100644 --- a/control/Cookie.php +++ b/control/Cookie.php @@ -19,13 +19,6 @@ class Cookie { * @return Cookie_Backend The cookie backend */ public static function get_inst() { - //if we don't have a CookieJar service yet, register it - if(!Injector::inst()->hasService('Cookie_Backend')) { - Injector::inst()->registerService( - Injector::inst()->create('CookieJar', $_COOKIE), - 'Cookie_Backend' - ); - } return Injector::inst()->get('Cookie_Backend'); } @@ -47,13 +40,25 @@ class Cookie { } /** - * Get a cookie variable. + * Get the cookie value by name * - * @param string - * @return mixed + * @param string $name The name of the cookie to get + * @param boolean $includeUnsent Include cookies we've yet to send when fetching values + * + * @return string|null The cookie value or null if unset */ - public static function get($name) { - return self::get_inst()->get($name); + public static function get($name, $includeUnsent = true) { + return self::get_inst()->get($name, $includeUnsent); + } + + /** + * Get all the cookies + * + * @param boolean $includeUnsent Include cookies we've yet to send + * @return array All the cookies + */ + public static function get_all($includeUnsent = true) { + return self::get_inst()->getAll($includeUnsent); } /** @@ -61,25 +66,7 @@ class Cookie { * @param string * @param string */ - public static function force_expiry($name, $path = null, $domain = null, $secure = false, $httpOnly = false) { + public static function force_expiry($name, $path = null, $domain = null, $secure = false, $httpOnly = true) { return self::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly); } - - /** - * @deprecated 3.2 Use the "Cookie.report_errors" config setting instead - * @param bool - */ - protected function set_report_errors($reportErrors) { - Deprecation::notice('3.2', 'Use the "Cookie.report_errors" config setting instead'); - Config::inst()->update('Cookie', 'report_errors', $reportErrors); - } - - /** - * @deprecated 3.2 Use the "Cookie.report_errors" config setting instead - * @return bool - */ - protected function report_errors() { - Deprecation::notice('3.2', 'Use the "Cookie.report_errors" config setting instead'); - return Config::inst()->get('Cookie', 'report_errors'); - } } diff --git a/control/CookieJar.php b/control/CookieJar.php index 7b295cd29..8e7211375 100644 --- a/control/CookieJar.php +++ b/control/CookieJar.php @@ -42,10 +42,13 @@ class CookieJar implements Cookie_Backend { * "existing" array. This allows us to distinguish between cookies we recieved * or we set ourselves (and didn't get from the browser) * - * @param array $cookies The existing cookies to load into the cookie jar + * @param array $cookies The existing cookies to load into the cookie jar. + * Omit this to default to $_COOKIE */ - public function __construct(array $cookies = null) { - $this->current = $this->existing = !empty($cookies) ? $cookies : $this->existing; + public function __construct($cookies = array()) { + $this->current = $this->existing = func_num_args() + ? ($cookies ?: array()) // Convert empty values to blank arrays + : $_COOKIE; } /** @@ -105,7 +108,6 @@ class CookieJar implements Cookie_Backend { * Get all the cookies * * @param boolean $includeUnsent Include cookies we've yet to send - * * @return array All the cookies */ public function getAll($includeUnsent = true) { @@ -121,8 +123,8 @@ class CookieJar implements Cookie_Backend { * @param boolean $secure Can the cookie only be sent over SSL? * @param boolean $httpOnly Prevent the cookie being accessible by JS */ - public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = false) { - $this->set($name, false,-1, $path, $domain, $secure, $httpOnly); + public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = true) { + $this->set($name, false, -1, $path, $domain, $secure, $httpOnly); } /** @@ -137,11 +139,10 @@ class CookieJar implements Cookie_Backend { * @param string $domain The domain to make the cookie available on * @param boolean $secure Can the cookie only be sent over SSL? * @param boolean $httpOnly Prevent the cookie being accessible by JS - * * @return boolean If the cookie was set or not; doesn't mean it's accepted by the browser */ protected function outputCookie( - $name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = false + $name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = true ) { // if headers aren't sent, we can set the cookie if(!headers_sent($file, $line)) { diff --git a/control/Cookie_Backend.php b/control/Cookie_Backend.php index ffbf1a7e5..857120254 100644 --- a/control/Cookie_Backend.php +++ b/control/Cookie_Backend.php @@ -14,7 +14,7 @@ interface Cookie_Backend { * * @param array $cookies The existing cookies to load into the cookie jar */ - public function __construct(array $cookies = null); + public function __construct($cookies = array()); /** * Set a cookie @@ -27,23 +27,25 @@ interface Cookie_Backend { * @param boolean $secure Can the cookie only be sent over SSL? * @param boolean $httpOnly Prevent the cookie being accessible by JS */ - public function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = false); + public function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = true); /** * Get the cookie value by name * * @param string $name The name of the cookie to get + * @param boolean $includeUnsent Include cookies we've yet to send when fetching values * * @return string|null The cookie value or null if unset */ - public function get($name); + public function get($name, $includeUnsent = true); /** * Get all the cookies * + * @param boolean $includeUnsent Include cookies we've yet to send * @return array All the cookies */ - public function getAll(); + public function getAll($includeUnsent = true); /** * Force the expiry of a cookie by name @@ -54,6 +56,6 @@ interface Cookie_Backend { * @param boolean $secure Can the cookie only be sent over SSL? * @param boolean $httpOnly Prevent the cookie being accessible by JS */ - public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = false); + public function forceExpiry($name, $path = null, $domain = null, $secure = false, $httpOnly = true); } diff --git a/control/Director.php b/control/Director.php index 6072f34be..ad843c2fd 100644 --- a/control/Director.php +++ b/control/Director.php @@ -207,17 +207,18 @@ class Director implements TemplateGlobalProvider { * GET otherwise. Overwritten by $postVars['_method'] if present. * @param string $body The HTTP body * @param array $headers HTTP headers with key-value pairs - * @param Cookie_Backend $cookies to populate $_COOKIE + * @param array|Cookie_Backend $cookies to populate $_COOKIE * @param HTTP_Request $request The {@see HTTP_Request} object generated as a part of this request * @return SS_HTTPResponse * * @uses getControllerForURL() The rule-lookup logic is handled by this. * @uses Controller::run() Controller::run() handles the page logic for a Director::direct() call. */ - public static function test($url, $postVars = null, $session = null, $httpMethod = null, $body = null, - $headers = null, $cookies = null, &$request = null) { + public static function test($url, $postVars = null, $session = array(), $httpMethod = null, $body = null, + $headers = array(), $cookies = array(), &$request = null) { Config::nest(); + Injector::nest(); // These are needed so that calling Director::test() doesnt muck with whoever is calling it. // Really, it's some inappropriate coupling and should be resolved by making less use of statics @@ -227,7 +228,9 @@ class Director implements TemplateGlobalProvider { if(!$httpMethod) $httpMethod = ($postVars || is_array($postVars)) ? "POST" : "GET"; if(!$session) $session = Injector::inst()->create('Session', array()); - if(!$cookies) $cookies = new CookieJar(null); + $cookieJar = $cookies instanceof Cookie_Backend + ? $cookies + : Injector::inst()->createWithArgs('Cookie_Backend', array($cookies ?: array())); // Back up the current values of the superglobals $existingRequestVars = isset($_REQUEST) ? $_REQUEST : array(); @@ -236,13 +239,35 @@ class Director implements TemplateGlobalProvider { $existingSessionVars = isset($_SESSION) ? $_SESSION : array(); $existingCookies = isset($_COOKIE) ? $_COOKIE : array(); $existingServer = isset($_SERVER) ? $_SERVER : array(); - $existingCookieJar = Cookie::get_inst(); $existingRequirementsBackend = Requirements::backend(); Config::inst()->update('Cookie', 'report_errors', false); Requirements::set_backend(new Requirements_Backend()); + // Set callback to invoke prior to return + $onCleanup = function() use( + $existingRequestVars, $existingGetVars, $existingPostVars, $existingSessionVars, + $existingCookies, $existingServer, $existingRequirementsBackend, $oldStage + ) { + // Restore the superglobals + $_REQUEST = $existingRequestVars; + $_GET = $existingGetVars; + $_POST = $existingPostVars; + $_SESSION = $existingSessionVars; + $_COOKIE = $existingCookies; + $_SERVER = $existingServer; + + Requirements::set_backend($existingRequirementsBackend); + + // These are needed so that calling Director::test() doesnt muck with whoever is calling it. + // Really, it's some inappropriate coupling and should be resolved by making less use of statics + Versioned::reading_stage($oldStage); + + Injector::unnest(); // Restore old CookieJar, etc + Config::unnest(); + }; + if (strpos($url, '#') !== false) { $url = substr($url, 0, strpos($url, '#')); } @@ -271,8 +296,8 @@ class Director implements TemplateGlobalProvider { $_GET = (array)$getVars; $_POST = (array)$postVars; $_SESSION = $session ? $session->inst_getAll() : array(); - $_COOKIE = $cookies->getAll(false); - Injector::inst()->registerService($cookies, 'CookieJar'); + $_COOKIE = $cookieJar->getAll(false); + Injector::inst()->registerService($cookieJar, 'Cookie_Backend'); $_SERVER['REQUEST_URI'] = Director::baseURL() . $urlWithQuerystring; $request = new SS_HTTPRequest($httpMethod, $url, $getVars, $postVars, $body); @@ -283,7 +308,7 @@ class Director implements TemplateGlobalProvider { $model = DataModel::inst(); $output = Injector::inst()->get('RequestProcessor')->preRequest($request, $session, $model); if ($output === false) { - // @TODO Need to NOT proceed with the request in an elegant manner + $onCleanup(); throw new SS_HTTPResponse_Exception(_t('Director.INVALID_REQUEST', 'Invalid request'), 400); } @@ -303,27 +328,12 @@ class Director implements TemplateGlobalProvider { $output = Injector::inst()->get('RequestProcessor')->postRequest($request, $result, $model); if ($output === false) { + $onCleanup(); throw new SS_HTTPResponse_Exception("Invalid response"); } - // Restore the superglobals - $_REQUEST = $existingRequestVars; - $_GET = $existingGetVars; - $_POST = $existingPostVars; - $_SESSION = $existingSessionVars; - $_COOKIE = $existingCookies; - $_SERVER = $existingServer; - - Injector::inst()->registerService($existingCookieJar, 'CookieJar'); - - Requirements::set_backend($existingRequirementsBackend); - - // These are needed so that calling Director::test() doesnt muck with whoever is calling it. - // Really, it's some inappropriate coupling and should be resolved by making less use of statics - Versioned::reading_stage($oldStage); - - Config::unnest(); - + // Return valid response + $onCleanup(); return $result; } diff --git a/control/Session.php b/control/Session.php index d7d447d09..d312f582d 100644 --- a/control/Session.php +++ b/control/Session.php @@ -395,18 +395,11 @@ class Session { public function inst_destroy($removeCookie = true) { if(session_id()) { if($removeCookie) { - $path = Config::inst()->get('Session', 'cookie_path'); - if(!$path) $path = Director::baseURL(); + $path = Config::inst()->get('Session', 'cookie_path') ?: Director::baseURL(); $domain = Config::inst()->get('Session', 'cookie_domain'); $secure = Config::inst()->get('Session', 'cookie_secure'); - if($domain) { - Cookie::set(session_name(), '', null, $path, $domain, $secure, true); - } else { - Cookie::set(session_name(), '', null, $path, null, $secure, true); - } - - unset($_COOKIE[session_name()]); + Cookie::force_expiry(session_name(), $path, $domain, $secure, true); } session_destroy(); @@ -609,7 +602,6 @@ class Session { */ public static function destroy($removeCookie = true) { self::current_session()->inst_destroy($removeCookie); - Cookie::force_expiry(session_name(), $path, null, $secure, true); } /** diff --git a/control/injector/Injector.php b/control/injector/Injector.php index 59b4496b1..7b7fcc3e1 100644 --- a/control/injector/Injector.php +++ b/control/injector/Injector.php @@ -740,21 +740,13 @@ class Injector { $this->inject($service); } - /** - * Remove a service that has been registered - * - * @param string $service The service to clear - */ - public function clearService($service) { - unset($this->specs[$service], $this->serviceCache[$service]); - } /** * Register a service with an explicit name * * @deprecated since 3.1.1 */ public function registerNamedService($name, $service) { - Deprecation::notice('3.2', 'registerNamedService is deprecated, use registerService instead'); + Deprecation::notice('3.1.1', 'registerNamedService is deprecated, use registerService instead'); return $this->registerService($service, $name); } diff --git a/dev/TestSession.php b/dev/TestSession.php index ee25f61e2..d5d44f7c0 100644 --- a/dev/TestSession.php +++ b/dev/TestSession.php @@ -12,11 +12,15 @@ class TestSession { * @var Session */ private $session; + + /** + * @var Cookie_Backend + */ + private $cookies; /** * @var SS_HTTPResponse */ - private $cookies; private $lastResponse; /** @@ -37,7 +41,7 @@ class TestSession { public function __construct() { $this->session = Injector::inst()->create('Session', array()); - $this->cookies = new CookieJar(); + $this->cookies = Injector::inst()->create('Cookie_Backend'); $this->controller = new Controller(); $this->controller->setSession($this->session); $this->controller->pushCurrent(); @@ -67,11 +71,11 @@ class TestSession { $this->lastResponse = Director::test( $url, null, - $session ? $session : $this->session, + $session ?: $this->session, null, null, $headers, - $cookies ? $cookies : $this->cookies + $cookies ?: $this->cookies ); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); @@ -96,11 +100,11 @@ class TestSession { $this->lastResponse = Director::test( $url, $data, - $session ? $session : $this->session, + $session ?: $this->session, null, $body, $headers, - $cookies ? $cookies : $this->cookies + $cookies ?: $this->cookies ); $this->lastUrl = $url; if(!$this->lastResponse) user_error("Director::test($url) returned null", E_USER_WARNING); diff --git a/docs/en/reference/cookies.md b/docs/en/reference/cookies.md index 691f89163..789712f1a 100644 --- a/docs/en/reference/cookies.md +++ b/docs/en/reference/cookies.md @@ -6,28 +6,24 @@ Cookies can be set/get/expired using the `Cookie` class and its static methods setting: -```php -Cookie::set('CookieName', 'CookieValue'); -``` + :::php + Cookie::set('CookieName', 'CookieValue'); getting: -```php -Cookie::get('CookieName'); //returns null if not set or the value if set -``` + :::php + Cookie::get('CookieName'); //returns null if not set or the value if set expiring / removing / clearing: -```php -Cookie::force_expiry('CookieName'); -``` - + :::php + Cookie::force_expiry('CookieName'); ## The `Cookie_Backend` The `Cookie` class manipulates and sets cookies using a `Cookie_Backend`. The backend is in charge of the logic that fetches, sets and expires cookies. By default we use a the `CookieJar` backend which uses PHP's -(`setcookie`)[http://www.php.net/manual/en/function.setcookie.php] function. +[setcookie](http://www.php.net/manual/en/function.setcookie.php) function. The `CookieJar` keeps track of cookies that have been set by the current process as well as those that were recieved from the browser. @@ -38,48 +34,42 @@ the `Injector`. eg: -```php -$myCookies = array( - 'cookie1' => 'value1', -); + :::php + $myCookies = array( + 'cookie1' => 'value1', + ); -$newBackend = new CookieJar($myCookies); + $newBackend = new CookieJar($myCookies); -Injector::inst()->registerService($newBackend, 'CookieJar'); + Injector::inst()->registerService($newBackend, 'Cookie_Backend'); -Cookie::get('cookie1'); //will return 'value1' -``` + Cookie::get('cookie1'); //will return 'value1' ### Resetting the Cookie_Backend state -Assuming that your appliation hasn't messed around with the `$_COOKIE` superglobal, you can reset the state of your +Assuming that your application hasn't messed around with the `$_COOKIE` superglobal, you can reset the state of your `Cookie_Backend` by simply unregistering the `CookieJar` service with `Injector`. Next time you access `Cookie` it'll -create a new service for you using the `$_COOKIE` superglobal +create a new service for you using the `$_COOKIE` superglobal. eg: -```php + :::php + Injector::inst()->unregisterNamedObject('Cookie_Backend'); -Injector::inst()->unregisterNamedObject('CookieJar'); + Cookie::get('cookiename'); // will return $_COOKIE['cookiename'] if set -Cookie::get('cookiename'); //will return $_COOKIE['cookiename'] if set - -``` Alternatively, if you know that the superglobal has been changed (or you aren't sure it hasn't) you can attempt to use the current `CookieJar` service to tell you what it was like when it was registered. eg: -```php + :::php + //store the cookies that were loaded into the `CookieJar` + $recievedCookie = Cookie::get_inst()->getAll(false); -//store the cookies that were loaded into the `CookieJar` -$recievedCookie = Cookie::get_inst()->getAll(false); - -//set a new `CookieJar` -Injector::inst()->registerService(new CookieJar($recievedCookie), 'CookieJar'); - -``` + //set a new `CookieJar` + Injector::inst()->registerService(new CookieJar($recievedCookie), 'CookieJar'); ### Using your own Cookie_Backend @@ -88,11 +78,14 @@ If you need to implement your own Cookie_Backend you can use the injector system example: -```yml -Injector: - CookieJar: - class: MyCookieJar -``` + :::yml + --- + Name: mycookie + After: '#cookie' + --- + Injector: + Cookie_Backend: + class: MyCookieJar To be a valid backend your class must implement the `Cookie_Backend` interface. @@ -100,29 +93,25 @@ To be a valid backend your class must implement the `Cookie_Backend` interface. ### Sent vs Received Cookies -Sometimes it's useful to be able to tell if a cookie was set by the process (thus will be sent to the browser) or if it +Sometimes it's useful to be able to tell if a cookie was set by the process (thus will be sent to the browser) or if it came from the browser as part of the request. Using the `Cookie_Backend` we can do this like such: -```php + :::php + Cookie::set('CookieName', 'CookieVal'); -Cookie::set('CookieName', 'CookieVal'); + Cookie::get('CookieName'); //gets the cookie as we set it -Cookie::get('CookieName'); //gets the cookie as we set it + //will return the cookie as it was when it was sent in the request + Cookie::get('CookieName', false); -//will return the cookie as it was when it was sent in the request -Cookie::get_inst()->get('CookieName', false); -``` ### Accessing all the cookies at once One can also access all of the cookies in one go using the `Cookie_Backend` -```php + :::php + Cookie::get_inst()->getAll(); //returns all the cookies including ones set during the current process -Cookie::get_inst()->getAll(); //returns all the cookies including ones set during the current process - -Cookie::get_inst()->getAll(false); //returns all the cookies in the request - -``` + Cookie::get_inst()->getAll(false); //returns all the cookies in the request diff --git a/tests/control/CookieJarTest.php b/tests/control/CookieJarTest.php index 10197589b..326b2507a 100644 --- a/tests/control/CookieJarTest.php +++ b/tests/control/CookieJarTest.php @@ -27,7 +27,7 @@ class CookieJarTest extends SapphireTest { $this->assertEquals($defaultCookies, $cookieJar->getAll(false)); //make sure there are no "phantom" cookies - $this->assertEquals($defaultCookies, $cookieJar->getAll(false)); + $this->assertEquals($defaultCookies, $cookieJar->getAll(true)); //check an empty array is accepted $cookieJar = new CookieJar(array()); diff --git a/tests/control/CookieTest.php b/tests/control/CookieTest.php index 2725161ac..8f2fdfa79 100644 --- a/tests/control/CookieTest.php +++ b/tests/control/CookieTest.php @@ -4,25 +4,16 @@ class CookieTest extends SapphireTest { private $cookieInst; - public function setUpOnce() { - parent::setUpOnce(); - Injector::nest(); - } - - public function tearDownOnce() { - parent::tearDownOnce(); - //restore the cookie_backend - Injector::unnest(); - } - public function setUp() { parent::setUp(); + Injector::nest(); Injector::inst()->registerService(new CookieJar($_COOKIE), 'Cookie_Backend'); } public function tearDown() { + //restore the cookie_backend + Injector::unnest(); parent::tearDown(); - Injector::inst()->unregisterNamedObject('Cookie_Backend'); } /** @@ -101,4 +92,97 @@ class CookieTest extends SapphireTest { } + /** + * Test that we can set and get cookies + */ + public function testSetAndGet() { + $this->assertEmpty(Cookie::get('testCookie')); + + //set a test cookie + Cookie::set('testCookie', 'testVal'); + + //make sure it was set + $this->assertEquals('testVal', Cookie::get('testCookie')); + + //make sure we can distinguise it from ones that were "existing" + $this->assertEmpty(Cookie::get('testCookie', false)); + } + + /** + * Test that we can distinguish between vars that were loaded on instantiation + * and those added later + */ + public function testExistingVersusNew() { + //load with a cookie + $cookieJar = new CookieJar(array( + 'cookieExisting' => 'i woz here', + )); + Injector::inst()->registerService($cookieJar, 'Cookie_Backend'); + + //set a new cookie + Cookie::set('cookieNew', 'i am new'); + + //check we can fetch new and old cookie values + $this->assertEquals('i woz here', Cookie::get('cookieExisting')); + $this->assertEquals('i woz here', Cookie::get('cookieExisting', false)); + $this->assertEquals('i am new', Cookie::get('cookieNew')); + //there should be no original value for the new cookie + $this->assertEmpty(Cookie::get('cookieNew', false)); + + //change the existing cookie, can we fetch the new and old value + Cookie::set('cookieExisting', 'i woz changed'); + + $this->assertEquals('i woz changed', Cookie::get('cookieExisting')); + $this->assertEquals('i woz here', Cookie::get('cookieExisting', false)); + + //check we can get all cookies + $this->assertEquals(array( + 'cookieExisting' => 'i woz changed', + 'cookieNew' => 'i am new', + ), Cookie::get_all()); + + //check we can get all original cookies + $this->assertEquals(array( + 'cookieExisting' => 'i woz here', + ), Cookie::get_all(false)); + } + + /** + * Check we can remove cookies and we can access their original values + */ + public function testForceExpiry() { + //load an existing cookie + $cookieJar = new CookieJar(array( + 'cookieExisting' => 'i woz here', + )); + Injector::inst()->registerService($cookieJar, 'Cookie_Backend'); + + //make sure it's available + $this->assertEquals('i woz here', Cookie::get('cookieExisting')); + + //remove the cookie + Cookie::force_expiry('cookieExisting'); + + //check it's gone + $this->assertEmpty(Cookie::get('cookieExisting')); + + //check we can get it's original value + $this->assertEquals('i woz here', Cookie::get('cookieExisting', false)); + + + //check we can add a new cookie and remove it and it doesn't leave any phantom values + Cookie::set('newCookie', 'i am new'); + + //check it's set by not recieved + $this->assertEquals('i am new', Cookie::get('newCookie')); + $this->assertEmpty(Cookie::get('newCookie', false)); + + //remove it + Cookie::force_expiry('newCookie'); + + //check it's neither set nor reveived + $this->assertEmpty(Cookie::get('newCookie')); + $this->assertEmpty(Cookie::get('newCookie', false)); + } + } diff --git a/tests/control/DirectorTest.php b/tests/control/DirectorTest.php index 878e2d58a..21090622f 100644 --- a/tests/control/DirectorTest.php +++ b/tests/control/DirectorTest.php @@ -240,6 +240,7 @@ class DirectorTest extends SapphireTest { unset($_SESSION['isLive']); unset($_GET['isTest']); unset($_GET['isDev']); + $_SESSION = $_SESSION ?: array(); // Test isDev=1 $_GET['isDev'] = '1'; @@ -271,7 +272,10 @@ class DirectorTest extends SapphireTest { $_POST = array('somekey' => 'postvalue'); $_COOKIE = array('somekey' => 'cookievalue'); - $cookies = new CookieJar(array('somekey' => 'sometestcookievalue')); + $cookies = Injector::inst()->createWithArgs( + 'Cookie_Backend', + array(array('somekey' => 'sometestcookievalue')) + ); $getresponse = Director::test('errorpage?somekey=sometestgetvalue', array('somekey' => 'sometestpostvalue'), null, null, null, null, $cookies); @@ -298,7 +302,7 @@ class DirectorTest extends SapphireTest { strtoupper($method), null, null, - new CookieJar($fixture) + Injector::inst()->createWithArgs('Cookie_Backend', array($fixture)) ); $this->assertInstanceOf('SS_HTTPResponse', $getresponse, 'Director::test() returns SS_HTTPResponse'); diff --git a/tests/injector/InjectorTest.php b/tests/injector/InjectorTest.php index e410d8619..d3c019c0a 100644 --- a/tests/injector/InjectorTest.php +++ b/tests/injector/InjectorTest.php @@ -558,7 +558,7 @@ class InjectorTest extends SapphireTest { $injector = new Injector(); $service = new stdClass(); - $injector->registerNamedService('NamedService', $service); + $injector->registerService($service, 'NamedService'); $this->assertEquals($service, $injector->get('NamedService')); }