Finalize request and response cookies implementation

[JoeBengalen]

Since the Http\Cookies only contains static helper methods, why is there still an interface defined? The class does not even implement the interface. (see https://github.com/slimphp/Slim/blob/develop/Slim/Http/Cookies.php#L14)

[geggleto]

I am plus one for dropping cookies out of core and putting them as middleware. Seems to have been following the trend ie sessions and lots of other things

[codeguy]

Agreed on both counts. @geggleto I'm unsure if middleware or a service provider (or both?) would be more appropriate. What would your proposed middleware look like?

[geggleto]

Actually I think your suggestion might be better. A service provider to parse the header and then middleware to interact with it. For the interaction we can use an interface so you can develop your own and plug in play for additional functionality

[codeguy]

Right. So the service provider could provide Request Cookie header parsing and Response Set-Cookie header serialization, while the middleware is where these methods would be used.

What is the workflow for a developer to add a cookie to the Response? What does that look like, and where does that happen?

[codeguy]

Bear in mind we should limit ourselves to PSR7 message interfaces.

[geggleto]

Is there even any allowances for this via the psr7 interface? I am not up to speed on it yet. But it would seem that, our choices are going to depend on filling that requirement first .

[codeguy]

Read this https://groups.google.com/forum/#!topic/php-fig/6zesbZWLY_Y

[codeguy]

The clean solution appears to be this: creating new cookies means storing the cookie data in a separate, stand-alone object (via service provider). This object is in no way contained by or related to the response object. I imagine a shared Pimple service that looks like this:

$app['cookies']->set('name', ['value' => 'foo', 'domain' => 'example.com', 'expires' => '3 days']);

The middleware layer is responsible for 1) unserializing the Request Cookie header, and 2) serializing the cookie data from the service provider above into HTTP Set-Cookie headers. I don't like how this is separated from the Response object; it feels wrong, but I think it's the best solution.

Aside: For middleware and service providers that work together like this, do you still prefer adding them to the application separately (e.g., $app->add(middleware) and $app->register(provider)) or is there a way to add them both at once?

[codeguy]

Disregard that last aside. I think we'll register the provider in the App constructor, and we may not even need a stand-alone middleware. We can instead deserialize cookies in the Request (for its getCookieParams() method), and serialize cookies in the App::run() method.

[geggleto]

Reading that thread gave me chills. The whole point of a framework is to make things easier. I don't see how rawly encoding cookies into header fields does that.

Hmm I think this might be getting out of hand a bit. I am going to bow out for now so I can think on it having 2 more objects seems excessive and not very clean especially given the coupling they are likely to have.

[codeguy]

Right. That's the predicament :( I'll think on this too. What is available now in the develop branch is definitely not final.

[codeguy]

Another option is to just re-introduce first-class methods on the App class itself. App::getCookie() uses Request object's PSR7 methods to fetch a cookie value. App::setCookie() uses the built-in provider (hidden from end user) to store cookies that are later serialized onto the Response object (again hidden from end user).

EDIT: I kind of like this solution. It keeps things clean and compatible with PSR7 interfaces, yet simplifies the end user experience.

[geggleto]

+1 for that last suggestion.

[akrabat]

As someone who doesn't use cookies at all, I'd prefer that they were handled by a helper class. However, if they really are in the 80%-use case, then App methods are fine.

[geggleto]

I personally don't use cookies at all, however I know there are valid use cases. I don't think they will be around forever so having them somewhat optional would be beneficial.

[codeguy]

Pushed up some changes here https://github.com/slimphp/Slim/commit/fa6f24b88bdf8af4ae6a0d705d3807181d23f599

Usage:

$app->get('/set', function ($req, $res, $args) {
    $this['cookies']->set('foo', [
        'value' => 'bar',
        'domain' => 'example.com',
        'path' => '/sub',
        'expires' => '4 days'
    ]);
});

Or you can just do:

$app->get('/set', function ($req, $res, $args) {
    $this['cookies']->set('foo', 'bar');
});

[lalop]

Maybe I'm wrong, but the current implementation has 2 issues for me:

• In the current state of this project, we can send and receive multiple request/response from the same app instance ( while testing for exemple or react the day they will implement psr7 ). The only thing that will be share between the request-response is the cookie service.
• if a middleware adds token by its own this will erase the cookie it may have set

I'm really not an expert but I don't understand why this is cleanest than the withCookie that was in the Response class, if anyone want to enlighten me he's welcome.

[codeguy]

So the two best solutions are:

1. Keeping Response::withCookie($name, $props) as @lalop suggested. This keeps Cookie concerns with the Response object. However, this must immediately serialize the cookie into a Set-Cookie header. This also flows well with the existing with* methods on the Response object. However, this is not a part of the PSR7 interface and cannot be relied upon anywhere else in the framework.
2. Use the cookie service. This decouples cookies from the HTTP messages, and it lets a Slim app continue to have cookie management tools even if the PSR7 message objects are swapped out with an alternative implementation.

I'm honestly fine with either. Neither is perfect. What are everyone's thoughts?

[codeguy]

In my opinion, the non-standard withCookie()method is cleaner. However, the cookies service is guaranteed to work regardless of PSR7 implementation. I understand the semantic argument against it, but I'm going to keep that solution.

[lalop]

Hi, that 's fine for me, I wanted to point it. I've seen that the same method is used on the httCache middleware so that can be a kind of convention.