New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move CookieComponent functionality to CakeCookie #2001

Closed
lh-import opened this Issue Oct 11, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@lh-import

lh-import commented Oct 11, 2013

Created by euromark, 12th Oct 2012. (originally Lighthouse ticket #3276):


A CakeCookie class should handle the main cookie handling similar to CakeSession and sessions.
The CookieComponent then could wrap the functionality similar as SessionComponent does for CakeSession.

The main benefit would be that we can read/write cookies outside of the controller scope if necessary without hacking it into the code like

$Cookie = new CookieComponent(new ComponentCollection());

So it would just be

$Cookie = new CakeCookie();

it would also mean less coupling between the cookie logic and controller/component stuff (which then only wrap the lib class).

@lh-import

This comment has been minimized.

lh-import commented Oct 11, 2013

12th Oct 2012, euromark said:


My first approach on this (test cases still fail, of course):
https://github.com/dereuromark/cakephp/compare/cakephp:2.3...dereuromark:2.3-cake-cookie

Let me know what you think.

Basic ideas:

  • Decoupling into a clean lib and static access
  • CakeResponse calls the wrapper methods only
  • More control since you could send() manually now (if really necessary) - cookies are cleared after sending to prevent sending them twice
  • CakeResponse now slimmer, Component now slim, maybe even a short read-only CookieHelper possible (for cookie infos regarding which tabs are currently open or which button is selected and other View related cookie information similar to Session stuff used this way)
  • Configuration like Session via Configure class instead of beforeFilter hacks etc
  • keeping BC for component access (and configuration via beforeFilter access)
@lh-import

This comment has been minimized.

lh-import commented Oct 11, 2013

13th Oct 2012, Mark Story said:


I like that configuration would become like configuration elsewhere in the framework. Instead of reading from $_COOKIE directly, couldn't that be an input parameter to the class' constructor? If you're going to make a constructable instance, why are all the properties/methods static? Wouldn't it make more sense to have instances be separate and share the instance?

@lh-import

This comment has been minimized.

lh-import commented Oct 11, 2013

13th Oct 2012, euromark said:


It wasn't my intension to make it constructable, but to ensure, init() and all the configuration setup is only triggered once (therefore the started attribute). Is there a better way of doing it? or is the instance maybe even a desired result?
My guideline here was the "Pseudo constructor" of CakeSession.

I tried to keep it as similar to CakeSession as possible, with full static access.

You should be able to do

$res = CakeCookie::read('foo');

right away, and if init() has not been invoked yet, it will do so internally. but only once, of course.

Not sure if we should keep the separate class attributes (copied over from CookieComponent). The CakeSession class sets Configure::write() and uses those then. No need to write them to the CakeCookie class as static attributes only to re-read them for each operation IMO.

PS: CakeCookie itself is up and running (all tests pass), I need to assert now that BC for CookieComponent is given.

@lh-import

This comment has been minimized.

lh-import commented Oct 11, 2013

13th Oct 2012, euromark said:


After furthing working on it, I am pretty certain that the configuration should completely be covered by core/boostrap and Configure::write() and not be set in beforeFilter etc. This will be the only (small) non-BC-change here which can be added to the upgrade guide.
I think not too many actually modify the cookie configuration so far and even if, moving it into Configure should be fairly easy to manage.

To keep things even smoother, you could also just replace beforeFilters'

 $this->Cookie->httpOnly = true;

with

 CakeCookie::$httpOnly = true;

for example to achieve the same thing.

@markstory

This comment has been minimized.

Member

markstory commented Jun 4, 2014

Closing as the new CookieComponent reduces the need to have a separate class, as it allows per cookie configuration.

@markstory markstory closed this Jun 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment