3
\$\begingroup\$

I wrote this class to make cookie management easier.

I know it makes use of serialize(), but I have no intention of storing very much data in the cookie, and it seemed a cleaner solution than, say, the JSON functions (like, even if a class implements the JSONSerializable interface, it doesn't retain the class name by default, etc.). serialize() seemed more robust, although if there's a way to compress the output without it losing its integrity, please share. Probably the worst pitfall is that, if the cookie is messed with, unserialize() will trigger a notice and who knows what else -- but I don't feel I need to cater to the user experience of users who mess with cookies. (Although if the cookie is maxed out at a number of bytes, like 4,096, and it truncates, that could be an issue....)

I am in the process of writing a class to put on top of this one for managing user sessions implementing Charles Miller's solution.

I decided against implementing exceptions because it was simple enough to just return constants.

<?php
/**
 * Cookie.class.php, the Cookie class
 *
 * Contains a class for making cookies easier to manage via PHP
 * @author Miller <wyattstorch42 at outlook dot com>
 * @version 1.0
 * @package Miller
 * @subpackage Utils
 */

namespace Miller\Utils;

if (!function_exists('array_copy')) {
    /**
     * array_copy Properly and recursively clones an array
     * @param array $source The original array
     * @return array The clone
     */
    function array_copy ($source) {
        $return = array ();
        $keys = array_keys($source);
        $values = array_values($source);

        for ($i = 0; $i < count($keys); ++$i) {
            if (is_object($values[$i])) {
                $return[$keys[$i]] = clone $values[$i];
            }
            else if (is_array($values[$i])) {
                $return[$keys[$i]] = array_copy($values[$i]);
            }
            else {
                $return[$keys[$i]] = $values[$i];
            }
        }
        return $return;
    }
}

/**
 * A class for making cookies easier to manage via PHP
 * @package Miller\Utils
 */
class Cookie {
    /**
     * Class constants
     * SESSION, EXPIRE, DAY, WEEK, MONTH, and YEAR are used to define the life of the cookie
     * ERROR_HEADERS_SENT, ERROR_SET_COOKIE, and ERROR_NONE are possible return values for the sent() method.
     */
    const
        SESSION = 0,
        EXPIRE = -3600,
        DAY = 86400,
        WEEK = 604800,
        MONTH = 2592000,
        YEAR = 31536000,
        ERROR_HEADERS_SENT = -1,
        ERROR_SET_COOKIE = -2,
        ERROR_NONE = 1;

    protected
        /**
         * Holds the name of the cookie
         * @var string
         */
        $name,
        /**
         * Holds the expiration timestamp of the cookie
         * @var integer
         */
        $expiry,
        /**
         * Holds the path of the cookie
         * @var string
         */
        $path,
        /**
         * Holds the domain of the cookie
         * @var string
         */
        $domain,
        /**
         * A flag that sets the secure option for the cookie
         * @var boolean
         */
        $secure,
        /**
         * A flag that sets the httponly option for the cookie
         * @var boolean
         */
        $httponly,
        /**
         * An array that holds the current unserialized properties stored in the cookie
         * @var array
         */
        $properties,
        /**
         * An array that holds the properties that will be stored and serialzed in the cookie on send()
         * @var string
         */
        $internalProperties;

    public function __construct ($name, $expiry = self::WEEK, $path = '', $domain = '', $secure = null, $httponly = false) {
        /**
         * Constructor
         * @param string $name The name of the cookie
         * @param null|integer|string $expiry The expiration timestamp of the cookie (if null, sets a session cookie; if string, converts using strtotime())
         * @param string $path The path of the cookie
         * @param string $domain The domain of the cookie
         * @param null|boolean $secure The flag that sets the secure option for the cookie (if null, sets to true if $_SERVER['HTTPS'] is set, false otherwise)
         * @param boolean $httponly The flag that sets the httponly option for the cookie
         */
        $this->name = (string) $name;

        if ($expiry === self::SESSION) {
            $this->expiry = $expiry;
        }
        else if (is_numeric($expiry)) {
            $this->expiry = time()+$expiry;
        }
        else {
            $this->expiry = strtotime($expiry);
        }

        $this->path = (string) $path;
        $this->domain = (string) $domain;
        $this->secure = $secure === null ? isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] : (bool) $secure;
        $this->httponly = (bool) $httponly;

        $this->properties = array ();
        $this->internalProperties = array ();
        if (isset($_COOKIE[$this->name])) {
            $properties = unserialize($_COOKIE[$this->name]);
            $this->internalProperties = array_copy($properties);
            $this->properties = array_copy($properties);
        }
    }

    public function exists () {
        /**
         * Checks if the cookie was sent from the last browser request
         * @return bool True if the cookie is set, false otherwise
         */
        return (bool) $this->properties;
    }

    public function __isset ($property) {
        /**
         * Checks if the given property is set on the cookie
         * @param string $property The property key
         * @return bool True if the property is set, false otherwise
         */
        return isset($this->properties[$property]);
    }

    public function __get ($property) {
        /**
         * Returns the value of the given property on the cookie
         * @param string $property The property key
         * @return bool The value of the property if it is set, null otherwise (use __isset() to differentiate a null value from an undefined value)
         */
        return $this->__isset($property) ? $this->properties[$property] : null;
    }

    public function __set ($property, $value) {
        /**
         * Sets or overrides the given property (will not be accessible until the cookie is sent)
         * @param string $property The property key
         * @param mixed $value The value
         * @return void
         */
        $this->internalProperties[$property] = $value;
    }

public function __unset ($property) {
    /**
     * Unsets the given property (will not be accessible until the cookie is sent)
     * @param string $property The property key
     * @return void
     */
    if (isset($this->internalProperties[$property])) {
        unset($this->internalProperties[$property]);
    }
}

    public function send () {
        /**
         * Serializes the properties and sends the cookie to the browser. On success, updates the values of the properties.
         * @return integer ERROR_HEADERS_SENT if the headers have already been sent, ERROR_SET_COOKIE if the setcookie() function failed, or ERROR_NONE on success
         */
        if (headers_sent()) {
            return self::ERROR_HEADERS_SENT;
        }

        $value = $this->expiry === self::SESSION ? false : serialize($this->internalProperties);
        if (setcookie($this->name, $value, $this->expiry, $this->path, $this->domain, $this->secure, $this->httponly)) {
            $this->properties = array_copy($this->internalProperties);
            return self::ERROR_NONE;
        }
        return self::ERROR_SET_COOKIE;
    }
}
?>

Example usage:

<?php
namespace Miller\Utils;
error_reporting(E_ALL);
require 'Cookie.class.php';

ob_start();

$cookie = new Cookie('example_cookie', Cookie::SESSION);
var_dump($cookie->exists());
   // true if cookie is stored in the browser already, false otherwise
var_dump(isset($cookie->example_prop));
   // true if example_prop exists in cookie, false otherwise
var_dump($cookie->example_prop);
   // the value of example_prop, or null if it is not set
$cookie->example_prop = 123;
var_dump($cookie->example_prop);
   // will still be the previous value, because the cookie hasn't been sent
var_dump($cookie->example_prop_2);
   // the value of example_prop_2, or null if it is not set
unset($cookie->example_prop_2);
var_dump($cookie->example_prop_2);
   // will still be previous value, because cookie hasn't been sent
$cookie->current_time = new \DateTime('now', new \DateTimeZone('America/Phoenix'));
   // will serialize objects using PHP native function and unserialize automatically
var_dump($cookie->send());
   // -1, -2, or 0 (ERROR_HEADERS_SENT, ERROR_SET_COOKIE, or ERROR_NONE)
var_dump($cookie->example_prop);
   // if send() was successful, this will now be 123
var_dump(isset($cookie->example_prop_2));
   // if send() was successful, this will now be false
var_dump($cookie->current_time);
   // if send() was successful, this will now be the recorded time

ob_end_flush();
?>
\$\endgroup\$
  • \$\begingroup\$ Don't use the closing ?> in definition files (PHP recommends not using the closing tag!). If a file is called something.class.php, I'd expect it to contain only a class definition, not functions, too... Implement one of the Travesable interfaces, so you can use the real isset, not your pseudo-magic __isset method \$\endgroup\$ – Elias Van Ootegem Sep 7 '13 at 16:51
  • \$\begingroup\$ @EliasVanOotegem: I think your comment would be worth an answer (with some references). I'd upvote it. \$\endgroup\$ – palacsint Sep 8 '13 at 2:12
  • \$\begingroup\$ @palacsint: Posted a rand/answer there's quite a lot more that needs doing in this code, come to think of it... sorry if I put things bluntly, but You have to, when doing code-review \$\endgroup\$ – Elias Van Ootegem Sep 9 '13 at 8:10
1
\$\begingroup\$

First off, I've posted a fairly length answer on the subject of request classes, which can be found here. Because you state you're thinking of writing a Session class on top of this one, I'm posting a link to my answer here.
In that answer, I discuss what you should keep in mind when doing so. For example: using setters and getters, sanitize all request data etc...

Then, as I said in the comment, I noticed you're using the ?> closing tag, which is optional. Omitting it, especially in those files that will be include or require-ed is considered good practice. It's, amoungst other things, to avoid excess whitespace. Read more here
Another part of good practice is to write the doc-blocks just before the method definition, not in the function body. As soon as you start using the ReflectionClass (especially the getDocBlock method), you'll see the value of this. Even if you don't use it, other libs might (like Doctrine to name one, not unimportant example)

You're also defining a method called __isset. The double underscore would suggest this is a magic-method, that enables the user of this class to write something like:

if (isset($instance['cookieName']))

And that php will convert that into if ($instance->__isset('cookieName')). That's not the case. The magic isset method only works when you write isset($instance->cookieName), which, to all intents and purpouses still is a different syntax.
If I were in your shoes, I'd implement a Travesable interface, or the ArrayAccess interface. read through the examples, and you'll see array-like usage of isset is possible thanks to this interface.

The last remark I'd like to make is the fact that your file is called Cookie.class.php, so I'd expect its contents to be just a class definition. It isn't, though: you're defining a function, too. That's just not nice. I'd suggest you read, and make sure to comply with the unofficial PHP-FIG standards. Though not official, all major players (check list here) subscribe to these standards. If your code complies, too, autoloaders from any of the major frameworks will just work with your classes.
While I'm on the subject of autoloaders: implement them, use them, rely on them. If your classes are defined according to the aforementioned standards, you can do away with all those pesky require's. Even if you do need the occasional require: use the safer require_once variant. Sure it's a tad slower, but at least it's safe.

Oh, and if you still need that array_copy in the Miller\Utils namespace, you could use a static method, since PHP statics are just globals in drag, or declare a global function and call it with a leading \.
Still, look at where you're using it:

public function __construct ($name, $expiry = self::WEEK, $path = '', $domain = '', $secure = null, $httponly = false)
{
    $this->name = (string) $name;
    if ($expiry === self::SESSION) {
        $this->expiry = $expiry;
    }
    else if (is_numeric($expiry)) {
        $this->expiry = time()+$expiry;
    }
    else {
        $this->expiry = strtotime($expiry);
    }

    $this->path = (string) $path;
    $this->domain = (string) $domain;
    $this->secure = $secure === null ? isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] : (bool) $secure;
    $this->httponly = (bool) $httponly;

    $this->properties = array ();
    $this->internalProperties = array ();
    if (isset($_COOKIE[$this->name])) {
        $properties = unserialize($_COOKIE[$this->name]);
        $this->internalProperties = array_copy($properties);
        $this->properties = array_copy($properties);
    }
}

You're clearly using it to set properties inside the class, so why isn't this function a private method?

Moving on to some actual code review: Your constructor is doing way to much work. Besides, when I create a cookie, I'd like to be able to set the data when I choose, not passing it all to the constructor. I'd like to use a Cookie class to check if a given cookie exists, and if it does, check when it expires, if it doesn't I'd like to create it and maybe determine its expiration date/time later on. Implement setters and getters for that.
Also use these setters to sanitize whatever data is being passed to the constructor. Sure:

$this->name = (string) $name;

Assures you that the name property is bound to be a string. Good, but what if I passed an object. I'm not going to get an exception thrown, though I clearly passed an argument that is a good reason to throw an InvalidArgumentException. Don't just cast to the types you want, check if the arguments passed are:

  1. Castable to the type without loss of data
  2. Valid types in the first place

By the second I mean that, if I were to accidentally pass an instance of PDO as name, I should be notified of the error in my code. Basically: your class is too reliant on magic-methods, it allows overloading of properties, which is terrible, especially with request objects (the request shouldn't be subject to change, unless there's a good method defined for it). Some constants (like the SESSION constant indicate a class of responsability, check first link to find out more).
As ever: don't hush-up/work around errors. Throw exceptions when something is not quite right. You don't know why or how the name property was passed, and why it's not a string, nor does your class need to know. Just throw an exception. Let the user of your class fix his code.

On the use of serialize: Since you're thinking about a session object: if you're going to use serialize, save it for the session, just don't use a cookie.

On the __set method: check what the value is. If I'm setting something like:

$instance->connection = $pdo;

Your code won't complain, but you can't serialize a DB connection! In fact, there are quite a few things that can't be serialized. Google around a bit

\$\endgroup\$
  • \$\begingroup\$ What would you suggest in place of serialize() on unserializable objects? I think it would make more sense to include in the documentation that the serialize() function is called on properties and leave it up to the user to implement the Serializable interface, etc., because I can't accommodate an unlimited number of classes that are unserializable. \$\endgroup\$ – M Miller Sep 11 '13 at 16:36
  • 1
    \$\begingroup\$ @MMiller: Personally, I'm not a big fan of serialize calls, especially when the serialized data ends up in a cookie or session, but that's just me. If you're keen on using it, though, I, too, would add it in the doc-block, and do something like if (is_object($param) && !($param instanceof Serializable || $param instanceof stdClass)) throw new InvalidArgumentException('cannot be serialized safely'); \$\endgroup\$ – Elias Van Ootegem Sep 12 '13 at 7:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.