hansott / psr7-cookies

🍪 bakes cookies for PSR-7 messages
https://hansott.github.io
MIT License
40 stars 4 forks source link

"Useless Functionality" #7

Closed stormwulfren closed 5 years ago

stormwulfren commented 5 years ago

Hi! I've been using this library for a while now. I'm curious - why remove the cookie signing and RequestCookies classes? That functionality is far from useless. We use RequestCookies to unpack cookies from a PSR-7 request ...

hansott commented 5 years ago

Hi @laurenblackfox, thanks for opening an issue (and awesome that you're using this library!).

First of all, you can keep using version 2.x.x of this package, nothing will break. The removed functionality is released with a new major version (3).

I decided to remove the RequestCookies because it doesn't offer a lot of functionality. Yes, it wraps the cookies in value objects and throws an exception if you access a cookie that doesn't exist and while I'm a fan of that, there are some PHP quirks that makes RequestCookies annoying (See https://github.com/hansott/psr7-cookies/issues/5). It's better to use ServerRequest->getCookieParams() directly.

I decided to remove the cookie signing functionality because I'm not an expert in cryptography/security. I try to avoid writing cryptography/security code. I'll leave that for the experts in that field. You're are free to port the signing functionality to your own package (to complement this package).

When I'm looking for a package myself, I look for small and focused packages. I tried to make this package smaller and more useful.

Thanks for understanding. If you have other questions, please let me know.

Hans

stormwulfren commented 5 years ago

I can't say I wholly disagree with your reasoning. I think I might just do that. I think when I have some time, I'll fork the cookie HMAC code and have a tinker.

I think an argument could be made to make an improvement to the RequestCookies logic, rather than removing it outright. I'm personally of the opinion that any data that is non-atomic should be an object.

I think if I were rewriting this from scratch I'd have a code flow like:

$cookieCollection = Cookies::fromRequest($request);
//Alternatively: $cookieCollection = Cookies::fromGlobal($_COOKIE);
if($cookieCollection->has('CookieName'))
  $cookie = $cookieCollection->get('CookieName');
$cookieVal = $cookie->getValue();

//Single Cookie
$newCookie = Cookie::create($name, $val, etc..);
$newCookie->addToResponse($response);

//Multiple Cookies
$newCookies = new Cookies();
$newCookies->add($newCookie1);
$newCookies->add($newCookie2);
$newCookies->add($newCookie3);
$newCookies->addToResponse($response);

Then you could have method type hinting for Cookies and all that goodness. Then, in conjunction with my end, I could just do:

$signedCookie = SignedCookie::sha512($cookie, $key);
$signedCookie->addToResponse($response);

shrug Just throwing it out there.

hansott commented 5 years ago

Not everyone needs the RequestCookies or Signer functionality. Having separate packages for that makes more sense to me. Users that only need to add a single cookie to a response can do that now. Maybe I should have released complementing packages, it's still code that I have to maintain of course. If you decide to create packages for that functionality, please let me know. I'm more than happy to add links to the packages (https://getcomposer.org/doc/04-schema.md#suggest).

hansott commented 5 years ago

And thanks for throwing your ideas/concerns here, always happy to listen.