1
\$\begingroup\$

I was wondering if there was a better way to do this. Would that mean there would need to be no client-side refreshing to show the cookies when they are created?

<?php
class cookieMonster
{
    public static $lastDestroyed;

    public function __construct() {  }

    /**
     * Register a cookie with the browser.
     * @param string $name Name to be given to the cookie
     * @param string $value Value to be put inside the cookie
     * @param int $expire How long the cookie will last, leave at 0 to expire on end of session/when the browser closes
     * @return bool True if cookie is created and false if not
     */
    public function registerCookie($name, $value = '', $expire = 0)
    {
        return setcookie($name, $value, $expire);
    }

    /**
     * Summary of retrieveCookie.
     * @param string $name The name of the cookie to retrieve.
     * @return bool True if found/not empty and false if not.
     */
    public function retrieveCookie($name)
    {
        $return = true;
        isset($_COOKIE[$name]) ?: $return = false;
        !empty($_COOKIE[$name]) ?: $return = false;
        return $return ? $_COOKIE[$name] : $return;
    }

    /**
     * Destroy a cookie variable, this can be undone, but not after a second has been destroyed.
     * @param mixed $name 
     * @return bool
     */
    public function destroyCookie($name)
    {
        $return = true;
        isset($_COOKIE[$name]) ?: $return = false;
        !empty($_COOKIE[$name]) ?: $return = false;
        if ($return) :
            static::$lastDestroyed = ["name"=>$name, "value"=>$this->retrieveCookie($name)];
            unset($_COOKIE[$name]); return true;
        else :
            return false;
        endif;
    }

    /**
     * Restores the last destroyed cookie, can only be done for the very last cookie that was destroyed.
     * Does not require a param as the last destroyed is held in the class.
     * @return bool
     */
    public function restoreLastCookie()
    {
        $allow = true;
        static::$lastDestroyed ?: $allow = false;
        return $allow ? $this->registerCookie(static::$lastDestroyed['name'], static::$lastDestroyed['value']) : false;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Jamal Sep 16 '15 at 15:57
2
\$\begingroup\$

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!

\$\endgroup\$
0
\$\begingroup\$

There's a few things that can be pointed out about this:


There's a better way to structure these sorts of constructs:

public function retrieveCookie($name)
{
    $return = true;
    isset($_COOKIE[$name]) ?: $return = false; // if false set to false 
    empty($_COOKIE[$name]) ? false : $return = true; // if false set to false
    return $return ? $_COOKIE[$name] : $return;
}

Assign the variable the actual condition result rather than a ternary.

public function retrieveCookie($name)
{
    $return = (isset($_COOKIE[$name]));
    $return = (!empty($_COOKIE[$name]));
    return $return ? $_COOKIE[$name] : false;
}

Notice that I changed : $return to : false, as if it's set to false, there's no point calling a variable result, just put false in as is.


    if ($return) :
        static::$lastDestroyed = ["name"=>$name, "value"=>$this->retrieveCookie($name)];
        unset($_COOKIE[$name]); return true;
    else :
        return false;
    endif;

Why are you using the if : endif; structure? Just use a normal if statement loop.

Don't have the return true statement on the same line as other code, that's just saving the few bytes of a new line at the sake of readability.

Also, include some whitespace between the => in the definition of static::$lastDestroyed.

Both the if and the else blocks have one thing in common, returning the state of $return, so you should factorise this out, and make it so it runs after $returns true case has been executed.

All together:

if ($return){
    static::$lastDestroyed = ["name" => $name, "value" => $this->retrieveCookie($name)];
    unset($_COOKIE[$name]);
}
return $return;

Classes shouldn't be named like class cookieMonster, rather like class CookieMonster.

\$\endgroup\$
0
\$\begingroup\$

You don't clear a cookie in the client by calling unset $_COOKIE[..]. That is only removing the value from the $_COOKIE array that is created automagically by PHP just before executing your code (using data sent by the browser).

You should re-set the cookie using:

setcookie('same_name_of_the_old_cookie', NULL, -1, '/')
// NULL to clear the value, -1 to set expiration in the past and '/'
// to make it valid for all domain

Also, there are a lot of style problems as pointed by Quill that you should work into.

\$\endgroup\$

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.