When reading through your code I find several things which I would change/remove.
The first is the empty constructor. A class without a constructor and a class with an empty constructor behave the same. I think it creates unnecessary noise in your code, so I would remove it.
Second is the naming of your methods. Since the class name is CookieMonster
I find the suffix Cookie
in registerCookie()
, retrieveCookie()
and destroyCookie()
unnecessary. You could also name them register()
, retrieve()
and destroy()
which I find more concise.
In your retrieveCookie()
method I think there is room for improvement. The language construct empty also checks if the variable/key exists before determining if it evaluates as empty. So you could refactor the method body to the following:
/*
* I have also renamed the method according to the
* suggestions above.
*/
public function retrieve($name)
{
if(!empty($_COOKIE[$name])) {
return $_COOKIE[$name];
}
return false;
}
This is also true for your destroyCookie()
method.
Regarding your destroyCookie()
method. It doesn't destroy the cookie, but merely unsets the value inside the superglobal $_COOKIE
. The cookie would be loaded again on the next request and would therefore never be deleted until it expires. To destroy a cookie you will have to redeclare it with an empty value and an expiration time in the past using the native PHP function setcookie()
as you did when setting the cookie in registerCookie()
. Unsetting the cookie inside the $_COOKIE
superglobal is important too, as this can prevent any further code accessing a to be deleted cookie value. This effectively denies the code the possibility to rely on unpredictable data and helps reduce extremely hard to find bugs.
When destroying a cookie you will also have to make sure it has the same parameters as when set. This includes both the path and domain, When destroying cookies you should also be aware of the timezone complications outlined in this question. An example:
/*
* Create a timestamp one hour in the past, which is 3600 seconds.
*/
$expire = time() - 3600;
setcookie('mycookie', null, $expire);
/*
* If a path and/or domain was specified when the cookie was
* created they too should be included.
*/
setcookie('mycookie', null, $expire, $path, $domain);
Talking about the path and domain leads me to the next part. When creating a cookie with your registerCookie()
method I have no way to set a path and/or domain as well as I cannot declare whether the cookie should only be sent over secure connections (SSL etc.) or if it is only restricted to HTTP requests leaving javascript unable to read/update/delete the cookie (this is often preferable when dealing with security).
Therefore I would change the method signature to include this.
/*
* Renamed according to suggestions.
*/
public function register($name, $value, $expire, $path, $domain, $secure, $httponly) {...
The $path
, $domain
, $secure
and $httponly
parameters can usually have default values. See the documentation of setcookie()
for more information.
Now when I am able to change these settings I will also require them to be available when destroying the cookie as two of them are required, namely $path
and $domain
. Therefore you should also change your destroyCookie()
method signature.
/*
* Renamed according to suggestions.
*/
public function destroy($name, $path, $domain) {...
Again these can have default values matching the register()
method signature.
Now to the last thing. I cannot see the use of and have never required the possibility to restore a deleted cookie. I would even go as far as sayings its a security issue. Consider the following. Your authentication system destroys the cookie when the user logs out. Then somewhere inside your code the cookie is restored before a response is sent to the client. Now your authentication system has been undermined by one single method call. This cannot be good and you're right!
I would simply remove the method and its related class property and focus on making this a utility class which wraps the cookie functions with reasonable logic. Such logic could be ensuring the cookie $name
is in fact a string and that is doesn't contain invalid characters. This is also appropriate for your other methods.
I would urge you to take a look at the Symfony cookie wrapper. They have a dedicated class to this and cookies are set using the setCookie()
method in response headers, which you might also find interesting.
I hope this can help guide you, happy coding!