Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ENHANCEMENT: Improving Cookie class to allow for extendability #595

Merged
merged 2 commits into from

7 participants

@fatlewis

Previous to this the Cookie class has been very inflexible (cookies are
all set using the static Cookie::set() and so the functionality is not
extendable). Cookie class has been adjusted so extension is now a
possibility for those wishing to alter its functionality. (Improves
compliance to the Law of Demeter)

Matt Lewis added some commits
Matt Lewis ENHANCEMENT: Improving Cookie class to allow for extendability
Previous to this the Cookie class has been very inflexible (cookies are
all set using the static Cookie::set() and so the functionality is not
extendable). Cookie class has been adjusted so extension is now a
possibility for those wishing to alter its functionality. Improves
compliance to the law of demeter.
ebb2458
Matt Lewis MINOR: Altering Visibility
Altering visibility to protected on instance methods for the cookie
class
85a1e1a
@dhensby
Collaborator

Perfect for us EU types! Up vote!

@wilr
Collaborator

Perhaps it would be better to use the 3.0 DI framework for creating the cookie class? http://api.silverstripe.org/3.0/framework/injector/Injector.html

@dhensby
Collaborator

To be honest, it's based on the way session is implemented, it makes little sense for it to implement injector from what I can see.

@halkyon
Owner

Looks okay to me. Although I would've thought it should act more like Requirements class. So in that case you'd have something like a Cookie_SessionBackend class which does the current session cookie work, leaving Cookie class as the accessor class to the currently used backend. This is the same pattern the Requirements class uses.

Anyone have any other thoughts?

The Injector idea @willrossi suggested was probably for the way you can set the Cookie backend class as an alternative to the static variable?

@dhensby
Collaborator

@halkyon yep, agreed. Lots of ways to implement this, however the route of being as similar to Session was intended to keep code consistency.

@fatlewis

Agree with everything Dan's said. There are other ways that this can be done, but it is modelled on the way Session is already implemented to conform as best as possible with the current state of the framework.

One of the issues at present is that core classes rely on the Cookie static methods, so cookies are set before Silverstripe even begins to execute a developer's custom code. It's pretty hard to override this and prevent any cookies from being set unless you go about it in a hacky manner (and these changes would solve that problem).

@ajshort

I'm not really a fan of this approach - I personally would prefer to have a separate class and use the new injector stuff rather than having to namespace all the methods with inst_. The session class was implemented before the new Injector API was available, so I think it would be better to use this. One other possibility would be to attach the cookie class to the request and response objects, since they are really part of the request and response. Thoughts?

@dhensby
Collaborator

@ajshort unless my understanding of the injector class is completely wrong, I don't see how this would work. Can you give some kind of idea of how using the injector methodology.

Using the injector methodology would also require a lot of changes of where cookies are being set. This fix is incredibly unobtrusive, no other changes other than to the Cookie class are required and all old code can be ported with no fuss. I think that is it's strongest quality and why it should be accepted. At least until someone else can come up with a 'better' way to do it.

@willmorgan
Collaborator

I think we should remember what exactly we are wrapping. Essentially, we are wrapping setcookie. That's all.

The solution Matt and Dan have produced is lightweight enough, and consistent with Session's architecture. I don't think implementing full blown injector methodologies here is proportional to this case.

This solves an important problem if you want to be legal in the EU, and is completely unobtrusive. In any case, the API should not change regardless of how one intends to implement the guts of the changes. So as Dan said, unless someone else can code a better, accepted, solution, I see no objective reason not to accept it.

But if we're going to stall, I would question why I should bother writing patches at all... :rolleyes:

@wilr
Collaborator

@willmorgan this is just the standard approach, patches undergo review hence why we use pull requests :). It's not a criticism, it just allows each dev to chip in.

I'll merge if but beware of the wrath of chillu. I would expect this to get refactored along with session in the near future.

@wilr wilr merged commit 2ac2977 into silverstripe:master
@dhensby
Collaborator

@willrossi Thanks for pulling it in. I'll handle @chillu !

Here @betterbrief we're more than happy to see it get re-factored, but we think this is a great solution for the immidiate that is unobtrusive and in line with the current API. No developers need to change their Cookie implementation to make this work, and that's great. So only people that WANT to modify the Cookie behaviour need to change anything.

Any other implementation would mean that users would have to modify their code so that it would work as it did before.

PS: well done @fatlewis for your first SS core contribution!!! WHOOP!

@wilr
Collaborator
@ajshort

The instance methods still use static variables and method calls, it would be good to fix this as well.

@tractorcow
Owner

This broke my framework lib due to duplicate function naming. I changed line 89 from:

    protected function report_errors() {

to:


    protected function inst_report_errors() {

to fix.

@wilr
Collaborator

Have also pushed c91e855 to fix the builds.

@fatlewis

Well spotted @tractorcow, thanks. Looks like it's been fixed by @willrossi now, thanks to you as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 29, 2012
  1. ENHANCEMENT: Improving Cookie class to allow for extendability

    Matt Lewis authored
    Previous to this the Cookie class has been very inflexible (cookies are
    all set using the static Cookie::set() and so the functionality is not
    extendable). Cookie class has been adjusted so extension is now a
    possibility for those wishing to alter its functionality. Improves
    compliance to the law of demeter.
  2. MINOR: Altering Visibility

    Matt Lewis authored
    Altering visibility to protected on instance methods for the cookie
    class
This page is out of date. Refresh to see the latest.
Showing with 50 additions and 16 deletions.
  1. +50 −16 control/Cookie.php
View
66 control/Cookie.php
@@ -6,15 +6,29 @@
* @subpackage misc
*/
class Cookie {
-
+
/**
* @var boolean
*/
static $report_errors = true;
-
+
+ /**
+ * Cookie class and instance for extendability purposes
+ */
+ static $cookie_class = 'Cookie';
+
+ static $inst = null;
+
+ public static function get_inst() {
+ if(is_null(self::$inst)) {
+ self::$inst = new self::$cookie_class();
+ }
+ return self::$inst;
+ }
+
/**
* Set a cookie variable
- *
+ *
* @param string $name The variable name
* @param string $value The variable value.
* @param int $expiry The expiry time, in days. Defaults to 90.
@@ -24,6 +38,29 @@ class Cookie {
* @param boolean $httpOnly See http://php.net/set_session
*/
static function set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = false) {
+ return self::get_inst()->inst_set($name, $value, $expiry, $path, $domain, $secure, $httpOnly);
+ }
+
+ /**
+ * Get a cookie variable
+ */
+ static function get($name) {
+ return self::get_inst()->inst_get($name);
+ }
+
+ static function forceExpiry($name, $path = null, $domain = null) {
+ return self::get_inst()->inst_forceExpiry($name, $path, $domain);
+ }
+
+ static function set_report_errors($reportErrors) {
+ return self::get_inst()->inst_set_report_errors($reportErrors);
+ }
+
+ static function report_errors() {
+ return self::get_inst()->inst_report_errors();
+ }
+
+ protected function inst_set($name, $value, $expiry = 90, $path = null, $domain = null, $secure = false, $httpOnly = false) {
if(!headers_sent($file, $line)) {
$expiry = $expiry > 0 ? time()+(86400*$expiry) : $expiry;
$path = ($path) ? $path : Director::baseURL();
@@ -34,25 +71,22 @@ static function set($name, $value, $expiry = 90, $path = null, $domain = null, $
}
}
}
-
- /**
- * Get a cookie variable
- */
- static function get($name) {
- return isset($_COOKIE[$name]) ? $_COOKIE[$name] : null;
- }
-
- static function forceExpiry($name, $path = null, $domain = null) {
+
+ protected function inst_get($name) {
+ return isset($_COOKIE[$name]) ? $_COOKIE[$name] : null;
+ }
+
+ protected function inst_forceExpiry($name, $path = null, $domain = null) {
if(!headers_sent($file, $line)) {
self::set($name, null, -20, $path, $domain);
}
}
-
- static function set_report_errors($reportErrors) {
+
+ protected function inst_set_report_errors($reportErrors) {
self::$report_errors = $reportErrors;
}
-
- static function report_errors() {
+
+ protected function report_errors() {
return self::$report_errors;
}
}
Something went wrong with that request. Please try again.