ZF-4520: Convenience class for creating Set-Cookie HTTP headers

Description

Currently in ZF MVC applications, cookie management is still handled via the superglobal $_COOKIE; we provide no means to simplify or abstract this -- which is leading to issues when attempting to test applications that make use of cookies.

The Response object needs the following methods: * addCookie() * addCookies() * setCookies() * getCookie() * getCookies() * removeCookie() * clearCookies()

Comments

A patch has been uploaded.

It is just a begining throwing ideas : CookieJar needs to be refactored Inspired from Zend_Http_Response.

This patch looks really good. If you can supply unit tests as well, please go ahead and commit directly to trunk; if not, let me know so I can get it on my backlog.

Zend_Http_CookieJar patch provided at r13641. For Zend_Controller_Response_Http cookie handling, I suggest one thing :

Zend_Controller_* in HTTP environment (response and request object) should have a bridge to use Zend_Http_* stuff. That makes more dependencies, and that' not very good, anyway, that could improve code reuse.

Think about that :


// Zend_Controller_Front
public function cookieFactory($name, $value, $expires = null, $path = null)
    {
        $request = $this->getRequest();
        if (!$request instanceof Zend_Controller_request_Http) { 
            throw new Zend_Controller_Exception('Cookie can only be generated in HTTP environment');
        }
        return new Zend_Http_Cookie($name, $value, $request->getHttpHost() , $expires, $path, $request->isSecure());
    }

// somewhere else
$response->setCookie(Zend_Controller_Front::getInstance()->cookieFactory('foo','bar');
// or, from an action, providing a method in HTTP environement :
$this->setResponseCookie('foo', 'bar');
// or again, (see the second patch)
$response->setCookie('foo','bar'); // uses ZC_Front idea to generate a cookie

And then refactor Zend_Controller_Response_Http from the first patch idea to the one attached as the second patch to that comment

Second idea of patch attached

It's been a while since the last comment, but I'd like to continue the discussion on this issue.

The idea of making front controller aware of the cookies is kinda wrong of course because it's protocol specific and has nothing to do with it, but, we still need an easy way to set cookies.

Now, people usually do setcookies(), and I don't like it at all, because it is out of the Response object context. It breaks the OOP design.

What I do know (it might look ugly, but I still prefer this way) is:


        // Setting page cookie header
        $expires = new Zend_Date();
        $expires->addSecond($pageCookieLifetime);

        // And here we go:
        $this->getResponse()->setHeader('Set-Cookie',
            'page=' . $page->getCode() . '; '
            . 'expires=' . $expires->get(Zend_Date::COOKIE) . '; '
            . 'path=/'
        );

and besides that there shouldn't be many cookies set, it is still uncomfortable.

I could use an instance of Zend_Http_Cookie and it would be much more comfortable (and, probably enough for me) but I can't do so, because of current state of Zend_Http_Cookie::__toString method:


    /**
     * Get the cookie as a string, suitable for sending as a "Cookie" header in an
     * HTTP request
     *
     * @return string
     */
    public function __toString()
    {
        return $this->name . '=' . urlencode($this->value) . ';';
    }

which does not include path, domain, expires parts. Why? I'd suggest to add them as the easiest and the fastest what we can do.

Though would be nice to have this part of the framework (Cookie, CookieJar) fully refactored.

This is a very simple feature that would be great to have but seems to be languishing in the home stretch for some reason. Can it be resolved or reassigned to someone who will close it out?

I implemented the solution to a local *_Controller_Response_Http class but changed the _sendCookies() method to avoid the Zend_Http_Cookie::__toString() :

     /**
      * Appends the cookies in the cookiejar
      * to raw headers
      *
      */
     protected function _sendCookies()
     {
         if (!$this->_cookieJar->isEmpty()) {
             $cookies = $this->_cookieJar->getAllCookies();
             
             foreach ($cookies as $cookie)  {
                 setcookie( $cookie->getName(),
                            $cookie->getValue(),
                            $cookie->getExpiryTime(),
                            $cookie->getPath(),
                            $cookie->getDomain(),
                            $cookie->isSecure());
             }
         }
     }

Shall I create a new issue for the Zend_Http_Cookie::__toString() which is described as suitable for HTTP but is not ? The Zend_Http_Cookie class does not include a isHttpOnly() method that could be useful to completely define the cookie.

This issue op open for a while and many people would like to have this feature implemented. As far as I have read the comments and patches, two things cross my mind:

1) Two patches are provided, both patching the same class and both having the same changes to the api. Their difference is the inner working. Which one is the best and can the other be removed? As far as I can see the Zend_Controller_Response_Http.patch is the most preferred to use.

2) Whilst the patch uses Zend_Http_Cookie and Zend_Http_CookieJar, in the comments it's argued it's not possible to use them because of a problem with Zend_Http_Cookie::__toString(). The patch shows it uses Zend_Http_CookieJar::getAllCookies() together with Zend_Http_CookieJar::COOKIE_STRING_CONCAT which makes it possible to use the return string in a http header. So I can conclude it's now possible to set cookies OOP style with the Reponse object, using a Zend_Http_Cookie?

If all I said is true, the only step required to implement this, is providing unit tests?

I have just stumbled across this issue, having spent a joyous few hours discovering that it's not possible to unit test cookie setting.

I don't believe that patching the response object is the correct thing to do here.

1) It relies on changing what is supposed to be standard functionality so moving code across platforms would cause it to mysteriously break (never a good thing when you've just moved from development to staging and want to impress your client). Even if the patch is committed to the trunk, it may cause problems if anyone has sub-classed it. That includes the ripple-down effect into the HttpTestCase.

2) It's marginally better to inherit than patch, although this has the same caveats with respect to sub-classes.

3) It's too much responsibility. The response object shouldn't be managing cookies.

4) SetCookie is really just a convenience function to generate the Set-Cookie header. The response object already knows how to send headers, and you wouldn't patch it to provide support for opensocial, firephp, or anything else that may send custom headers.

It seems to be a better approach would be to pass the response to the cookie jar and have it set the headers. Or, given that there is already a cookie jar, some kind of mediating object which handles it.

On the subject of the Zend_Http_Cookie implementation, I'm guessing that the short-comings of the __toString() magic method are because it is intended to be used client-side, and clients send only name-value pairs to the server. I can see the logic in that decision, though there probably does need to be some kind of convenience accessor for the entire cookie as required server-side. The other problem I note with it, though only a minor one, is that there is no support for the http_only flag.

Duncan, I'd have to disagree in that Set-Cookie is not a "custom" header since it's part of the HTTP spec.

http://www.w3.org/Protocols/rfc2109/rfc2109

It's more similar to sending redirect headers than it is to sending FirePHP headers. If it's part of the spec and used often, I don't see why it shouldn't be supported by Zend_Controller_Response.

I wouldn't go so far as to say that the Response would be managing cookies, but rather managing headers (including writing cookies but not reading them). Reading cookies should be managed by Zend_Controller_Request.

Perhaps an action helper would be the easiest way to manage cookies, which reads cookies from the request and writes cookies to the response. This should make it unit-testable while keeping things organized.

The main issue with {{Zend_Http_Cookie}} and {{Zend_Http_CookieJar}} is that the are designed almost exclusively for {{Zend_Http_Client}}, and so they only provide methods (mainly __toString) to construct "Cookie" header values. Those classes don't provide methods for creating "Set-Cookie" header values. Perhaps we could backport ZF2's {{Zend\Http\Header\SetCookie}} for this purpose?

Once Set-Cookie header value generation is in place, you should easily be able to do something like this from the action controller:


$this->getResponse()->setHeader('Set-Cookie', new Zend_Http_Header_SetCookie('foo', 'bar', 'example.com'));

As for using cookie values in an action controller, {{Zend_Controller_Request_Http}} does provide a {{getCookie}} method:


$fooCookieValue = $this->getRequest()->getCookie('foo');

Is this what you are looking for?

I've attached a patch for a prototype implementation of my previous suggestion (Zend_Http_Header_SetCookie): !ZF-4520_v1.patch!

Usage Test Case:


public function testCanSendSetCookieHeaderUsingProvidedHeaderClass()
{
    $resp = new Zend_Controller_Response_HttpTestCase();
    
    $cookie = new Zend_Http_Header_SetCookie();
    $cookie->setName('foo')
            ->setValue('bar')
            ->setDomain('example.com')
            ->setHttponly(true);
    $resp->setHeader('Set-Cookie', $cookie->getFieldValue());
    
    $headers = $resp->sendHeaders();
    $this->assertEquals('Set-Cookie: foo=bar; Domain=example.com; HttpOnly', $headers['set-cookie']);
}

Real-World Usage: (also showing short-hand notation)


// From within action controller (Short-hand)
$this->getResponse()->setHeader('Set-Cookie', new Zend_Http_Header_SetCookie('foo','bar','example.com',NULL,NULL,NULL,true));

The {{Zend_Http_Header_SetCookie}} is based on {{Zend\Http\Header\SetCookie}} from ZF2 master, though I suspect that one isn't the most up-to-date implementation as it has some issues (not all set*() methods are fluent, httpOnly is not handled, etc).

It's been more than four months since I made the above suggestion. If this is something which can be included in ZF 1.12, I can backport the latest {{Zend\Http\Header\SetCookie}} from ZF2 to complete my initial patch.

Adam -- go for it. Deadline for new features is 23 March 2012; as long as you have it ready by then, it can go in.

Added update version of patch (ZF-4520_v2.patch)

My new patch backports the latest Zend\Http\Header\SetCookie and it's tests from ZF2, plus some additional tests. I have some more questions before I proceed:

  1. Should I add the convenience methods to the response object, as outlined in the issue description? There was some concern in comments above that this would give too much responsibility to the response object.

  2. I believe that the SetCookie implementation in ZF2 still falls victim to ZF2-169. The fromString method assumes that any key not matching one of the pre-defined key names is the name/value pair, but the cookie RFCs state that only the first K=V pair can be the cookie name/value. I'll whip up a reproduce test case and if my research and hunch are both correct i'll send a ZF2 PR.

Once my patch has been reviewed and OKed i'll add the necessary manual page entry/entries.

2 above has been resolved. Once I get a chance to whip up a manual page entry for the new code I will commit it all to SVN.

Updated patch

  • Rearranged the order of constructor arguments to match PHP's setcookie()
  • Added a unit test to ensure {{__toString}} method output is compatible with {{Zend_Controller_Response_Http::setRawHeader}}
  • Added a manual page section showing how to use the new class (in {{Zend_Controller-Response.xml}})

Screengrab of manual page section !bf2a50.png!

Are there any comments on or concerns with this approach? Are there any additional use cases that could be covered that I've missed?

Adam -- looks good. Go ahead and push to trunk.

Fixed in trunk (1.12.0): r24783